Skip to content

[CALCITE-7702] JoinAggregateTransposeRule produces a non-equivalent plan when the aggregate with empty input and empty group set - #5171

Merged
mihaibudiu merged 1 commit into
apache:mainfrom
zzwqqq:fix_join_agg_transpose
Aug 11, 2026
Merged

[CALCITE-7702] JoinAggregateTransposeRule produces a non-equivalent plan when the aggregate with empty input and empty group set#5171
mihaibudiu merged 1 commit into
apache:mainfrom
zzwqqq:fix_join_agg_transpose

Conversation

@zzwqqq

@zzwqqq zzwqqq commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7702

Changes Proposed

JoinAggregateTransposeRule can produce a non-equivalent plan when an Aggregate has an empty input and an empty group set. The original Aggregate returns one row, but after pull-up the JOIN columns become group keys and the new Aggregate returns no rows.

For an Aggregate with an empty group set, the rule now applies only when metadata proves that its input is non-empty.
A regression test covers this case.

~ limitations under the License.
-->
<Root>
<TestCase name="testNoPullAggregateWithEmptyInputAndEmptyGroupSet">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a quidem test which exhibits the bug? Reading plans is hard.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing. I added a Quidem test to join-agg-transpose.iq. It checks the query result directly. Without the fix, the query returns no rows instead of (0, 10).

// Pull-up adds group keys and may lose that row. Require the input to be
// known non-empty.
&& (!left.getGroupSet().isEmpty()
|| Boolean.FALSE.equals(mq.isEmpty(left.getInput())))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that this is conservative in the right direction: it never answers "yes" when it could be empty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. mq.isEmpty returns false only when the input is proved to be non-empty. If it is empty or may be empty, the rule does not fire.

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Aug 10, 2026
@mihaibudiu

Copy link
Copy Markdown
Contributor

I will wait a bit to see if @julianhyde is satisfied.

@julianhyde

Copy link
Copy Markdown
Contributor

@mihaibudiu, I've not reviewed the PR but my issues with the spec are resolved.

@mihaibudiu

Copy link
Copy Markdown
Contributor

I think you can squash the commits for merging.

…lan when the aggregate with empty input and empty group set
@zzwqqq
zzwqqq force-pushed the fix_join_agg_transpose branch from ab622f8 to 71a12b1 Compare August 11, 2026 02:25
@zzwqqq

zzwqqq commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

I think you can squash the commits for merging.

Thanks. Done.

@sonarqubecloud

Copy link
Copy Markdown

@mihaibudiu
mihaibudiu merged commit dd067b6 into apache:main Aug 11, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants