colbuilder: fix aggregation with no aggregate funcs #87662
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @msirek)
yuzefovich
force-pushed
the
fix-no-aggregation
branch
from
a3be9cd to
2236dd6
Compare
There was a problem hiding this comment.
Added a test, but it relies on the disabling of some rules, so it'll fail the CI until #87658 is merged. I'm not sure whether it is possible to come up with a regression test without disabling the rules. Do you have an idea if that is possible?
Reviewable status:
There was a problem hiding this comment.
I don't think there's currently a way to disable specific rules in logic tests / from the command line. There probably should be, though. And I think this case would always be normalized away without any rules disabled.
Reviewable status:
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:
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)
yuzefovich
force-pushed
the
fix-no-aggregation
branch
from
2236dd6 to
e4510af
Compare
|
Adjusted the test so that it succeeds without #87658. TFTRs! bors r+ |
|
Build succeeded: |
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.
Epic: CRDB-20535
Release note: None (this shouldn't be a user-visible bug)