You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "zhuwenzhuang (via GitHub)" <gi...@apache.org> on 2026/02/24 10:52:26 UTC
[PR] [CALCITE-7422] Support large plan optimizaion mode for HepPlanner [calcite]
zhuwenzhuang opened a new pull request, #4803:
URL: https://github.com/apache/calcite/pull/4803
Motivation and details: https://issues.apache.org/jira/browse/CALCITE-7422
Before:
LargePlanBenchmark:100 : 1s
LargePlanBenchmark:1000 : 9s
LargePlanBenchmark:10000 : pretty slow, cannot measure.
After:
LargePlanBenchmark:100 : 1s
LargePlanBenchmark:1000 : 2s
LargePlanBenchmark:10000 : about 60s
Benchmark (unionNum) Mode Cnt Score Error Units
LargePlanBenchmark.testLargeUnionPlan 100 avgt 256.561 ms/op
LargePlanBenchmark.testLargeUnionPlan 1000 avgt 1616.421 ms/op
LargePlanBenchmark.testLargeUnionPlan 10000 avgt 53393.727 ms/op
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2955436508
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -774,7 +862,8 @@ private HepRelVertex applyTransformationResults(
parents.add(parent);
}
- HepRelVertex newVertex = addRelToGraph(bestRel);
+ HepRelVertex newVertex = addRelToGraph(bestRel, null);
+ Set<HepRelVertex> garbageVertexSet = new LinkedHashSet<>();
Review Comment:
you should probably add a comment about this too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4039930240
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [27.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando merged PR #4803:
URL: https://github.com/apache/calcite/pull/4803
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4119212929
Sure, experimental is fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4115185066
We can merge this gated by a flag; to remove the flag and make it the default we have to be able to run all tests without the flag. This could be potentially done in a later PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4115381720
Perhaps I didn't make myself clear. This cache optimization hasn't gone through extensive testing, so strictly speaking, its correctness could be questionable. Just like when we previously introduced new decorrelation and the new DpHyp — even though there was a lot of testing to prove they were correct, we still marked them as experimental, as they hadn't undergone extensive production validation. So, I think the entire feature could be marked as experimental. The above is just to clarify what I didn't explain clearly in my previous reply. As for this PR, I don't have many more suggestions. It can be kept as is, and let's prepare to merge it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "zhuwenzhuang (via GitHub)" <gi...@apache.org>.
zhuwenzhuang commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2958651312
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -136,11 +139,13 @@ public class HepPlanner extends AbstractRelOptPlanner {
*
* <p>Value: match-key ID lists in {@link #firedRulesCache} that contain the key ID.
*/
- private final Multimap<Integer, List<Integer>> firedRulesCacheIndex = HashMultimap.create();
-
+ private final Multimap<Integer, ImmutableIntList> firedRulesCacheIndex = HashMultimap.create();
private boolean enableFiredRulesCache = false;
+ private boolean largePlanMode = false;
Review Comment:
1.Maybe I can run all tests with large plan mode/rule cache/type digest enabled, and see the results.
2.Not adversely affected. If someone uses the match limit and match order to do some match order sensitive stuff, it might break.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "zhuwenzhuang (via GitHub)" <gi...@apache.org>.
zhuwenzhuang commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4045351580
After default depth first iterator replaced by HepVertexIterator.
LargePlanBenchmark:10000 takes 3.6s [10000 union (40000 rel nodes), large plan mode + rule cache + ARBITRARY match order]:
1. CPU Profiler Result
1.1.`fireRule(RelOptRuleCall ruleCall)` takes 70 % CPU.
<img width="1391" height="468" alt="image" src="https://github.com/user-attachments/assets/e396ef92-1483-4ed4-a653-71ac05deb372" />
1.2. garbageCollection's `removeEdge(V source, V target)` takes 23 % CPU.
<img width="414" height="157" alt="image" src="https://github.com/user-attachments/assets/fd468817-f939-4938-8618-7159f7c8e78a" />
Memory Profiler Result:
<img width="1373" height="416" alt="image" src="https://github.com/user-attachments/assets/957cbb5a-a6d9-4132-b657-b2d743464d2f" />
LargePlanBenchmark:100000 takes 68.741 s [400k rel nodes, same configuration]
All perf result of LargePlanBenchmark:
MatchOrder | UnionNum | NodeCount | RuleTransforms | Time (ms)
-- | -- | -- | -- | --
ARBITRARY | 1,000 | 4,000 | 6,006 | 1,043
ARBITRARY | 3,000 | 12,000 | 18,006 | 1,306
ARBITRARY | 10,000 | 40,000 | 60,006 | 3,655
ARBITRARY | 30,000 | 120,000 | 180,006 | 13,040
DEPTH_FIRST | 1,000 | 4,000 | 6,006 | 347
DEPTH_FIRST | 3,000 | 12,000 | 18,006 | 1,068
DEPTH_FIRST | 10,000 | 40,000 | 60,006 | 4,165
DEPTH_FIRST | 30,000 | 120,000 | 180,006 | 12,898
BOTTOM_UP | 1,000 | 4,000 | 6,006 | 1,145
BOTTOM_UP | 3,000 | 12,000 | 18,006 | 10,152
TOP_DOWN | 1,000 | 4,000 | 6,006 | 1,193
TOP_DOWN | 3,000 | 12,000 | 18,006 | 8,074
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "zhuwenzhuang (via GitHub)" <gi...@apache.org>.
zhuwenzhuang commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2969270006
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -136,11 +139,13 @@ public class HepPlanner extends AbstractRelOptPlanner {
*
* <p>Value: match-key ID lists in {@link #firedRulesCache} that contain the key ID.
*/
- private final Multimap<Integer, List<Integer>> firedRulesCacheIndex = HashMultimap.create();
-
+ private final Multimap<Integer, ImmutableIntList> firedRulesCacheIndex = HashMultimap.create();
private boolean enableFiredRulesCache = false;
+ private boolean largePlanMode = false;
Review Comment:
Thank you for the review. @xiedeyantu @asolimando
I added regression coverage for HepPlanner-related cases.
I also added a concrete compatibility-sensitive case in HepPlannerTest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2958566036
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -136,11 +139,13 @@ public class HepPlanner extends AbstractRelOptPlanner {
*
* <p>Value: match-key ID lists in {@link #firedRulesCache} that contain the key ID.
*/
- private final Multimap<Integer, List<Integer>> firedRulesCacheIndex = HashMultimap.create();
-
+ private final Multimap<Integer, ImmutableIntList> firedRulesCacheIndex = HashMultimap.create();
private boolean enableFiredRulesCache = false;
+ private boolean largePlanMode = false;
Review Comment:
Thank you for the explanation. Regarding the first question, my concern is whether a single case can effectively cover a wide range of scenarios. As for the second question, while it is true that HepPlanner supports several traversal strategies, I believe that the current optimization should not interfere with these traversal methods—since they are currently supported, we must ensure backward compatibility. If a specific traversal strategy were to be adversely affected, shouldn't we consider disabling this optimization in such instances? I would appreciate it if you could let me know whether my understanding is correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4115302189
I don't object to merging this PR — it's a very good optimization. I'm not particularly worried about correctness either; it's just that more comprehensive testing would be more convincing. Maybe I'm overthinking, but I'm concerned that this flag might never be removed. Perhaps we could add a little more context, marking it as an experimental feature? I have no other suggestions beyond that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4046218677
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [26.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4102855876
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [63.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2955433875
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -470,7 +541,13 @@ private void applyRules(HepProgram.State programState,
// To the extent possible, pick up where we left
// off; have to create a new iterator because old
// one was invalidated by transformation.
- iter = getGraphIterator(programState, newVertex);
+ if (!useHepVertexIterator) {
+ iter = getGraphIterator(programState, newVertex);
+ } else {
+ // Continue from newVertex and keep previous iterator status.
+ // It prevents revisiting the large plan's stable subgraph from root.
Review Comment:
You should add this information to a comment, it seems useful
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2963676665
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -136,11 +139,13 @@ public class HepPlanner extends AbstractRelOptPlanner {
*
* <p>Value: match-key ID lists in {@link #firedRulesCache} that contain the key ID.
*/
- private final Multimap<Integer, List<Integer>> firedRulesCacheIndex = HashMultimap.create();
-
+ private final Multimap<Integer, ImmutableIntList> firedRulesCacheIndex = HashMultimap.create();
private boolean enableFiredRulesCache = false;
+ private boolean largePlanMode = false;
Review Comment:
For the first point, I'm concerned that a one-time review of the results doesn't establish a standardized process for future iterations. For the second point, I want to ask whether this optimization can avoid these issues, or if it can automatically fall back to a degraded mode with certain constraints when users employ such methods. Is this feasible to implement?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4045778642
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [26.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimizaion mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-3952823819
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [20.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4103475548
Actually, my personal opinion is that we should enable this configuration by default (or perhaps we don't even need to add such a configuration), but only apply it to traversal methods that are backward compatible. If it cannot support all traversal methods, wouldn't it be better to add a configuration parameter here? Since you mentioned that this optimization is beneficial and never worse in any scenario. Although you added many tests (I'm not sure if these tests can be run successfully this way, but I assume they can), I feel it's still not enough. Perhaps we could seek opinions from @mihaibudiu and @asolimando .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "zhuwenzhuang (via GitHub)" <gi...@apache.org>.
zhuwenzhuang commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4134638497
Thanks @mihaibudiu @xiedeyantu @asolimando for the helpful reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4040154951
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [27.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4088473821
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [26.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "zhuwenzhuang (via GitHub)" <gi...@apache.org>.
zhuwenzhuang commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2958513522
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -136,11 +139,13 @@ public class HepPlanner extends AbstractRelOptPlanner {
*
* <p>Value: match-key ID lists in {@link #firedRulesCache} that contain the key ID.
*/
- private final Multimap<Integer, List<Integer>> firedRulesCacheIndex = HashMultimap.create();
-
+ private final Multimap<Integer, ImmutableIntList> firedRulesCacheIndex = HashMultimap.create();
private boolean enableFiredRulesCache = false;
+ private boolean largePlanMode = false;
Review Comment:
1) LargePlanBenchmark includes a test compare different match orders’ rule match count. it is a specific test to verify the correctness.
2) I think this also depends on how the community views the specific compatibility impact of this change. The optimization still guarantees that planning does not stop until no more rules can be matched.
However, if a user has some hacky rules that rely on the traversal order strictly following a fixed pattern, the optimization result may change for those users. I believe such users are actually using HepPlanner incorrectly. If everyone thinks the risk of enabling this by default is acceptable, I would prefer to turn this optimization on by default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimizaion mode for HepPlanner [calcite]
Posted by "zhuwenzhuang (via GitHub)" <gi...@apache.org>.
zhuwenzhuang commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-3958783411
A better iterater implementation of DFS/BFS is needed. I will optimize this later (unavailable for the next two weeks).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimizaion mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-3953273941
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [20.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimizaion mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-3957163732
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [20.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2964956551
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -136,11 +139,13 @@ public class HepPlanner extends AbstractRelOptPlanner {
*
* <p>Value: match-key ID lists in {@link #firedRulesCache} that contain the key ID.
*/
- private final Multimap<Integer, List<Integer>> firedRulesCacheIndex = HashMultimap.create();
-
+ private final Multimap<Integer, ImmutableIntList> firedRulesCacheIndex = HashMultimap.create();
private boolean enableFiredRulesCache = false;
+ private boolean largePlanMode = false;
Review Comment:
I agree with @xiedeyantu for 1., it's OK for now but unfortunately it won't protect from future regressions.
For 2., I think I got what you mean, but if you could provide a concrete example, it would help making sure we are aligned, backwards compatibility is a major concern for libraries like Calcite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4115324896
The flag is a flag - what would make it experimental? An additional comment?
It is hard to remove flags in Calcite, because you never know if someone may rely on some features.
But you have to make progress to. I am just saying we can leave the removal for another PR, which would include the requisite testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4040109485
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [27.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4040356618
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [27.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4803:
URL: https://github.com/apache/calcite/pull/4803#issuecomment-4134752128
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4803&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4803&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [63.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4803&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4803)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2958398992
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -136,11 +139,13 @@ public class HepPlanner extends AbstractRelOptPlanner {
*
* <p>Value: match-key ID lists in {@link #firedRulesCache} that contain the key ID.
*/
- private final Multimap<Integer, List<Integer>> firedRulesCacheIndex = HashMultimap.create();
-
+ private final Multimap<Integer, ImmutableIntList> firedRulesCacheIndex = HashMultimap.create();
private boolean enableFiredRulesCache = false;
+ private boolean largePlanMode = false;
Review Comment:
I'm not very familiar with this optimization; I only took a quick look, but it seems to be quite effective. I do have one concern, though: should we add a specific test to verify the correctness of the execution plan? Alternatively, could we simply remove the `largePlanMode` configuration entirely (or set it to `true` for the purpose of comparative testing) and let all existing test cases validate the plan's correctness?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7422] Support large plan optimization mode for HepPlanner [calcite]
Posted by "zhuwenzhuang (via GitHub)" <gi...@apache.org>.
zhuwenzhuang commented on code in PR #4803:
URL: https://github.com/apache/calcite/pull/4803#discussion_r2958529906
##########
core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java:
##########
@@ -136,11 +139,13 @@ public class HepPlanner extends AbstractRelOptPlanner {
*
* <p>Value: match-key ID lists in {@link #firedRulesCache} that contain the key ID.
*/
- private final Multimap<Integer, List<Integer>> firedRulesCacheIndex = HashMultimap.create();
-
+ private final Multimap<Integer, ImmutableIntList> firedRulesCacheIndex = HashMultimap.create();
private boolean enableFiredRulesCache = false;
+ private boolean largePlanMode = false;
Review Comment:
And if the type digest flag is false by default, the main performance bottleneck is type digest hashing and comparison.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org