-
Notifications
You must be signed in to change notification settings - Fork 841
refactor: reduce the memory of the HashIndex in the AggregateHashTable #19046
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
base: main
Are you sure you want to change the base?
Conversation
Docker Image for PR
|
This reverts commit 1e27e0f.
Docker Image for PR
|
Docker Image for PR
|
Docker Image for PR
|
Docker Image for PR
|
Docker Image for PR
|
Docker Image for 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Docker Image for PR
|
Docker Image for 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| BlockEntry::Const(Scalar::Boolean(v), _, _) => { | ||
| if *v { | ||
| return Ok(()); | ||
| } | ||
| self.nested.accumulate_keys( |
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.
Constant
IF predicate drops rows
In accumulate_keys the constant predicate branch is inverted: when the filter argument is BlockEntry::Const(Scalar::Boolean(true), …) the function returns immediately and skips accumulating any rows, while the false case falls through to the nested aggregate (lines below). For aggregates using the _if combinator, a constant TRUE filter should include all rows and a constant FALSE should skip them; current logic flips that behavior, producing zeroed aggregates for always-true filters and counting rows when the filter is always false.
Useful? React with 👍 / 👎.
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
Tests
Type of change
This change is