-
Notifications
You must be signed in to change notification settings - Fork 846
feat(query): add group-by type shrinking rule #19177
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
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".
| for rewrite in rewrites.iter() { | ||
| new_group_items[rewrite.position].index = rewrite.shrink_index; | ||
| new_group_items[rewrite.position].scalar = ScalarExpr::BoundColumnRef(BoundColumnRef { | ||
| span: None, | ||
| column: rewrite.shrink_binding.clone(), |
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.
Missing stats for shrink group key causes aggregate panic
When the rule rewrites a group key to use the new shrink_index (group_items[position].index = rewrite.shrink_index), no column statistics are created for that derived column (EvalScalar’s derive_stats returns the child stats unchanged). Aggregate::derive_agg_stats later indexes column_stats[&group.index] and unwraps, so any aggregate that triggers this shrink will panic during planning because the stats map lacks an entry for the new index. Add stats for the shrink column or avoid replacing the group index in stats-based code paths.
Useful? React with 👍 / 👎.
Docker Image for PR
|
Docker Image for PR
|
Docker Image for PR
|
🤖 CI Job Analysis
📊 Summary
❌ NO RETRY NEEDEDAll failures appear to be code/test issues requiring manual fixes. 🔍 Job Details
🤖 AboutAutomated analysis using job annotations to distinguish infrastructure issues (auto-retried) from code/test issues (manual fixes needed). |
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
feat(query): add group-by type shrinking rule
Tests
Type of change
This change is