You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2022/01/04 07:38:58 UTC
[GitHub] [calcite] xiayutianwei1994 opened a new pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((a or d) and (a or c) and a and b) to (a and b) (Ziwei Liu)
xiayutianwei1994 opened a new pull request #2673:
URL: https://github.com/apache/calcite/pull/2673
Enhance simplify to reduce ((a or d) and (a or c) and a and b) to (a and b) (Ziwei Liu)
--
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
[GitHub] [calcite] xiayutianwei1994 commented on a change in pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on a change in pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#discussion_r779548486
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -1506,8 +1506,46 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
return simplifyAnd2(terms, notTerms);
}
+ boolean satisfy(Set<RexNode> condition, RexNode term) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(term, terms);
+ return condition.containsAll(terms);
+ }
+
+ boolean satisfyForUnknownAsFalse(Set<RexNode> condition, Set<RexNode> notNullOperands,
+ RexNode node) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(node, terms);
+ for (RexNode term : terms) {
+ if (!condition.contains(term) && !(term.getKind() == SqlKind.IS_NOT_NULL && notNullOperands
+ .contains(((RexCall) term).getOperands().get(0)))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ boolean containSatisfyTerm(RexNode node, java.util.function.Predicate<RexNode> satisfy) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeDisjunction(node, terms);
+ for (RexNode term : terms) {
+ if (satisfy.test(term)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// package-protected only to support a deprecated method; treat as private
RexNode simplifyAnd2(List<RexNode> terms, List<RexNode> notTerms) {
+ Set<RexNode> termsSet = new HashSet<>(terms);
Review comment:
Thanks for your comment.
If use unknown as unknown, some predicate will not be simplified to IS NOT NULL or IS NULL like the third test case I added.
And sometimes, predicate like (C0+C1) IS NOT NULL will not be pulled up.
There also has another question: If (A OR B) AND A, predicate A is nondeterministic, whether it can be simplified to A.
Now we only pull up predicate which is deterministic.
--
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
[GitHub] [calcite] xiayutianwei1994 commented on a change in pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on a change in pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#discussion_r779548486
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -1506,8 +1506,46 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
return simplifyAnd2(terms, notTerms);
}
+ boolean satisfy(Set<RexNode> condition, RexNode term) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(term, terms);
+ return condition.containsAll(terms);
+ }
+
+ boolean satisfyForUnknownAsFalse(Set<RexNode> condition, Set<RexNode> notNullOperands,
+ RexNode node) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(node, terms);
+ for (RexNode term : terms) {
+ if (!condition.contains(term) && !(term.getKind() == SqlKind.IS_NOT_NULL && notNullOperands
+ .contains(((RexCall) term).getOperands().get(0)))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ boolean containSatisfyTerm(RexNode node, java.util.function.Predicate<RexNode> satisfy) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeDisjunction(node, terms);
+ for (RexNode term : terms) {
+ if (satisfy.test(term)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// package-protected only to support a deprecated method; treat as private
RexNode simplifyAnd2(List<RexNode> terms, List<RexNode> notTerms) {
+ Set<RexNode> termsSet = new HashSet<>(terms);
Review comment:
Thanks for your comment.
If use unknown as unknown, some predicate will not be simplified to IS NOT NULL or IS NULL like the third test case I added.
There also has another question: If (A OR B) AND A, predicate A is nondeterministic, whether it can be simplified to A. I think is can not be simplified.
Now we only pull up predicate which is deterministic.
If the upper question's answer is can not simplify, change some code in `simplifyIsNull/simpliftIsNotNull` may handle this logic when unknown as false.
--
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
[GitHub] [calcite] xiayutianwei1994 commented on a change in pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on a change in pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#discussion_r779548486
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -1506,8 +1506,46 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
return simplifyAnd2(terms, notTerms);
}
+ boolean satisfy(Set<RexNode> condition, RexNode term) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(term, terms);
+ return condition.containsAll(terms);
+ }
+
+ boolean satisfyForUnknownAsFalse(Set<RexNode> condition, Set<RexNode> notNullOperands,
+ RexNode node) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(node, terms);
+ for (RexNode term : terms) {
+ if (!condition.contains(term) && !(term.getKind() == SqlKind.IS_NOT_NULL && notNullOperands
+ .contains(((RexCall) term).getOperands().get(0)))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ boolean containSatisfyTerm(RexNode node, java.util.function.Predicate<RexNode> satisfy) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeDisjunction(node, terms);
+ for (RexNode term : terms) {
+ if (satisfy.test(term)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// package-protected only to support a deprecated method; treat as private
RexNode simplifyAnd2(List<RexNode> terms, List<RexNode> notTerms) {
+ Set<RexNode> termsSet = new HashSet<>(terms);
Review comment:
Thanks for your comment.
If use unknown as unknown, some predicate will not be simplified to IS NOT NULL or IS NULL like the third test case I added.
And sometimes, predicate like (C0+C1) IS NOT NULL will not be pulled up. Because we only pull up predicate like C0 IS NOT NULL.
There also has another question: If (A OR B) AND A, predicate A is nondeterministic, whether it can be simplified to A. I think is can not be simplified.
Now we only pull up predicate which is deterministic.
If the upper question's answer is can not simplify, change some code in `simplifyIsNull/simpliftIsNotNull` may handle this logic when unknown as false.
--
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
[GitHub] [calcite] xiayutianwei1994 commented on pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#issuecomment-1005452781
> @xiayutianwei1994 Until we have a final state, please don't force push the commit before that.
Ok, Thanks
--
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
[GitHub] [calcite] xiayutianwei1994 commented on a change in pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on a change in pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#discussion_r781205240
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -1506,8 +1506,46 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
return simplifyAnd2(terms, notTerms);
}
+ boolean satisfy(Set<RexNode> condition, RexNode term) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(term, terms);
+ return condition.containsAll(terms);
+ }
+
+ boolean satisfyForUnknownAsFalse(Set<RexNode> condition, Set<RexNode> notNullOperands,
+ RexNode node) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(node, terms);
+ for (RexNode term : terms) {
+ if (!condition.contains(term) && !(term.getKind() == SqlKind.IS_NOT_NULL && notNullOperands
+ .contains(((RexCall) term).getOperands().get(0)))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ boolean containSatisfyTerm(RexNode node, java.util.function.Predicate<RexNode> satisfy) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeDisjunction(node, terms);
+ for (RexNode term : terms) {
+ if (satisfy.test(term)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// package-protected only to support a deprecated method; treat as private
RexNode simplifyAnd2(List<RexNode> terms, List<RexNode> notTerms) {
+ Set<RexNode> termsSet = new HashSet<>(terms);
Review comment:
I have updated the code, please review it again.
Thanks very much.
--
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
[GitHub] [calcite] kgyrtkirk commented on a change in pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#discussion_r778963281
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -1506,8 +1506,46 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
return simplifyAnd2(terms, notTerms);
}
+ boolean satisfy(Set<RexNode> condition, RexNode term) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(term, terms);
+ return condition.containsAll(terms);
+ }
+
+ boolean satisfyForUnknownAsFalse(Set<RexNode> condition, Set<RexNode> notNullOperands,
+ RexNode node) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(node, terms);
+ for (RexNode term : terms) {
+ if (!condition.contains(term) && !(term.getKind() == SqlKind.IS_NOT_NULL && notNullOperands
+ .contains(((RexCall) term).getOperands().get(0)))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ boolean containSatisfyTerm(RexNode node, java.util.function.Predicate<RexNode> satisfy) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeDisjunction(node, terms);
+ for (RexNode term : terms) {
+ if (satisfy.test(term)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// package-protected only to support a deprecated method; treat as private
RexNode simplifyAnd2(List<RexNode> terms, List<RexNode> notTerms) {
+ Set<RexNode> termsSet = new HashSet<>(terms);
Review comment:
I think we already have similar logic here - based on the predicates - and most likely we only miss the handling of an existing predicate somewhere around in the `simplifyIsNull/simpliftIsNotNull` methods
I think this approach is a little bit too much by the way as at first glance I would be afraid that it would make "unsafe" simplifications as well
let me know if you think otherwise - I'll try to take a closer look tomorrow
--
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
[GitHub] [calcite] xiayutianwei1994 commented on pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#issuecomment-1008889998
I have updated the code, please review it again.
Thanks very much.
--
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
[GitHub] [calcite] NobiGo commented on pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#issuecomment-1005452337
@xiayutianwei1994 Until we have a final state, please don't force push the commit before 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
[GitHub] [calcite] xiayutianwei1994 commented on a change in pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on a change in pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#discussion_r779548486
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -1506,8 +1506,46 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
return simplifyAnd2(terms, notTerms);
}
+ boolean satisfy(Set<RexNode> condition, RexNode term) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(term, terms);
+ return condition.containsAll(terms);
+ }
+
+ boolean satisfyForUnknownAsFalse(Set<RexNode> condition, Set<RexNode> notNullOperands,
+ RexNode node) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(node, terms);
+ for (RexNode term : terms) {
+ if (!condition.contains(term) && !(term.getKind() == SqlKind.IS_NOT_NULL && notNullOperands
+ .contains(((RexCall) term).getOperands().get(0)))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ boolean containSatisfyTerm(RexNode node, java.util.function.Predicate<RexNode> satisfy) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeDisjunction(node, terms);
+ for (RexNode term : terms) {
+ if (satisfy.test(term)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// package-protected only to support a deprecated method; treat as private
RexNode simplifyAnd2(List<RexNode> terms, List<RexNode> notTerms) {
+ Set<RexNode> termsSet = new HashSet<>(terms);
Review comment:
Thanks for your comment.
If use unknown as unknown, some predicate will not be simplified to IS NOT NULL or IS NULL like the third test case I added.
And sometimes, predicate like (C0+C1) IS NOT NULL will not be pulled up. Because we only pull up predicate like C0 IS NOT NULL.
There also has another question: If (A OR B) AND A, predicate A is nondeterministic, whether it can be simplified to A.
Now we only pull up predicate which is deterministic.
If the upper question's answer is can not simplify, change some code in `simplifyIsNull/simpliftIsNotNull` may handle this logic when unknown as false.
--
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
[GitHub] [calcite] xiayutianwei1994 commented on a change in pull request #2673: [CALCITE-4910] Enhance simplify to reduce ((A OR D) and (A OR C) AND A AND B) to (A AND B) (Ziwei Liu)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on a change in pull request #2673:
URL: https://github.com/apache/calcite/pull/2673#discussion_r779548486
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -1506,8 +1506,46 @@ RexNode simplifyAnd(RexCall e, RexUnknownAs unknownAs) {
return simplifyAnd2(terms, notTerms);
}
+ boolean satisfy(Set<RexNode> condition, RexNode term) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(term, terms);
+ return condition.containsAll(terms);
+ }
+
+ boolean satisfyForUnknownAsFalse(Set<RexNode> condition, Set<RexNode> notNullOperands,
+ RexNode node) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeConjunction(node, terms);
+ for (RexNode term : terms) {
+ if (!condition.contains(term) && !(term.getKind() == SqlKind.IS_NOT_NULL && notNullOperands
+ .contains(((RexCall) term).getOperands().get(0)))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ boolean containSatisfyTerm(RexNode node, java.util.function.Predicate<RexNode> satisfy) {
+ List<RexNode> terms = new ArrayList<>();
+ RelOptUtil.decomposeDisjunction(node, terms);
+ for (RexNode term : terms) {
+ if (satisfy.test(term)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// package-protected only to support a deprecated method; treat as private
RexNode simplifyAnd2(List<RexNode> terms, List<RexNode> notTerms) {
+ Set<RexNode> termsSet = new HashSet<>(terms);
Review comment:
Thanks for your comment.
If use unknown as unknown, some predicate will not be simplified to IS NOT NULL or IS NULL like the third test case I added.
And sometimes, predicate like (C0+C1) IS NOT NULL will not be pulled up. Because we only pull up predicate like C0 IS NOT NULL.
There also has another question: If (A OR B) AND A, predicate A is nondeterministic, whether it can be simplified to A.
Now we only pull up predicate which is deterministic.
--
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