-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix Distinct expression lowering
#22970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
rapids-bot
merged 11 commits into
NVIDIA:main
from
rjzamora:fix-unique-expression-dynamic-distinct
Jun 29, 2026
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
beb6fad
fix unique expr lowering to use dynamic planning
rjzamora fcd6b11
Merge branch 'main' into fix-unique-expression-dynamic-distinct
rjzamora 1150fdb
use coderabbit suggestion
rjzamora 19ed80e
Merge branch 'fix-unique-expression-dynamic-distinct' of github.com:r…
rjzamora af08ca0
Merge branch 'main' into fix-unique-expression-dynamic-distinct
rjzamora 7f44158
update test
rjzamora 93dede1
minor safty tweak
rjzamora db1a2e8
Merge remote-tracking branch 'upstream/main' into fix-unique-expressi…
rjzamora bf56338
fix bloom-filter bug
rjzamora 042b9b4
Merge remote-tracking branch 'upstream/main' into fix-unique-expressi…
rjzamora ab90ee0
Merge branch 'main' into fix-unique-expression-dynamic-distinct
rjzamora File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pentschev - Thanks for the review! Just a note that these changes "fix" the hang when this branch is used in the absence of #22995 - Hopefully you don't mind the conflicts this creates in that PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies, I had indeed forgotten to pull the latest changes. However, the current state with latest 042b9b4 is actually back to the original problem, q22 fails:
The previous state I was trying was the latest commit I had locally, which was 19ed80e, and merging that on top of #22997 , that had worked as expected, and all queries passed (without the need for changes to the queries themselves).
With the above being said I want to ask whether the changes coming after 19ed80e are really necessary, or are they fixed by a combination of 19ed80e + #22997 (which includes changes also from #22995 and #22996), WDYT? Once again, I have already verified original changes from this PR + #22997 has everything in a good state, but the same is not true with the current in 042b9b4, which brings back the original issue to Q22. For the sake of simplicity (rerunning everything at scale is time-consuming) I would propose instead merging the changes here only up to and including 19ed80e and then #22995, #22996 and #22997, which I have already confirmed to get us to the state we want to be in.
Let me know what you think. For now I'm changing my approval to block the PR from an accidental merge until we are sure of next steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay - I don't quite understand why that might be the case yet, but that's good to know.
The changes were meant to avoid a hang between 22970 and 22995 being merged. However, I was assuming you would just ignore/replace any changes made to this file.
I definitely don't understand why we would be back to the int-overflow issue with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like something messed up with my image build and I probably ran an incorrect version. Indeed, after rebuilding I can confirm everything works with this PR now. I'm very sorry for the confusion and added work on verifications.