You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "xuzifu666 (via GitHub)" <gi...@apache.org> on 2026/09/06 05:29:53 UTC
[PR] [CALCITE-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
xuzifu666 opened a new pull request, #5246:
URL: https://github.com/apache/calcite/pull/5246
jira: https://issues.apache.org/jira/browse/CALCITE-739
Base on the jira, I added normalizeComparison(RexNode), which canonicalizes binary comparisons (=`,!=,<,>,<=,>=) so that:
1. When both operands are `RexInputRefs`, the one with the smaller index appears on the left.
2. When one side is an input ref and the other is not (e.g., a literal or complex expression), the input ref appears on the left.
3. The method flips the operator using the existing RexUtil.invert when needed.
In addition applied normalizeComparison in pull, commonFactors, and removeFactor so that equivalent comparisons can be matched as common factors.
**Note**: I have not implemented **the second rule** mentioned in the Jira ticket in this PR. The reasons are the risk of exponential expansion resulting from CNF expansion and the overhead of additional implication checks on high-frequency call paths. Although this would handle more complex absorption scenarios, patterns like `(a OR b) AND ((x AND a) OR (y AND b))` appear relatively infrequently in actual SQL optimization, whereas the proposed CNF expansion and implication checks would impact all code paths that call `pullFactors`.
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 merged PR #5246:
URL: https://github.com/apache/calcite/pull/5246
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5246:
URL: https://github.com/apache/calcite/pull/5246#issuecomment-5731842683
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [92.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246)
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #5246:
URL: https://github.com/apache/calcite/pull/5246#discussion_r3998666520
##########
core/src/main/java/org/apache/calcite/rex/RexUtil.java:
##########
@@ -3005,13 +3007,52 @@ private static LinkedHashMap<RexNode, RexNode> commonFactors(List<RexNode> nodes
private RexNode removeFactor(Map<RexNode, RexNode> factors, RexNode node) {
List<RexNode> list = new ArrayList<>();
for (RexNode operand : RelOptUtil.conjunctions(node)) {
- if (!factors.containsKey(operand)) {
+ RexNode normalized = normalizeComparison(operand);
+ if (!factors.containsKey(normalized)) {
list.add(operand);
}
}
return and(list);
}
+ /**
+ * Normalizes a comparison expression so that, when possible, an input ref
Review Comment:
Done, the Javadoc of normalizeComparison now states that only comparisons involving an input ref are normalized, and that other operand kinds (e.g. two CASTs) are returned unchanged, matching the instanceof RexInputRef logic in the implementation.
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5246:
URL: https://github.com/apache/calcite/pull/5246#issuecomment-5637668647
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [92.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246)
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #5246:
URL: https://github.com/apache/calcite/pull/5246#issuecomment-5603108586
> How come there are no plan changes? Is this analysis used in Calcite at all?
@mihaibudiu RexUtil.pullFactors is currently only used in tests, not in the main optimizer. So the fix corrects the utility method itself but does not change any production query plans.
But its value is more about correctness and future use. It could be adopted by RexSimplify or a rule. The same normalization idea could be applied where it actually matters, e.g., inside RexSimplify.simplifyAnds or a predicate-pulling rule, which would then produce plan changes.
I will investigate how to make improvements in RexSimplify with minimal cost.
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5246:
URL: https://github.com/apache/calcite/pull/5246#discussion_r3997125363
##########
core/src/main/java/org/apache/calcite/rex/RexUtil.java:
##########
@@ -3005,13 +3007,52 @@ private static LinkedHashMap<RexNode, RexNode> commonFactors(List<RexNode> nodes
private RexNode removeFactor(Map<RexNode, RexNode> factors, RexNode node) {
List<RexNode> list = new ArrayList<>();
for (RexNode operand : RelOptUtil.conjunctions(node)) {
- if (!factors.containsKey(operand)) {
+ RexNode normalized = normalizeComparison(operand);
+ if (!factors.containsKey(normalized)) {
list.add(operand);
}
}
return and(list);
}
+ /**
+ * Normalizes a comparison expression so that, when possible, an input ref
Review Comment:
this only seems to work for comparisons of columns; this should be in the javadoc
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5246:
URL: https://github.com/apache/calcite/pull/5246#issuecomment-5637379872
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [92.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246)
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #5246:
URL: https://github.com/apache/calcite/pull/5246#issuecomment-5646163632
Apologies @mihaibudiu , I accidentally squashed the previous commit history with the current changes. Here is an overview of the current modifications:
RexSimplify.absorb now recognizes symmetric comparison forms during matching.
A new helper, equivalentComparison(a, b), treats two comparisons as equal when they differ only in operand order (a = b ≡ b = a, a < b ≡ b > a) — kind must be reversible and operands cross-equal. It replaces the digest-exact components.contains(other) check inside absorb. No expression is ever rewritten; the surviving term keeps its original form. I have actually added tests for RexSimplify; prior to this change, these tests would not pass.
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5246:
URL: https://github.com/apache/calcite/pull/5246#issuecomment-5651113483
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [92.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246)
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5246:
URL: https://github.com/apache/calcite/pull/5246#issuecomment-5575330987
How come there are no plan changes? Is this analysis used in Calcite at all?
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5246:
URL: https://github.com/apache/calcite/pull/5246#issuecomment-5557305744
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5246&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5246&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [94.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5246&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5246)
--
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-739] Extend RexUtil.pullFactors to recognize additional common factors [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #5246:
URL: https://github.com/apache/calcite/pull/5246#issuecomment-5651020634
> If this code is never executed, it means it has very low coverage.
IMO the code should been executed and covered, in both senses:
1. absorb runs on every AND/OR simplification, so the new predicate is on the hot path, not a rare branch.
2. the new symmetric-match branch is exercised directly by testSimplifyComparisonSymmetry: the absorption cases fail under the old code and pass with it, proving the branch is reached and load-bearing.
--
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