mihaibudiu commented on code in PR #3921:
URL: https://github.com/apache/calcite/pull/3921#discussion_r1721037610
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -2169,57 +2142,106 @@ public static boolean equalType(String desc0,
MutableRel rel0, String desc1,
}
/**
- * Check if filter under join can be pulled up,
+ * Check if calc under join can be pulled up,
* when meeting JoinOnCalc of query unify to Join of target.
* Working in rules: {@link JoinOnLeftCalcToJoinUnifyRule} <br/>
* {@link JoinOnRightCalcToJoinUnifyRule} <br/>
* {@link JoinOnCalcsToJoinUnifyRule} <br/>
*/
- private static boolean canPullUpFilterUnderJoin(JoinRelType joinType,
- @Nullable RexNode leftFilterRexNode, @Nullable RexNode
rightFilterRexNode) {
- if (joinType == JoinRelType.INNER) {
- return true;
- }
- if (joinType == JoinRelType.LEFT
- && (rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())) {
- return true;
- }
- if (joinType == JoinRelType.RIGHT
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue())) {
- return true;
+ private static boolean canPullUpCalcUnderJoin(JoinRelType joinType,
+ @Nullable Pair<RexNode, List<RexNode>> qInput0Explained,
+ @Nullable Pair<RexNode, List<RexNode>> qInput1Explained) {
+ if (qInput0Explained != null
+ && joinType.generatesNullsOn(0)
+ && !isCalcStrong(qInput0Explained)) {
+ return false;
}
- if (joinType == JoinRelType.FULL
- && ((rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue()))) {
- return true;
+ if (qInput1Explained != null
+ && joinType.generatesNullsOn(1)
+ && !isCalcStrong(qInput1Explained)) {
+ return false;
}
- return false;
+ return true;
+ }
+
+ /** Determines if all projects are strong and the condition is always true.
*/
+ private static boolean isCalcStrong(Pair<RexNode, List<RexNode>>
inputExplained) {
+ final RexNode cond = requireNonNull(inputExplained.left, "condition");
+ final List<RexNode> projs = requireNonNull(inputExplained.right,
"projects");
+ return cond.isAlwaysTrue() && projs.stream().allMatch(STRONG::isNull);
}
/**
- * Check if project under join can be pulled up,
- * when meeting JoinOnCalc of query unify to Join of target.
+ * Generates project expressions by shifting and adjusting the nullability
of expressions
+ * based on the provided join targets and inputs.
+ *
+ * <p>Used in the Join rewrite to pull up the calc in query
+ * to the join in mv to ensure operator equivalence. (Already make sure that
pull up is valid).
* Working in rules: {@link JoinOnLeftCalcToJoinUnifyRule} <br/>
* {@link JoinOnRightCalcToJoinUnifyRule} <br/>
* {@link JoinOnCalcsToJoinUnifyRule} <br/>
*/
- private static boolean canPullUpProjectUnderJoin(JoinRelType joinType,
- @Nullable List<RexNode> leftProjects, @Nullable List<RexNode>
rightProjects) {
- if (leftProjects != null && joinType.generatesNullsOn(0)
- && !allProjectsNullable(leftProjects)) {
- return false;
- }
- if (rightProjects != null && joinType.generatesNullsOn(1)
- && !allProjectsNullable(rightProjects)) {
- return false;
+ private static List<RexNode> shiftAndAdjustProjectExpr(MutableJoin query,
MutableJoin target,
Review Comment:
I think JavaDoc at least for some parameters could also help.
In particular, what is the target?
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -2169,57 +2142,106 @@ public static boolean equalType(String desc0,
MutableRel rel0, String desc1,
}
/**
- * Check if filter under join can be pulled up,
+ * Check if calc under join can be pulled up,
* when meeting JoinOnCalc of query unify to Join of target.
* Working in rules: {@link JoinOnLeftCalcToJoinUnifyRule} <br/>
* {@link JoinOnRightCalcToJoinUnifyRule} <br/>
* {@link JoinOnCalcsToJoinUnifyRule} <br/>
*/
- private static boolean canPullUpFilterUnderJoin(JoinRelType joinType,
- @Nullable RexNode leftFilterRexNode, @Nullable RexNode
rightFilterRexNode) {
- if (joinType == JoinRelType.INNER) {
- return true;
- }
- if (joinType == JoinRelType.LEFT
- && (rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())) {
- return true;
- }
- if (joinType == JoinRelType.RIGHT
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue())) {
- return true;
+ private static boolean canPullUpCalcUnderJoin(JoinRelType joinType,
+ @Nullable Pair<RexNode, List<RexNode>> qInput0Explained,
+ @Nullable Pair<RexNode, List<RexNode>> qInput1Explained) {
+ if (qInput0Explained != null
+ && joinType.generatesNullsOn(0)
+ && !isCalcStrong(qInput0Explained)) {
+ return false;
}
- if (joinType == JoinRelType.FULL
- && ((rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue()))) {
- return true;
+ if (qInput1Explained != null
+ && joinType.generatesNullsOn(1)
+ && !isCalcStrong(qInput1Explained)) {
+ return false;
}
- return false;
+ return true;
+ }
+
+ /** Determines if all projects are strong and the condition is always true.
*/
+ private static boolean isCalcStrong(Pair<RexNode, List<RexNode>>
inputExplained) {
+ final RexNode cond = requireNonNull(inputExplained.left, "condition");
+ final List<RexNode> projs = requireNonNull(inputExplained.right,
"projects");
+ return cond.isAlwaysTrue() && projs.stream().allMatch(STRONG::isNull);
}
/**
- * Check if project under join can be pulled up,
- * when meeting JoinOnCalc of query unify to Join of target.
+ * Generates project expressions by shifting and adjusting the nullability
of expressions
+ * based on the provided join targets and inputs.
+ *
+ * <p>Used in the Join rewrite to pull up the calc in query
+ * to the join in mv to ensure operator equivalence. (Already make sure that
pull up is valid).
* Working in rules: {@link JoinOnLeftCalcToJoinUnifyRule} <br/>
* {@link JoinOnRightCalcToJoinUnifyRule} <br/>
* {@link JoinOnCalcsToJoinUnifyRule} <br/>
*/
- private static boolean canPullUpProjectUnderJoin(JoinRelType joinType,
- @Nullable List<RexNode> leftProjects, @Nullable List<RexNode>
rightProjects) {
- if (leftProjects != null && joinType.generatesNullsOn(0)
- && !allProjectsNullable(leftProjects)) {
- return false;
- }
- if (rightProjects != null && joinType.generatesNullsOn(1)
- && !allProjectsNullable(rightProjects)) {
- return false;
+ private static List<RexNode> shiftAndAdjustProjectExpr(MutableJoin query,
MutableJoin target,
+ RexBuilder rexBuilder, int leftCount,
+ @Nullable List<RexNode> qInput0Projs, @Nullable List<RelDataTypeField>
qInput0InputFields,
+ @Nullable List<RexNode> qInput1Projs, @Nullable List<RelDataTypeField>
qInput1InputFields) {
+
+ int[] adjustments0 = new int[target.rowType.getFieldCount()];
+ int[] adjustments1 = new int[target.rowType.getFieldCount()];
+
+ if (qInput1Projs != null) {
+ Arrays.fill(adjustments1, fieldCnt(target.getLeft()));
+ }
+
+ // In cases such as JoinOnLeftCalcToJoinUnifyRule and
JoinOnCalcsToJoinUnifyRule rules,
+ // the left calc need to be pulled up the target, initialize the converter.
Review Comment:
should this be "where the left calc needs to be pulled up"?
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -2169,57 +2142,106 @@ public static boolean equalType(String desc0,
MutableRel rel0, String desc1,
}
/**
- * Check if filter under join can be pulled up,
+ * Check if calc under join can be pulled up,
* when meeting JoinOnCalc of query unify to Join of target.
* Working in rules: {@link JoinOnLeftCalcToJoinUnifyRule} <br/>
* {@link JoinOnRightCalcToJoinUnifyRule} <br/>
* {@link JoinOnCalcsToJoinUnifyRule} <br/>
*/
- private static boolean canPullUpFilterUnderJoin(JoinRelType joinType,
- @Nullable RexNode leftFilterRexNode, @Nullable RexNode
rightFilterRexNode) {
- if (joinType == JoinRelType.INNER) {
- return true;
- }
- if (joinType == JoinRelType.LEFT
- && (rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())) {
- return true;
- }
- if (joinType == JoinRelType.RIGHT
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue())) {
- return true;
+ private static boolean canPullUpCalcUnderJoin(JoinRelType joinType,
+ @Nullable Pair<RexNode, List<RexNode>> qInput0Explained,
+ @Nullable Pair<RexNode, List<RexNode>> qInput1Explained) {
+ if (qInput0Explained != null
+ && joinType.generatesNullsOn(0)
+ && !isCalcStrong(qInput0Explained)) {
+ return false;
}
- if (joinType == JoinRelType.FULL
- && ((rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue()))) {
- return true;
+ if (qInput1Explained != null
+ && joinType.generatesNullsOn(1)
+ && !isCalcStrong(qInput1Explained)) {
+ return false;
}
- return false;
+ return true;
+ }
+
+ /** Determines if all projects are strong and the condition is always true.
*/
Review Comment:
not clear what the "projects" are and what the condition is given single
input parameter. Maybe you can add `@param` to describe where inputExplained
comes from.
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -2169,57 +2142,106 @@ public static boolean equalType(String desc0,
MutableRel rel0, String desc1,
}
/**
- * Check if filter under join can be pulled up,
+ * Check if calc under join can be pulled up,
* when meeting JoinOnCalc of query unify to Join of target.
* Working in rules: {@link JoinOnLeftCalcToJoinUnifyRule} <br/>
* {@link JoinOnRightCalcToJoinUnifyRule} <br/>
* {@link JoinOnCalcsToJoinUnifyRule} <br/>
*/
- private static boolean canPullUpFilterUnderJoin(JoinRelType joinType,
- @Nullable RexNode leftFilterRexNode, @Nullable RexNode
rightFilterRexNode) {
- if (joinType == JoinRelType.INNER) {
- return true;
- }
- if (joinType == JoinRelType.LEFT
- && (rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())) {
- return true;
- }
- if (joinType == JoinRelType.RIGHT
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue())) {
- return true;
+ private static boolean canPullUpCalcUnderJoin(JoinRelType joinType,
+ @Nullable Pair<RexNode, List<RexNode>> qInput0Explained,
+ @Nullable Pair<RexNode, List<RexNode>> qInput1Explained) {
+ if (qInput0Explained != null
+ && joinType.generatesNullsOn(0)
+ && !isCalcStrong(qInput0Explained)) {
+ return false;
}
- if (joinType == JoinRelType.FULL
- && ((rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue()))) {
- return true;
+ if (qInput1Explained != null
+ && joinType.generatesNullsOn(1)
+ && !isCalcStrong(qInput1Explained)) {
+ return false;
}
- return false;
+ return true;
+ }
+
+ /** Determines if all projects are strong and the condition is always true.
*/
+ private static boolean isCalcStrong(Pair<RexNode, List<RexNode>>
inputExplained) {
+ final RexNode cond = requireNonNull(inputExplained.left, "condition");
+ final List<RexNode> projs = requireNonNull(inputExplained.right,
"projects");
+ return cond.isAlwaysTrue() && projs.stream().allMatch(STRONG::isNull);
}
/**
- * Check if project under join can be pulled up,
- * when meeting JoinOnCalc of query unify to Join of target.
+ * Generates project expressions by shifting and adjusting the nullability
of expressions
+ * based on the provided join targets and inputs.
+ *
+ * <p>Used in the Join rewrite to pull up the calc in query
+ * to the join in mv to ensure operator equivalence. (Already make sure that
pull up is valid).
* Working in rules: {@link JoinOnLeftCalcToJoinUnifyRule} <br/>
* {@link JoinOnRightCalcToJoinUnifyRule} <br/>
* {@link JoinOnCalcsToJoinUnifyRule} <br/>
*/
- private static boolean canPullUpProjectUnderJoin(JoinRelType joinType,
- @Nullable List<RexNode> leftProjects, @Nullable List<RexNode>
rightProjects) {
- if (leftProjects != null && joinType.generatesNullsOn(0)
- && !allProjectsNullable(leftProjects)) {
- return false;
- }
- if (rightProjects != null && joinType.generatesNullsOn(1)
- && !allProjectsNullable(rightProjects)) {
- return false;
+ private static List<RexNode> shiftAndAdjustProjectExpr(MutableJoin query,
MutableJoin target,
+ RexBuilder rexBuilder, int leftCount,
+ @Nullable List<RexNode> qInput0Projs, @Nullable List<RelDataTypeField>
qInput0InputFields,
+ @Nullable List<RexNode> qInput1Projs, @Nullable List<RelDataTypeField>
qInput1InputFields) {
+
+ int[] adjustments0 = new int[target.rowType.getFieldCount()];
+ int[] adjustments1 = new int[target.rowType.getFieldCount()];
+
+ if (qInput1Projs != null) {
+ Arrays.fill(adjustments1, fieldCnt(target.getLeft()));
+ }
+
+ // In cases such as JoinOnLeftCalcToJoinUnifyRule and
JoinOnCalcsToJoinUnifyRule rules,
+ // the left calc need to be pulled up the target, initialize the converter.
+ RelOptUtil.RexInputConverter converter0 =
+ new RelOptUtil.RexInputConverter(rexBuilder, qInput0InputFields,
+ target.rowType.getFieldList(), adjustments0);
+
+ // In cases such as JoinOnRightCalcToJoinUnifyRule and
JoinOnCalcsToJoinUnifyRule rules,
+ // the right calc need to be pulled up the target, initialize the
converter.
+ RelOptUtil.RexInputConverter converter1 =
+ new RelOptUtil.RexInputConverter(rexBuilder, qInput1InputFields,
+ target.rowType.getFieldList(), adjustments1);
+
+ final List<RexNode> compenProjs = new ArrayList<>();
+ for (int i = 0; i < fieldCnt(query); i++) {
+ RelDataType type = query.rowType.getFieldList().get(i).getType();
+ if (i < leftCount) {
+ if (qInput0Projs == null) {
+ compenProjs.add(new RexInputRef(i, type));
+ } else {
+ RexNode apply = converter0.apply(qInput0Projs.get(i));
+ compenProjs.add(adjustNullability(apply, type, rexBuilder));
+ }
+ } else {
+ if (qInput1Projs == null) {
+ // This is equivalent to a shift, but without a calc on the right
side to pull up,
+ // we know it's a RexInputRef, so converter isn't needed.
+ compenProjs.add(
+ new RexInputRef(i - leftCount + fieldCnt(target.getLeft()),
type));
+ } else {
+ RexNode apply = converter1.apply(qInput1Projs.get(i - leftCount));
+ compenProjs.add(adjustNullability(apply, type, rexBuilder));
+ }
+ }
}
- return true;
+ return compenProjs;
}
- /** Returns whether all projects are nullable. */
- private static boolean allProjectsNullable(List<RexNode> projects) {
- return projects.stream()
- .allMatch(project -> project.getType().isNullable());
+ /** Cast RexNode to the given type if only nullability differs. */
Review Comment:
I would also add "Otherwise throw". The current comment implies that in all
other cases the type is left unchanged.
##########
core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java:
##########
@@ -1138,8 +1138,13 @@ protected final MaterializedViewFixture sql(String
materialize,
/** Test case for
* <a
href="https://issues.apache.org/jira/browse/CALCITE-6501";>[CALCITE-6501]
- * Assertion Error in JoinUnifyRule Due to Type Mismatch</a>. */
- @Test void testLeftProjectOnRightJoinToJoinFailed() {
+ * Assertion Error in JoinUnifyRule Due to Type Mismatch</a>.
+ *
+ * <p>Originally, this test was noMat due to a type mismatch.
Review Comment:
I don't think this kind of comment is useful long term. It is useful for the
reviewers of this PR, but not for maintaining the code. This comment would be
better as a github comment rather than a code comment. Same for the subsequent
comments.
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -2169,57 +2142,106 @@ public static boolean equalType(String desc0,
MutableRel rel0, String desc1,
}
/**
- * Check if filter under join can be pulled up,
+ * Check if calc under join can be pulled up,
* when meeting JoinOnCalc of query unify to Join of target.
* Working in rules: {@link JoinOnLeftCalcToJoinUnifyRule} <br/>
* {@link JoinOnRightCalcToJoinUnifyRule} <br/>
* {@link JoinOnCalcsToJoinUnifyRule} <br/>
*/
- private static boolean canPullUpFilterUnderJoin(JoinRelType joinType,
- @Nullable RexNode leftFilterRexNode, @Nullable RexNode
rightFilterRexNode) {
- if (joinType == JoinRelType.INNER) {
- return true;
- }
- if (joinType == JoinRelType.LEFT
- && (rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())) {
- return true;
- }
- if (joinType == JoinRelType.RIGHT
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue())) {
- return true;
+ private static boolean canPullUpCalcUnderJoin(JoinRelType joinType,
+ @Nullable Pair<RexNode, List<RexNode>> qInput0Explained,
+ @Nullable Pair<RexNode, List<RexNode>> qInput1Explained) {
+ if (qInput0Explained != null
+ && joinType.generatesNullsOn(0)
+ && !isCalcStrong(qInput0Explained)) {
+ return false;
}
- if (joinType == JoinRelType.FULL
- && ((rightFilterRexNode == null || rightFilterRexNode.isAlwaysTrue())
- && (leftFilterRexNode == null || leftFilterRexNode.isAlwaysTrue()))) {
- return true;
+ if (qInput1Explained != null
+ && joinType.generatesNullsOn(1)
+ && !isCalcStrong(qInput1Explained)) {
+ return false;
}
- return false;
+ return true;
+ }
+
+ /** Determines if all projects are strong and the condition is always true.
*/
+ private static boolean isCalcStrong(Pair<RexNode, List<RexNode>>
inputExplained) {
+ final RexNode cond = requireNonNull(inputExplained.left, "condition");
+ final List<RexNode> projs = requireNonNull(inputExplained.right,
"projects");
+ return cond.isAlwaysTrue() && projs.stream().allMatch(STRONG::isNull);
}
/**
- * Check if project under join can be pulled up,
- * when meeting JoinOnCalc of query unify to Join of target.
+ * Generates project expressions by shifting and adjusting the nullability
of expressions
+ * based on the provided join targets and inputs.
+ *
+ * <p>Used in the Join rewrite to pull up the calc in query
+ * to the join in mv to ensure operator equivalence. (Already make sure that
pull up is valid).
* Working in rules: {@link JoinOnLeftCalcToJoinUnifyRule} <br/>
* {@link JoinOnRightCalcToJoinUnifyRule} <br/>
* {@link JoinOnCalcsToJoinUnifyRule} <br/>
*/
- private static boolean canPullUpProjectUnderJoin(JoinRelType joinType,
- @Nullable List<RexNode> leftProjects, @Nullable List<RexNode>
rightProjects) {
- if (leftProjects != null && joinType.generatesNullsOn(0)
- && !allProjectsNullable(leftProjects)) {
- return false;
- }
- if (rightProjects != null && joinType.generatesNullsOn(1)
- && !allProjectsNullable(rightProjects)) {
- return false;
+ private static List<RexNode> shiftAndAdjustProjectExpr(MutableJoin query,
MutableJoin target,
+ RexBuilder rexBuilder, int leftCount,
+ @Nullable List<RexNode> qInput0Projs, @Nullable List<RelDataTypeField>
qInput0InputFields,
+ @Nullable List<RexNode> qInput1Projs, @Nullable List<RelDataTypeField>
qInput1InputFields) {
+
+ int[] adjustments0 = new int[target.rowType.getFieldCount()];
+ int[] adjustments1 = new int[target.rowType.getFieldCount()];
+
+ if (qInput1Projs != null) {
+ Arrays.fill(adjustments1, fieldCnt(target.getLeft()));
+ }
+
+ // In cases such as JoinOnLeftCalcToJoinUnifyRule and
JoinOnCalcsToJoinUnifyRule rules,
+ // the left calc need to be pulled up the target, initialize the converter.
+ RelOptUtil.RexInputConverter converter0 =
+ new RelOptUtil.RexInputConverter(rexBuilder, qInput0InputFields,
+ target.rowType.getFieldList(), adjustments0);
+
+ // In cases such as JoinOnRightCalcToJoinUnifyRule and
JoinOnCalcsToJoinUnifyRule rules,
+ // the right calc need to be pulled up the target, initialize the
converter.
+ RelOptUtil.RexInputConverter converter1 =
+ new RelOptUtil.RexInputConverter(rexBuilder, qInput1InputFields,
+ target.rowType.getFieldList(), adjustments1);
+
+ final List<RexNode> compenProjs = new ArrayList<>();
Review Comment:
what does "compen" mean?
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -127,6 +128,11 @@
*/
public class SubstitutionVisitor {
private static final boolean DEBUG = CalciteSystemProperty.DEBUG.value();
+ private static final Strong STRONG = new Strong() {
Review Comment:
This deserves a small comment. why are input refs assumed to be always null?
I guess it's because you will apply it to some specific types of expressions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
