[CALCITE-7342] Quidem test support for TopDownGeneralDecorrelator #4704

xiedeyantu · 2025-12-22T10:44:50Z

xiedeyantu · 2025-12-22T10:45:29Z

I haven't submitted a dedicated JIRA to describe this, mainly because I'm unsure if we currently need a configuration for Calcite to support the new TopDownGeneralDecorrelator, as its functionality is still incomplete. However, it seems that Quidm testing is needed during the feature development process. I tried writing an implementation, but I'm not sure if it's appropriate. If anyone thinks it's necessary, I will create a JIRA to describe some of the capabilities available at this stage.

asolimando · 2025-12-22T12:58:33Z

Thanks for working on this, I think this is worth a Jira ticket as it will be a non-trivial change, and I think it's very needed to build confidence on the new decorrelator.

All this might eventually be removed in the future if we agree that the new decorrelator covers what the original one does, without regressions.

xiedeyantu · 2025-12-22T13:48:44Z

Thanks for working on this, I think this is worth a Jira ticket as it will be a non-trivial change, and I think it's very needed to build confidence on the new decorrelator.

All this might eventually be removed in the future if we agree that the new decorrelator covers what the original one does, without regressions.

@asolimando Thanks for your reply. I have filed a jira CALCITE-7342. Seeing your reply, I feel it's time for me to create a Jira instance to document this, even though I'm not sure if it's appropriate to implement it right now. But it will be necessary at some point, so I'll implement it according to my understanding and let everyone see if they accept it.

mihaibudiu · 2025-12-22T22:19:57Z

core/src/test/java/org/apache/calcite/test/CoreQuidemTest.java

 .with(CalciteConnectionProperty.FUN, SqlLibrary.CALCITE.fun)
 .with(CalciteAssert.Config.SCOTT)
 .connect();
+ case "scott-top-down":


This will require doubling the number of "connections" to test everything.
I was thinking to do this by creating a single extra test file that inherits CoreQuidemTests and which sets the new algorithm for all test files and just runs all of them.
(The ones which output plans will fail, though, so something has to be done about that.)

That's a good idea. However, I'm worried that this work will take a long time, and it might be better to migrate the .iq files one by one, or even just a portion of the content within a file? Wouldn't using the same .iq file be quite messy? While we can use !if (enable_new_decorrelator) { ... } to mark it, wouldn't that be too cluttered? Alternatively, could we have the new QuidmCoreTests initially only involve the newly added decorrelate.iq file, and then gradually add the !if statements to the existing files to maintain two different logics?

I will move this discussion in JIRA.

Sorry, due to the significant changes, I have force-push the code.

mihaibudiu

This looks good, but the CI fails, maybe that's expected at this stage.

mihaibudiu · 2025-12-23T22:04:29Z

core/src/main/java/org/apache/calcite/rel/rules/FilterProjectTransposeRule.java

 .replaceIfs(RelCollationTraitDef.INSTANCE,
- () -> Collections.singletonList(
- input.getTraitSet().getTrait(RelCollationTraitDef.INSTANCE)))
+ () -> input.getTraitSet().getTraits(RelCollationTraitDef.INSTANCE))


is this change related to this issue?

Yes, I'm not entirely sure why; I made some adjustments based on the error message. I will revert the changes to RelDistribution later.

> Error while executing command ExplainCommand [sql: with t (a, b) as (select * from (values (60, 'b'))) > select * from t where a in (select deptno from "scott".dept)] > java.sql.SQLException: Error while executing SQL "explain plan for with t (a, b) as (select * from (values (60, 'b'))) > select * from t where a in (select deptno from "scott".dept)": class org.apache.calcite.plan.RelCompositeTrait cannot be cast to class org.apache.calcite.rel.RelCollation (org.apache.calcite.plan.RelCompositeTrait and org.apache.calcite.rel.RelCollation are in unnamed module of loader 'app')

xiedeyantu · 2025-12-23T23:10:45Z

This looks good, but the CI fails, maybe that's expected at this stage.

agg.iq failed, so I temporarily removed this test. I don't know the reason yet; it could be due to multiple factors. I want to get this test process working before analyzing it in detail.

xiedeyantu · 2025-12-24T15:23:53Z

The current CI for this PR seems to have some issues. It should actually pass all checks. You can refer to #4705 for reference.

xiedeyantu · 2025-12-24T23:01:53Z

testkit/src/main/java/org/apache/calcite/test/QuidemTest.java

 final URL inUrl = QuidemTest.class.getResource("/" + n2u(path));
 inFile = Sources.of(requireNonNull(inUrl, "inUrl")).file();
- outFile = replaceDir(inFile, "resources", "quidem");
+ outFile = replaceDir(inFile, "resources", "quidem/"


The previous error was caused by two tests using the same comparison file.

so can we do the overloading trick?

Now we're essentially adding a layer of class name as identifier. Overloading might be a good approach, but it could require duplicating some code or adding a new method, which doesn’t seem to differ much from the current implementation.

silundong · 2025-12-25T01:11:42Z

core/src/main/java/org/apache/calcite/tools/Programs.java

 if (config.forceDecorrelate()) {
 final RelBuilder relBuilder =
 RelFactories.LOGICAL_BUILDER.create(rel.getCluster(), null);
+ if (config.topDownGeneralDecorrelationEnabled()) {


Perhaps we also need to adapt the Program of removing subquery. When using the new decorrelator, We recommend using CoreRules.FILTER_SUB_QUERY_TO_MARK_CORRELATE and CoreRules.PROJECT_SUB_QUERY_TO_MARK_CORRELATE to remove subqueries from Filter and Project.

Good catch! I will fix it later.

After replacing these two rules, left_mark join will be generated in agg.iq, so I removed this test for now.

silundong · 2025-12-27T05:24:13Z

The code looks great, I just have some questions about the quidem test:

We want to use the new decorrelator to cover the test cases in the current iq files. After implementing left mark join in enumerable convention, we can verify the execution results. However, many iq files (such as sub-query.iq) use !plan to verify the plan (the plan is based on the old decorrelator ). How should we handle these?
After we verify that the new decorrelator correctly covers the existing test cases, CoreQuidemTest2 might be removed. Perhaps at that time, CoreQuidemTest will support a configuration similar to set new_decorrelator, allowing users to switch between them in the iq file. Is that the intended approach?

xiedeyantu · 2025-12-27T10:37:07Z

The code looks great, I just have some questions about the quidem test:

We want to use the new decorrelator to cover the test cases in the current iq files. After implementing left mark join in enumerable convention, we can verify the execution results. However, many iq files (such as sub-query.iq) use !plan to verify the plan (the plan is based on the old decorrelator ). How should we handle these?

After we verify that the new decorrelator correctly covers the existing test cases, CoreQuidemTest2 might be removed. Perhaps at that time, CoreQuidemTest will support a configuration similar to set new_decorrelator, allowing users to switch between them in the iq file. Is that the intended approach?

For the first issue, since quidem does not support the !if-!else syntax, I have added enable_new_decorrelater to support the demonstration of the new decorrelator’s execution. For the second issue, I believe we may maintain this state for a long time until we consider the old decorrelator no longer needs maintenance. At that point, we might clean up some of the logic, but it is also possible that both algorithms will continue to coexist. What we need to do now is to gradually include the excluded files in CoreQuidemTest2 one by one and ensure the new decorrelator can execute correctly.

mihaibudiu · 2025-12-27T18:47:46Z

core/src/main/java/org/apache/calcite/prepare/PlannerImpl.java

 final RelOptCluster cluster = RelOptCluster.create(planner, rexBuilder);
 final SqlToRelConverter.Config config =
- sqlToRelConverterConfig.withTrimUnusedFields(false);
+ sqlToRelConverterConfig.withTrimUnusedFields(false)


I would align the with() each on a new line

mihaibudiu · 2025-12-27T18:50:39Z

core/src/main/java/org/apache/calcite/tools/Programs.java

+ final Program oldProgram = of(builder.build(), true, metadataProvider);
+
+ final HepProgramBuilder newBuilder = HepProgram.builder();
+ newBuilder.addRuleCollection(


Do these cover all subquery cases?
I am not sure why the SUM rule is here.

I only found this one instance of logic for handling subqueries; there's nothing else. I'm also a bit confused about why it handles SUM, but since this is code from 2024, I've decided to keep it here for now, we can see CALCITE-6020.

mihaibudiu · 2025-12-27T18:51:31Z

core/src/test/java/org/apache/calcite/test/CoreQuidemTest2.java

+
+ @Override protected Collection<String> data() {
+ final List<String> paths = new ArrayList<>(super.data());
+ paths.remove("sql/agg.iq");


I would comment that these are temporary and will eventually be removed

mihaibudiu · 2025-12-27T18:53:38Z

core/src/test/resources/sql/hep.iq

+ EnumerableTableScan(table=[[scott, EMP]])
+ EnumerableCalc(expr#0..7=[{inputs}], expr#8=[CAST($t6):DECIMAL(12, 2)], expr#9=[5.00:DECIMAL(12, 2)], expr#10=[>($t8, $t9)], expr#11=[IS NOT NULL($t3)], expr#12=[AND($t10, $t11)], EMPNO=[$t0], MGR=[$t3], COMM=[$t6], $condition=[$t12])
+ EnumerableTableScan(table=[[scott, EMP]])
+!plan


Ideally the new decorrelator would make !plan always pass (by essentially ignoring the output), but I am not sure this is even possible with the way quidem works

I think we can't just ignore them; we still need to use !if statements to process them one by one. This is because the SQL plans in the .iq file aren't necessarily covered by other tests (such as RelOptRulesTest), so they still have value, even if we can ignore the !plan comparison. My approach is to handle each case rigorously, although this might be quite extensive.

sonarqubecloud · 2025-12-28T00:57:21Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

xiedeyantu marked this pull request as draft December 22, 2025 10:45

xiedeyantu force-pushed the new-decoorelator branch from 2dfefb4 to bbd70e1 Compare December 22, 2025 13:53

xiedeyantu changed the title ~~Quidem test support TopDownGeneralDecorrelator~~ [CALCITE-7342] Quidem test support for TopDownGeneralDecorrelator Dec 22, 2025

mihaibudiu reviewed Dec 22, 2025

View reviewed changes

xiedeyantu added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Dec 22, 2025

xiedeyantu force-pushed the new-decoorelator branch from bbd70e1 to 913b084 Compare December 23, 2025 10:53

[CALCITE-7342] Quidem test support for TopDownGeneralDecorrelator

5f02045

xiedeyantu force-pushed the new-decoorelator branch from 913b084 to 5f02045 Compare December 23, 2025 13:54

xiedeyantu removed the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Dec 23, 2025

mihaibudiu reviewed Dec 23, 2025

View reviewed changes

Fix quidem test

f28845d

xiedeyantu force-pushed the new-decoorelator branch from 092e031 to f28845d Compare December 24, 2025 14:47

Fix quidem test

47ba12a

xiedeyantu marked this pull request as ready for review December 24, 2025 15:25

xiedeyantu commented Dec 24, 2025

View reviewed changes

silundong reviewed Dec 25, 2025

View reviewed changes

Adapt to new subquery remove rules

3a809cd

xiedeyantu requested review from mihaibudiu and silundong December 27, 2025 01:04

Support enable_new_decorrelator to enable new decoorelator

7365ac2

mihaibudiu approved these changes Dec 27, 2025

View reviewed changes

Code review and Change some names

9633b5a

silundong approved these changes Dec 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CALCITE-7342] Quidem test support for TopDownGeneralDecorrelator #4704

[CALCITE-7342] Quidem test support for TopDownGeneralDecorrelator #4704

xiedeyantu commented Dec 22, 2025 •

edited

Loading

xiedeyantu commented Dec 22, 2025

asolimando commented Dec 22, 2025

xiedeyantu commented Dec 22, 2025

mihaibudiu Dec 22, 2025

xiedeyantu Dec 22, 2025

mihaibudiu Dec 22, 2025

xiedeyantu Dec 23, 2025

mihaibudiu left a comment

mihaibudiu Dec 23, 2025

xiedeyantu Dec 23, 2025

xiedeyantu commented Dec 23, 2025

xiedeyantu commented Dec 24, 2025

xiedeyantu Dec 24, 2025

mihaibudiu Dec 24, 2025

xiedeyantu Dec 24, 2025

silundong Dec 25, 2025

xiedeyantu Dec 25, 2025

xiedeyantu Dec 26, 2025

silundong commented Dec 27, 2025

xiedeyantu commented Dec 27, 2025

mihaibudiu Dec 27, 2025

mihaibudiu Dec 27, 2025

xiedeyantu Dec 28, 2025

mihaibudiu Dec 27, 2025

mihaibudiu Dec 27, 2025

xiedeyantu Dec 28, 2025

sonarqubecloud bot commented Dec 28, 2025

Labels

4 participants

[CALCITE-7342] Quidem test support for TopDownGeneralDecorrelator #4704

Are you sure you want to change the base?

[CALCITE-7342] Quidem test support for TopDownGeneralDecorrelator #4704

Conversation

xiedeyantu commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

xiedeyantu commented Dec 22, 2025

asolimando commented Dec 22, 2025

xiedeyantu commented Dec 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiedeyantu commented Dec 23, 2025

xiedeyantu commented Dec 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silundong commented Dec 27, 2025

xiedeyantu commented Dec 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 28, 2025

Quality Gate passed

Labels

4 participants

xiedeyantu commented Dec 22, 2025 •

edited

Loading