Skip to content

[CALCITE-7242] Implement a rule to eliminate LITERAL_AGG so that other databases can handle it #4598

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.

Sign up for GitHub

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

Jump to bottom
Open
iwanttobepowerful wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[CALCITE-7242] Implement a rule to eliminate LITERAL_AGG so that other databases can handle it #4598

iwanttobepowerful wants to merge 2 commits into from

Conversation

@iwanttobepowerful
Copy link
Contributor

@iwanttobepowerful iwanttobepowerful commented Oct 24, 2025

@iwanttobepowerful iwanttobepowerful force-pushed the main branch 2 times, most recently from aefb4ad to f7f2396 Compare October 24, 2025 07:55
@iwanttobepowerful
Copy link
Contributor Author

iwanttobepowerful commented Oct 24, 2025

For example:

select e.deptno, e.deptno < some (select deptno from emp where emp.ename = e.ename) as v from emp as e;
LogicalProject(DEPTNO=[$1], V=[OR(AND(IS TRUE(<($1, $3)), IS NOT TRUE(OR(IS NULL($6), =($4, 0)))), AND(IS TRUE(>($4, $5)), null, IS NOT TRUE(OR(IS NULL($6), =($4, 0))), IS NOT TRUE(<($1, $3))), AND(<($1, $3), IS NOT TRUE(OR(IS NULL($6), =($4, 0))), IS NOT TRUE(<($1, $3)), IS NOT TRUE(>($4, $5))))]), id = 7108
  LogicalJoin(condition=[IS NOT DISTINCT FROM($0, $2)], joinType=[left]), id = 7106
    LogicalProject(ename=[$1], deptno=[$8]), id = 7088
      LogicalTableScan(table=[[scott, emp]]), id = 7017
    LogicalProject(ename=[$0], m=[$2], c=[CASE(IS NOT NULL($3), $3, 0)], d=[CASE(IS NOT NULL($4), $4, 0)], trueLiteral=[$5]), id = 7104
      LogicalJoin(condition=[IS NOT DISTINCT FROM($0, $1)], joinType=[left]), id = 7102
        LogicalAggregate(group=[{0}]), id = 7093
          LogicalProject(ename=[$1]), id = 7091
            LogicalTableScan(table=[[scott, emp]]), id = 7017
        LogicalAggregate(group=[{0}], m=[MAX($1)], c=[COUNT()], d=[COUNT($1)], trueLiteral=[LITERAL_AGG(true)]), id = 7100
          LogicalProject(ename=[$1], DEPTNO=[$8]), id = 7098
            LogicalFilter(condition=[IS NOT NULL($1)]), id = 7096
              LogicalTableScan(table=[[scott, emp]]), id = 7017
I20251024 08:00:51.245133 767227 RuleEventLogger.java:45] 7b4b20a3988a9436:cf9ea6a600000000] call#1505: Apply rule [ExtractLiteralAgg] to [rel#7116:LogicalAggregate]
I20251024 08:00:51.245357 767227 RuleEventLogger.java:45] 7b4b20a3988a9436:cf9ea6a600000000] call#1506: Apply rule [ExtractLiteralAgg] to [rel#7123:LogicalAggregate]
I20251024 08:00:51.245544 767227 RuleEventLogger.java:54] 7b4b20a3988a9436:cf9ea6a600000000] call#1506: Full plan for rule input [rel#7123:LogicalAggregate]: 
LogicalAggregate(group=[{0}], m=[MAX($1)], c=[COUNT()], d=[COUNT($1)], trueLiteral=[LITERAL_AGG(true)])
  LogicalProject(ename=[$1], DEPTNO=[$8])
    LogicalFilter(condition=[IS NOT NULL($1)])
      LogicalTableScan(table=[[scott, emp]])
I20251024 08:00:51.245569 767227 RuleEventLogger.java:60] 7b4b20a3988a9436:cf9ea6a600000000] call#1506: Rule [ExtractLiteralAgg] produced [rel#7134:LogicalProject]
I20251024 08:00:51.245609 767227 RuleEventLogger.java:62] 7b4b20a3988a9436:cf9ea6a600000000] call#1506: Full plan for [rel#7134:LogicalProject]:
LogicalProject(ename=[$0], m=[$1], c=[$2], d=[$3], trueLiteral=[true])
  LogicalAggregate(group=[{0}], m=[MAX($1)], c=[COUNT()], d=[COUNT($1)])
    LogicalProject(ename=[$1], DEPTNO=[$8])
      LogicalFilter(condition=[IS NOT NULL($1)])
        LogicalTableScan(table=[[scott, emp]])

@iwanttobepowerful iwanttobepowerful force-pushed the main branch 4 times, most recently from 8c4ff2c to 3562768 Compare October 24, 2025 10:55
@xiedeyantu
Copy link
Member

xiedeyantu commented Oct 24, 2025

For example:

select e.deptno, e.deptno < some (select deptno from emp where emp.ename = e.ename) as v from emp as e;
LogicalProject(DEPTNO=[$1], V=[OR(AND(IS TRUE(<($1, $3)), IS NOT TRUE(OR(IS NULL($6), =($4, 0)))), AND(IS TRUE(>($4, $5)), null, IS NOT TRUE(OR(IS NULL($6), =($4, 0))), IS NOT TRUE(<($1, $3))), AND(<($1, $3), IS NOT TRUE(OR(IS NULL($6), =($4, 0))), IS NOT TRUE(<($1, $3)), IS NOT TRUE(>($4, $5))))]), id = 7108
  LogicalJoin(condition=[IS NOT DISTINCT FROM($0, $2)], joinType=[left]), id = 7106
    LogicalProject(ename=[$1], deptno=[$8]), id = 7088
      LogicalTableScan(table=[[scott, emp]]), id = 7017
    LogicalProject(ename=[$0], m=[$2], c=[CASE(IS NOT NULL($3), $3, 0)], d=[CASE(IS NOT NULL($4), $4, 0)], trueLiteral=[$5]), id = 7104
      LogicalJoin(condition=[IS NOT DISTINCT FROM($0, $1)], joinType=[left]), id = 7102
        LogicalAggregate(group=[{0}]), id = 7093
          LogicalProject(ename=[$1]), id = 7091
            LogicalTableScan(table=[[scott, emp]]), id = 7017
        LogicalAggregate(group=[{0}], m=[MAX($1)], c=[COUNT()], d=[COUNT($1)], trueLiteral=[LITERAL_AGG(true)]), id = 7100
          LogicalProject(ename=[$1], DEPTNO=[$8]), id = 7098
            LogicalFilter(condition=[IS NOT NULL($1)]), id = 7096
              LogicalTableScan(table=[[scott, emp]]), id = 7017
I20251024 08:00:51.245133 767227 RuleEventLogger.java:45] 7b4b20a3988a9436:cf9ea6a600000000] call#1505: Apply rule [ExtractLiteralAgg] to [rel#7116:LogicalAggregate]
I20251024 08:00:51.245357 767227 RuleEventLogger.java:45] 7b4b20a3988a9436:cf9ea6a600000000] call#1506: Apply rule [ExtractLiteralAgg] to [rel#7123:LogicalAggregate]
I20251024 08:00:51.245544 767227 RuleEventLogger.java:54] 7b4b20a3988a9436:cf9ea6a600000000] call#1506: Full plan for rule input [rel#7123:LogicalAggregate]: 
LogicalAggregate(group=[{0}], m=[MAX($1)], c=[COUNT()], d=[COUNT($1)], trueLiteral=[LITERAL_AGG(true)])
  LogicalProject(ename=[$1], DEPTNO=[$8])
    LogicalFilter(condition=[IS NOT NULL($1)])
      LogicalTableScan(table=[[scott, emp]])
I20251024 08:00:51.245569 767227 RuleEventLogger.java:60] 7b4b20a3988a9436:cf9ea6a600000000] call#1506: Rule [ExtractLiteralAgg] produced [rel#7134:LogicalProject]
I20251024 08:00:51.245609 767227 RuleEventLogger.java:62] 7b4b20a3988a9436:cf9ea6a600000000] call#1506: Full plan for [rel#7134:LogicalProject]:
LogicalProject(ename=[$0], m=[$1], c=[$2], d=[$3], trueLiteral=[true])
  LogicalAggregate(group=[{0}], m=[MAX($1)], c=[COUNT()], d=[COUNT($1)])
    LogicalProject(ename=[$1], DEPTNO=[$8])
      LogicalFilter(condition=[IS NOT NULL($1)])
        LogicalTableScan(table=[[scott, emp]])

Can this log be converted into a test case? Typically, a rule should have one or more test cases in RelOptRulesTest.java for verification. To go a step further, a corresponding end-to-end test can be added in planner.iq to validate the correctness of the result.

xiedeyantu
xiedeyantu reviewed Oct 24, 2025
View reviewed changes
core/src/main/java/org/apache/calcite/rel/rules/AggregateExtractLiteralAggRule.java Outdated
final List<Integer> literalAggIndices = new ArrayList<>();
for (int i = 0; i < aggCalls.size(); i++) {
final AggregateCall ac = aggCalls.get(i);
if (ac.getAggregation().getName().equals("LITERAL_AGG")) {
Copy link
Member

@xiedeyantu xiedeyantu Oct 24, 2025

Choose a reason for hiding this comment

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

Would it be better to use ac.getAggregation().getKind() == SqlKind.LITERAL_AGG?

core/src/main/java/org/apache/calcite/rel/rules/AggregateExtractLiteralAggRule.java Outdated
import java.util.Map;

/**
* AggregateExtractLiteralAggRule gets rid of the LITERAL_AGG into most databases can handle.
Copy link
Member

@xiedeyantu xiedeyantu Oct 24, 2025

Choose a reason for hiding this comment

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

Javadoc should typically describe what this rule does, specifically the transformation from which type of SQL to which other type of SQL, or from which type of Plan to which other type of Plan.

core/src/main/java/org/apache/calcite/rel/rules/AggregateExtractLiteralAggRule.java Outdated
}

@Override public void onMatch(RelOptRuleCall call) {
final LogicalAggregate aggregate = call.rel(0);
Copy link
Member

@xiedeyantu xiedeyantu Oct 24, 2025

Choose a reason for hiding this comment

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

Is better to use Aggregate?

core/src/main/java/org/apache/calcite/rel/rules/AggregateExtractLiteralAggRule.java Outdated
}

// Create new Aggregate preserving groupSet/groupSets/hints.
final LogicalAggregate newAggregate =
Copy link
Member

@xiedeyantu xiedeyantu Oct 24, 2025

Choose a reason for hiding this comment

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

I noticed that the operators in this rule are all created using the create method(Aggregate and Project). Could they be changed to use RelBuilder?

xiedeyantu
xiedeyantu reviewed Oct 26, 2025
View reviewed changes
core/src/main/java/org/apache/calcite/rel/rules/AggregateExtractLiteralAggRule.java
} else {
// Non-literal aggregate: compute its new output index in newAggregate.
final Integer newAggIndex = oldAggIndexToNewAggIndex.get(origAggIndex);
if (newAggIndex != null) {
Copy link
Member

@xiedeyantu xiedeyantu Oct 26, 2025

Choose a reason for hiding this comment

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

assert newAggIndex != null ？Could the index here be null?

core/src/main/java/org/apache/calcite/rel/rules/CoreRules.java
public static final AggregateGroupingSetsToUnionRule AGGREGATE_GROUPING_SETS_TO_UNION =
AggregateGroupingSetsToUnionRule.Config.DEFAULT.toRule();

/** Rule that gets rid of the LITERAL_AGG into most databases can handle. */
Copy link
Member

@xiedeyantu xiedeyantu Oct 26, 2025

Choose a reason for hiding this comment

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

I think a brief description of how the plan is transformed from one form to another is needed here, rather than describing where the rule will be used.

core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java Outdated
/** Test case of
* <a href="https://issues.apache.org/jira/browse/CALCITE-7242">[CALCITE-7242]
* Implement a rule to eliminate LITERAL_AGG so that other databases can handle it</a>. */
@Test void testAggregateExtractLiteralAggRule1() {
Copy link
Member

@xiedeyantu xiedeyantu Oct 26, 2025

Choose a reason for hiding this comment

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

If a rule requires multiple test cases, it's best to avoid naming them with a simple incrementing number suffix.. This makes it difficult to quickly understand the purpose of each test. For example, this name could be changed to something like testAggregateExtractLiteralAggRuleSomeLessThan.

@xiedeyantu
Copy link
Member

xiedeyantu commented Oct 26, 2025

CI failed, message is:

FAILURE   0.0sec, org.apache.calcite.test.LintTest > testLintLog()
    java.lang.AssertionError: 
    Expected: an empty collection
         but: <[invalid git log message 'address comments and add tests'; Message must start with upper-case letter]>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.apache.calcite.test.LintTest.testLintLog(LintTest.java:308)

@iwanttobepowerful iwanttobepowerful force-pushed the main branch 2 times, most recently from 28328ce to d2f7f9c Compare October 29, 2025 06:08
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 29, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
87.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@github-actions
Copy link

github-actions bot commented Nov 29, 2025

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 29, 2025
@mihaibudiu
Copy link
Contributor

mihaibudiu commented Dec 9, 2025

What is the status of this PR?

@iwanttobepowerful
Copy link
Contributor Author

iwanttobepowerful commented Dec 10, 2025 •
edited
Loading

What is the status of this PR?

@mihaibudiu
I’ll work on this PR once I’ve finished processing the other several PRs.

@github-actions github-actions bot removed the stale label Dec 10, 2025
@iwanttobepowerful iwanttobepowerful force-pushed the main branch 2 times, most recently from 25f57be to 33c290c Compare December 26, 2025 08:36
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 26, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
87.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

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

Reviewers

@xiedeyantu xiedeyantu xiedeyantu left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iwanttobepowerful @xiedeyantu @mihaibudiu