Skip to content

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

Open
zzwqqq wants to merge 2 commits into
apache:mainfrom
zzwqqq:fix_join_agg_transpose
Open

[CALCITE-7702] JoinAggregateTransposeRule produces a non-equivalent plan when the aggregate with empty input and empty group set#5171
zzwqqq wants to merge 2 commits 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.

…lan when the aggregate with empty input and empty group set
~ 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).

@sonarqubecloud

Copy link
Copy Markdown

// 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.

@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.

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