-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
release-22.1: colbuilder: fix aggregation with no aggregate funcs #87681
Conversation
4e21bcf
to
6ae19ae
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
Previously, if there are no aggregate functions to compute, we would plan a special "num fixed tuples" operator that would always return a single tuple when in scalar and no tuples when in non-scalar context, but this is incorrect - we should be returning the same number of tuples as there are groups for non-empty input. The previous setup was correct only when the input is empty. This is now fixed by teaching the ordered aggregator to handle this special case instead. The impact of the bug seems minor since the optimizer should only be creating such plans when some of its rules are disabled. Release note: None (this shouldn't be a user-visible bug)
6ae19ae
to
42bb9d2
Compare
Removed the regression test since the settings to disable rules aren't available on 22.1. |
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.
I should have asked this in the original PR, but does the row engine correctly handle this case? I don't think it will ever be used if vectorize=on
, but should be if it's off. We should probably also add a vectorize=off
version of the test case.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msirek and @yuzefovich)
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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
Yeah, it does - this is actually how we found the bug (due to the difference between outputs of two engines). Already included regression test to |
Backport 1/1 commits from #87662 on behalf of @yuzefovich.
/cc @cockroachdb/release
Previously, if there are no aggregate functions to compute, we would plan a special "num fixed tuples" operator that would always return a single tuple when in scalar and no tuples when in non-scalar context, but this is incorrect - we should be returning the same number of tuples as there are groups for non-empty input. The previous setup was correct only when the input is empty. This is now fixed by teaching the ordered aggregator to handle this special case instead. The impact of the bug seems minor since the optimizer should only be creating such plans when some of its rules are disabled.
Fixes: #87619.
Release note: None (this shouldn't be a user-visible bug)
Release justification: bug fix.