mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2013177827
##########
core/src/test/java/org/apache/calcite/test/JdbcTest.java:
##########
@@ -3950,6 +3950,24 @@ public void checkOrderBy(final boolean desc,
.returnsUnordered("empid=150; name=Sebastian");
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-6904";>[CALCITE-6904]
+ * IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule</a>. */
+ @Test void testIsNotDistinctFrom() {
+ final String sql = ""
+ + "select \"t1\".\"commission\" from \"hr\".\"emps\" as \"t1\"\n"
+ + "join\n"
+ + "\"hr\".\"emps\" as \"t2\"\n"
+ + "on \"t1\".\"commission\" is not distinct from
\"t2\".\"commission\"";
+ CalciteAssert.hr()
+ .query(sql)
+ .explainContains("")
Review Comment:
why have an empty string for the explain?
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -66,8 +66,36 @@ public static JoinInfo of(RelNode left, RelNode right,
RexNode condition) {
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
filterNulls, nonEquiList);
- return new JoinInfo(ImmutableIntList.copyOf(leftKeys),
- ImmutableIntList.copyOf(rightKeys), ImmutableList.copyOf(nonEquiList));
+
+ /**
+ * In join implementation when the condition is equality (=),
+ * we filter out nulls prior to the join. If the condition is
+ * `x IS NOT DISTINCT FROM y`, the leftKeys={x}, rightKeys={y},
+ * filterNulls={false}. And then {@link
EnumerableHashJoin#implementHashJoin}
+ * will use this JoinInfo to generate equi and non-equi condition,
+ * the filterNulls is ignored. We should keep `IS NOT DISTINCT FROM`
+ * in nonEquiConditions and remove x in leftKeys and y in rightKeys.
+ */
+ assert leftKeys.size() == filterNulls.size();
+ final List<Integer> leftKeysRewritten = new ArrayList<>();
+ final List<Integer> rightKeysRewritten = new ArrayList<>();
+ final List<RexNode> nonEquiListRewritten = new ArrayList<>();
+ nonEquiListRewritten.addAll(nonEquiList);
+ for (int i = 0; i < filterNulls.size(); i++) {
+ if (filterNulls.get(i)) {
+ leftKeysRewritten.add(leftKeys.get(i));
+ rightKeysRewritten.add(rightKeys.get(i));
+ } else {
+ nonEquiListRewritten.add(
+ // Until IsNotDistinctFromImplementor is implemented,
Review Comment:
If this code is just a temporary patch, wouldn't it be easier to implement
the IsNotDistinctFromImplementor and not change this method?
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -66,8 +66,36 @@ public static JoinInfo of(RelNode left, RelNode right,
RexNode condition) {
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
filterNulls, nonEquiList);
- return new JoinInfo(ImmutableIntList.copyOf(leftKeys),
- ImmutableIntList.copyOf(rightKeys), ImmutableList.copyOf(nonEquiList));
+
+ /**
+ * In join implementation when the condition is equality (=),
+ * we filter out nulls prior to the join. If the condition is
Review Comment:
There is no "we".
Moreover, this code has nothing to do with filtering.
This code is about building a description of the join predicate.
I think the JoinInfo class needs better documentation; in particular, it is
not clear what the field nonEquiConditions contains. Perhaps some of this
JavaDoc should move to document the fields themselves. Then we would not need
to discuss about whether IS NOT DISTINCT FROM is an equiCondition or not.
--
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]
