Skip to content

[CALCITE-7679] RelToSqlConverter generates GROUP BY literals for dial… - #5130

Open
ehds wants to merge 2 commits into
apache:mainfrom
ehds:fix-group-by-iteral
Open

[CALCITE-7679] RelToSqlConverter generates GROUP BY literals for dial…#5130
ehds wants to merge 2 commits into
apache:mainfrom
ehds:fix-group-by-iteral

Conversation

@ehds

@ehds ehds commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

…ects that do not support them when the constant is hidden by nested Projects

Jira Link

CALCITE-7679

Changes Proposed

Before applying AggregateProjectConstantToDummyJoinRule, normalize nested projects using PROJECT_MERGE and PROJECT_REMOVE.

@ehds
ehds force-pushed the fix-group-by-iteral branch from 0a2caeb to c9b21de Compare July 28, 2026 15:36
public final Result visitRoot(RelNode r) {
List<RelOptRule> rules = new ArrayList<>();
if (!this.dialect.supportsGroupByLiteral()) {
rules.add(CoreRules.PROJECT_MERGE);

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.

It's not obvious to me why this solves the problem. Can you explain?

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.

I added a new UT to explain why we need theses two rules.

For this sql:

SELECT id, employee_id
FROM (
  SELECT id, employee_id
  FROM (
    SELECT employee_id, NULL AS id
    FROM employee
  ) AS t1
) AS t2
GROUP BY id, employee_id

It will be transformed to:

LogicalAggregate(group=[{0, 1}])
  LogicalProject(id=[$1], employee_id=[$0])
    LogicalProject(employee_id=[$0], id=[null:NULL])
      JdbcTableScan(table=[[foodmart, employee]])

AggregateProjectConstantToDummyJoinRule (added by CALCITE-4702) only matches the Agg->Project pattern, however, $1 and $0 are not literals, so the rule does not fire.

To fix it, we should remove unnecessary PROJECT operations by these two rules.

Or if you have a better suggestion, I'll follow up.

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.

This works for this particular plan, but why does it do in general what you need?

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.

Indeed, this current solution only handles particular cases.

I think a more general fix would be to wrap the input in a subquery when determining needNewSubQuery if any GROUP BY expression contains a literal.

LogicalAggregate(group=[{0, 1}])
  LogicalProject(id=[$1], employee_id=[$0])
    LogicalProject(employee_id=[$0], id=[null:NULL])
      JdbcTableScan(table=[[foodmart, employee]])

Translate this plan into the following SQL form:

SELECT employee_id, id 
   FROM (SELECT employee_id, null as id from foodmart.employee) as t
group by t.employee_id, t.id

And this approach is similar to the fixes used for other cases,such as CALCITE-7655, CALCITE-4491

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.

In general you should use the Jira to propose a design, and the PR to implement it.
This approach may work.

…ects that do not support them when the constant is hidden by nested Projects
@ehds
ehds force-pushed the fix-group-by-iteral branch from c9b21de to 3cd7d34 Compare July 29, 2026 03:08
@mihaibudiu

Copy link
Copy Markdown
Contributor

I will wait for CI to be green before I review

@ehds
ehds force-pushed the fix-group-by-iteral branch from 4a3f31c to b2e6c4c Compare August 2, 2026 02:18
@mihaibudiu

Copy link
Copy Markdown
Contributor

You can run most of the ci checks locally using ./gradlew build before you submit a commit

…ubQuery if any GROUP BY expression contains a literal
@ehds
ehds force-pushed the fix-group-by-iteral branch from b2e6c4c to b6c19e4 Compare August 2, 2026 12:50
@sonarqubecloud

sonarqubecloud Bot commented Aug 2, 2026

Copy link
Copy Markdown

@ehds
ehds requested a review from mihaibudiu August 3, 2026 03:12
* Returns whether any grouping key of {@code aggregate} is represented by
* a literal expression in this result's {@code SELECT} list.
*
* <p>A literal wrapped in a {@code CAST} is also considered a literal.

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 don't see anything about casts here. What is this comment about?

@mihaibudiu

Copy link
Copy Markdown
Contributor

This generally looks fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants