You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "xiedeyantu (via GitHub)" <gi...@apache.org> on 2025/03/23 07:33:37 UTC
[PR] [CALCITE-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
xiedeyantu opened a new pull request, #4260:
URL: https://github.com/apache/calcite/pull/4260
Jira link: [CALCITE-6904](https://issues.apache.org/jira/browse/CALCITE-6904)
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2013322318
##########
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:
Maybe I need add "NestedLoopJoin" in explainContains.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011032875
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
I am not sure what "the execution logic of hashjoin" is. I am not sure where I made that statement, I could not find it in the JIRA case or this conversation.
There must be some conversion rules in the Enumerable implementation which decide when and how to implement a LogicalJoin using a HashJoin - I haven't looked at that part of the code. Maybe you are suggesting that the logic in that transformation is incorrect or inefficient.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2010896390
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
What is the problem of joins and null handling?
Joins can use arbitrary predicates.
What is an "equality condition"?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2014951506
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4741,6 +4743,47 @@ private static class IsNullImplementor extends AbstractRexCallImplementor {
}
}
+ /** Implementor for the {@code IS NOT DISTINCT FROM} SQL operator. */
+ private static class IsNotDistinctFromImplementor extends AbstractRexCallImplementor {
+ IsNotDistinctFromImplementor() {
+ super("is_not_distinct_from", NullPolicy.NONE, false);
+ }
+
+ @Override public RexToLixTranslator.Result implement(final RexToLixTranslator translator,
+ final RexCall call, final List<RexToLixTranslator.Result> arguments) {
+ final RexToLixTranslator.Result left = arguments.get(0);
Review Comment:
can you add a comment showing the generated java code?
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
is this change needed?
##########
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() {
Review Comment:
Can you also add a quidem test in one .iq file?
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -425,6 +425,7 @@
import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_JSON_SCALAR;
import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_JSON_VALUE;
import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_NOT_A_SET;
+import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_NOT_DISTINCT_FROM;
Review Comment:
what about IS_DISTINCT_FROM?
Does that need an implementor, or is it converted to IS_NOT_DISTINCT_FROM?
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4741,6 +4743,47 @@ private static class IsNullImplementor extends AbstractRexCallImplementor {
}
}
+ /** Implementor for the {@code IS NOT DISTINCT FROM} SQL operator. */
+ private static class IsNotDistinctFromImplementor extends AbstractRexCallImplementor {
+ IsNotDistinctFromImplementor() {
+ super("is_not_distinct_from", NullPolicy.NONE, false);
+ }
+
+ @Override public RexToLixTranslator.Result implement(final RexToLixTranslator translator,
+ final RexCall call, final List<RexToLixTranslator.Result> arguments) {
+ final RexToLixTranslator.Result left = arguments.get(0);
+ final RexToLixTranslator.Result right = arguments.get(1);
+
+ final Expression valueExpression =
+ Expressions.condition(left.isNullVariable,
+ Expressions.condition(right.isNullVariable, BOXED_TRUE_EXPR, BOXED_FALSE_EXPR),
+ Expressions.condition(right.isNullVariable, BOXED_FALSE_EXPR,
+ Expressions.equal(left.valueVariable, right.valueVariable)));
+
+ BlockBuilder builder = translator.getBlockBuilder();
+ final ParameterExpression valueVariable =
+ Expressions.parameter(valueExpression.getType(),
+ builder.newName(variableName + "_value"));
+ final Expression isNullExpression = FALSE_EXPR;
+ final ParameterExpression isNullVariable =
+ Expressions.parameter(Boolean.TYPE,
+ builder.newName(variableName + "_isNull"));
+
+ builder.add(
+ Expressions.declare(Modifier.FINAL, valueVariable, valueExpression));
+ builder.add(
+ Expressions.declare(Modifier.FINAL, isNullVariable, isNullExpression));
+
+ return new RexToLixTranslator.Result(isNullVariable, valueVariable);
+ }
+
+ @Override Expression implementSafe(final RexToLixTranslator translator,
+ final RexCall call, final List<Expression> argValueList) {
+ throw new IllegalStateException("This implementSafe should not be called,"
+ + " please call implement(...)");
Review Comment:
I am not sure this error message is helpful - who is the "please" for?
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4741,6 +4743,47 @@ private static class IsNullImplementor extends AbstractRexCallImplementor {
}
}
+ /** Implementor for the {@code IS NOT DISTINCT FROM} SQL operator. */
+ private static class IsNotDistinctFromImplementor extends AbstractRexCallImplementor {
+ IsNotDistinctFromImplementor() {
+ super("is_not_distinct_from", NullPolicy.NONE, false);
+ }
+
+ @Override public RexToLixTranslator.Result implement(final RexToLixTranslator translator,
+ final RexCall call, final List<RexToLixTranslator.Result> arguments) {
+ final RexToLixTranslator.Result left = arguments.get(0);
+ final RexToLixTranslator.Result right = arguments.get(1);
+
+ final Expression valueExpression =
+ Expressions.condition(left.isNullVariable,
+ Expressions.condition(right.isNullVariable, BOXED_TRUE_EXPR, BOXED_FALSE_EXPR),
+ Expressions.condition(right.isNullVariable, BOXED_FALSE_EXPR,
+ Expressions.equal(left.valueVariable, right.valueVariable)));
+
+ BlockBuilder builder = translator.getBlockBuilder();
+ final ParameterExpression valueVariable =
+ Expressions.parameter(valueExpression.getType(),
+ builder.newName(variableName + "_value"));
+ final Expression isNullExpression = FALSE_EXPR;
+ final ParameterExpression isNullVariable =
Review Comment:
is this declaration necessary or can you just use FALSE_EXPR below?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2015227048
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4741,6 +4743,47 @@ private static class IsNullImplementor extends AbstractRexCallImplementor {
}
}
+ /** Implementor for the {@code IS NOT DISTINCT FROM} SQL operator. */
+ private static class IsNotDistinctFromImplementor extends AbstractRexCallImplementor {
+ IsNotDistinctFromImplementor() {
+ super("is_not_distinct_from", NullPolicy.NONE, false);
+ }
+
+ @Override public RexToLixTranslator.Result implement(final RexToLixTranslator translator,
+ final RexCall call, final List<RexToLixTranslator.Result> arguments) {
+ final RexToLixTranslator.Result left = arguments.get(0);
+ final RexToLixTranslator.Result right = arguments.get(1);
+
+ final Expression valueExpression =
+ Expressions.condition(left.isNullVariable,
+ Expressions.condition(right.isNullVariable, BOXED_TRUE_EXPR, BOXED_FALSE_EXPR),
+ Expressions.condition(right.isNullVariable, BOXED_FALSE_EXPR,
+ Expressions.equal(left.valueVariable, right.valueVariable)));
+
+ BlockBuilder builder = translator.getBlockBuilder();
+ final ParameterExpression valueVariable =
+ Expressions.parameter(valueExpression.getType(),
+ builder.newName(variableName + "_value"));
+ final Expression isNullExpression = FALSE_EXPR;
+ final ParameterExpression isNullVariable =
+ Expressions.parameter(Boolean.TYPE,
+ builder.newName(variableName + "_isNull"));
+
+ builder.add(
+ Expressions.declare(Modifier.FINAL, valueVariable, valueExpression));
+ builder.add(
+ Expressions.declare(Modifier.FINAL, isNullVariable, isNullExpression));
+
+ return new RexToLixTranslator.Result(isNullVariable, valueVariable);
+ }
+
+ @Override Expression implementSafe(final RexToLixTranslator translator,
+ final RexCall call, final List<Expression> argValueList) {
+ throw new IllegalStateException("This implementSafe should not be called,"
+ + " please call implement(...)");
Review Comment:
Here we need to Implement `implementSafe` method, I referred to other implementations and also threw an error here.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017629037
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
If this function has no other uses, then it's easy: just define a new function to create a JoinInfo the way you like it, and call that function from EnumerableXxxJoin. I would also document the fields of JoinInfo to explain what they contain - what is considered equality.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2010984221
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
CALCITE-6904 does not say anything about standard joins, it says there is a problem with EnumerableJoinRule
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017356133
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
I am worried about this change.
This is a breaking change in a public API.
Users of this method will see different behaviors.
Where is this method called when the EnumerableJoinRule fires?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4260:
URL: https://github.com/apache/calcite/pull/4260#issuecomment-2784236055
Look like my comments didn't make it to github; maybe I forgot to press the right button.
I apologize about 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-6904] IS_NOT_DISTINCT_FROM is incorrectly handled by EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #4260:
URL: https://github.com/apache/calcite/pull/4260
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011022596
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
Maybe the question boils down at how HashJoin is supposed to be implemented.
In our implementation when the condition is equality (=) we filter out nulls prior to the join.
If the condition is `IS NOT DISTINCT FROM` we don't filter the nulls.
You can use a HashJoin in both cases.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011043589
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
I wonder if it would be better to treat IS NOT DISTINCT FROM as a non-equi condition and use NestedLoopJoin to execute it? I think it should be possible to implement it by modifying RelOptUtil.splitJoinCondition without modifying JoinInfo.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011037345
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
I am tempted to say "no" - JoinInfo is in the rel/core package, and has nothing to do with the enumerable implementation. I think any changes belong to that package.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011044623
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
There are lots of tests using joins and Enumerable which work.
What does not work?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2013323920
##########
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:
You are right, this description should be removed.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4260:
URL: https://github.com/apache/calcite/pull/4260#issuecomment-2780599978
> This is almost good to merge, I only made two suggestions for improving the documentation.
Hi, I want to confirm with you again, is there anything else I should modify in the document you mentioned?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4260:
URL: https://github.com/apache/calcite/pull/4260#issuecomment-2784814944
> Look like my comments didn't make it to github; maybe I forgot to press the right button. I apologize about that.
Thanks, I have addressed.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2015619037
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4741,6 +4743,47 @@ private static class IsNullImplementor extends AbstractRexCallImplementor {
}
}
+ /** Implementor for the {@code IS NOT DISTINCT FROM} SQL operator. */
+ private static class IsNotDistinctFromImplementor extends AbstractRexCallImplementor {
+ IsNotDistinctFromImplementor() {
+ super("is_not_distinct_from", NullPolicy.NONE, false);
+ }
+
+ @Override public RexToLixTranslator.Result implement(final RexToLixTranslator translator,
+ final RexCall call, final List<RexToLixTranslator.Result> arguments) {
+ final RexToLixTranslator.Result left = arguments.get(0);
+ final RexToLixTranslator.Result right = arguments.get(1);
+
+ final Expression valueExpression =
+ Expressions.condition(left.isNullVariable,
+ Expressions.condition(right.isNullVariable, BOXED_TRUE_EXPR, BOXED_FALSE_EXPR),
+ Expressions.condition(right.isNullVariable, BOXED_FALSE_EXPR,
+ Expressions.equal(left.valueVariable, right.valueVariable)));
+
+ BlockBuilder builder = translator.getBlockBuilder();
+ final ParameterExpression valueVariable =
+ Expressions.parameter(valueExpression.getType(),
+ builder.newName(variableName + "_value"));
+ final Expression isNullExpression = FALSE_EXPR;
+ final ParameterExpression isNullVariable =
Review Comment:
Fixed
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017709324
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
Got it, thanks. I will change it soon.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2015292104
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -425,6 +425,7 @@
import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_JSON_SCALAR;
import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_JSON_VALUE;
import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_NOT_A_SET;
+import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_NOT_DISTINCT_FROM;
Review Comment:
IS_DISTINCT_FROM will expand to `x=y IS NOT TRUE and ((x is not null) or (y is not null))`. There is currently no method to collapse it, so it may not be necessary to implement it temporarily. If necessary, I can add one, but it may not be called by any method.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4260:
URL: https://github.com/apache/calcite/pull/4260#issuecomment-2754753937
@mihaibudiu May I trouble you to help me check if my modifications match your comments.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011036194
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
> Maybe the question boils down at how HashJoin is supposed to be implemented. In our implementation when the condition is equality (=) we filter out nulls prior to the join. If the condition is `IS NOT DISTINCT FROM` we don't filter the nulls. You can use a HashJoin in both cases.
I mean, do we need to modify the logic here? If the condition is IS NOT DISTINCT FROM we don't filter the nulls.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011036194
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
> Maybe the question boils down at how HashJoin is supposed to be implemented. In our implementation when the condition is equality (=) we filter out nulls prior to the join. If the condition is `IS NOT DISTINCT FROM` we don't filter the nulls. You can use a HashJoin in both cases.
I mean, do we need to modify the logic here?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2010990409
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
In EnumerableJoinRule, JoinInfo is used to split the Equi and non-equi conditions, and then the two are combined as the join condition. And then will use HashJoin. If it does not do such a split, it is fine to use NestedLoopJoin directly with the original join condition.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2010830664
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
If IS_NOT_DISTINCT_FROM is considered an equality condition, then the implementation of join needs to be aware of null handling, otherwise IS_NOT_DISTINCT_FROM cannot be considered an equality condition, so the incorrect practice of joininfo needs to be corrected.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011028815
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
So you mean that it is reasonable for us to modify the execution logic of hashjoin? You said before that we should not change this.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017485793
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
You cannot see the code that is not in this repository
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017502033
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
I am actually a little confused. SplitJoinCondition can return different results with different parameters. So what should I do now to deal with it in a good way?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2013326290
##########
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:
Previously mentioned in [CALCITE-6902](https://issues.apache.org/jira/browse/CALCITE-6902) that IsNotDistinctFromImplementor is temporarily not implemented, so I think it's time to implement it. I will try to implement an IsNotDistinctFromImplementor.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2010773279
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
changing the signature of protected methods is a breaking change, which will affect projects that use calcite.
I don't understand why JoinInfo needs to be changed if the problem is in the EnumerableJoinRule.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2015196286
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
Yes, if filterNulls is not null, splitJoinCondition method will take IS NOT DISTINCT FROM as "equi condition". If filterNulls is null, it can skip IS NOT DISTINCT FROM correctly.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017700689
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
As I said, there may be a tool built on Calcite which calls this method.
So changing its behavior is a breaking change.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017483736
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
In my opinion, the purpose is to allow IS NOT DISTINCT FROM expressions to be handled correctly. The splitJoinCondition method is included in the JoinInfo.of method. JoinInfo.of is only used in the implementation class of EnumerableXxxJoin, that is, when handling equality and non-equality conditions, so it may not have any impact.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2021570408
##########
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]
Review Comment:
Can you change the JIRA title, the commit title and the PR title to: "IS_NOT_DISTINCT_FROM is incorrectly handled by EnumerableJoinRule"?
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -70,6 +70,22 @@ public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
ImmutableIntList.copyOf(rightKeys), ImmutableList.copyOf(nonEquiList));
}
+ /** Creates a {@code JoinInfo} by analyzing a condition.
+ * This method will only convert EQUALS condition considers it as an equi condition.
Review Comment:
Let's rephrase this slightly: "A JoinInfo created using this method only considers EQUALS operations equi-conditions; in contrast IS_NOT_DISTINCT_FROM operations are NOT considered equalities; they are inserted in the nonEquiList.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017691169
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
I understand what you mean, but now there is a problem. If I remember correctly, there are three places that call JoinInfo.of, involving HashJoin, NestedLoopJoin, MergeJoin. These three places need to be replaced. If I create a new function (such as JoinInfo.create), then JoinInfo.of will not have other methods calling it. Do I still need to create a new method?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2015214106
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4741,6 +4743,47 @@ private static class IsNullImplementor extends AbstractRexCallImplementor {
}
}
+ /** Implementor for the {@code IS NOT DISTINCT FROM} SQL operator. */
+ private static class IsNotDistinctFromImplementor extends AbstractRexCallImplementor {
+ IsNotDistinctFromImplementor() {
+ super("is_not_distinct_from", NullPolicy.NONE, false);
+ }
+
+ @Override public RexToLixTranslator.Result implement(final RexToLixTranslator translator,
+ final RexCall call, final List<RexToLixTranslator.Result> arguments) {
+ final RexToLixTranslator.Result left = arguments.get(0);
Review Comment:
I will add comment later.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017704190
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
Oh, I understand now. Can I keep it unchanged and create a new JoinInfo.create to replace the current JoinInfo.of? Also, do I need to modify other places besides here?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017429757
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
wouldn't that be also an incompatible change?
this may be backwards compatible only if new fields are added and the content of the old fields would not be changed
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011019863
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
Maybe modify RelOptUtil.splitJoinCondition is a right way?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2015292104
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -425,6 +425,7 @@
import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_JSON_SCALAR;
import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_JSON_VALUE;
import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_NOT_A_SET;
+import static org.apache.calcite.sql.fun.SqlStdOperatorTable.IS_NOT_DISTINCT_FROM;
Review Comment:
IS_DISTINCT_FROM will expand to `x=y IS NOT TRUE and ((x is not null) or (y is not null))`. There is currently no method to collapse it, so it may not be necessary to implement it temporarily?
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4260:
URL: https://github.com/apache/calcite/pull/4260#issuecomment-2760141346
@mihaibudiu I have finished, please have a look, 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
Re: [PR] [CALCITE-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2010990409
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
In EnumerableJoinRule, JoinInfo is used to split the Equi and non-equi conditions, and then the two are combined as the join condition. And then will use HashJoin. If it does not do such a split, it is fine to use NestedLoopJoin directly with the original join condition.This question does involve a lot of points. I don't know if I have made it clear this time.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2010982505
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
Maybe you can the details in jira 6904. leftKeys and rightKeys in JoinInfo contain equi condition.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2011048208
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -50,30 +52,59 @@ public class JoinInfo {
/** Creates a JoinInfo. */
protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
- ImmutableList<RexNode> nonEquiConditions) {
- this.leftKeys = requireNonNull(leftKeys, "leftKeys");
- this.rightKeys = requireNonNull(rightKeys, "rightKeys");
- this.nonEquiConditions =
- requireNonNull(nonEquiConditions, "nonEquiConditions");
+ ImmutableList<RexNode> nonEquiConditions, ImmutableList<Boolean> filterNulls,
Review Comment:
Maybe here https://github.com/apache/calcite/blob/c72e267f9c1b94abb3f4a7e31d14f7b4ed676ce1/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java#L1697
--
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-6904] IS_NOT_DISTINCT_FROM is incorrectly handled by EnumerableJoinRule [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4260:
URL: https://github.com/apache/calcite/pull/4260#issuecomment-2784828363
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4260) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4260&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4260&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4260&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [97.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4260&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4260&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4260)
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2015227048
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4741,6 +4743,47 @@ private static class IsNullImplementor extends AbstractRexCallImplementor {
}
}
+ /** Implementor for the {@code IS NOT DISTINCT FROM} SQL operator. */
+ private static class IsNotDistinctFromImplementor extends AbstractRexCallImplementor {
+ IsNotDistinctFromImplementor() {
+ super("is_not_distinct_from", NullPolicy.NONE, false);
+ }
+
+ @Override public RexToLixTranslator.Result implement(final RexToLixTranslator translator,
+ final RexCall call, final List<RexToLixTranslator.Result> arguments) {
+ final RexToLixTranslator.Result left = arguments.get(0);
+ final RexToLixTranslator.Result right = arguments.get(1);
+
+ final Expression valueExpression =
+ Expressions.condition(left.isNullVariable,
+ Expressions.condition(right.isNullVariable, BOXED_TRUE_EXPR, BOXED_FALSE_EXPR),
+ Expressions.condition(right.isNullVariable, BOXED_FALSE_EXPR,
+ Expressions.equal(left.valueVariable, right.valueVariable)));
+
+ BlockBuilder builder = translator.getBlockBuilder();
+ final ParameterExpression valueVariable =
+ Expressions.parameter(valueExpression.getType(),
+ builder.newName(variableName + "_value"));
+ final Expression isNullExpression = FALSE_EXPR;
+ final ParameterExpression isNullVariable =
+ Expressions.parameter(Boolean.TYPE,
+ builder.newName(variableName + "_isNull"));
+
+ builder.add(
+ Expressions.declare(Modifier.FINAL, valueVariable, valueExpression));
+ builder.add(
+ Expressions.declare(Modifier.FINAL, isNullVariable, isNullExpression));
+
+ return new RexToLixTranslator.Result(isNullVariable, valueVariable);
+ }
+
+ @Override Expression implementSafe(final RexToLixTranslator translator,
+ final RexCall call, final List<Expression> argValueList) {
+ throw new IllegalStateException("This implementSafe should not be called,"
+ + " please call implement(...)");
Review Comment:
Here we need to Implement `implementSafe` method that should not be used, I referred to other implementations and also threw an error here.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2015618879
##########
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() {
Review Comment:
Done
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017425388
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
I haven't reviewed the code in detail, but my guess is the issue arises from the equivalent conditional determination process, where non-equi conditions are fetched from the joinInfo(generated during the join construction) and treated as equivCond if they aren't in the list.
If that's the case, IMO a better approach would be to record the list of nullFilter in the JoinInfo. And use nullFilter to fix where you fundamentally want to fix the problem🤔️.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017425388
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
I haven't reviewed the code in detail, but my guess is the issue arises from the equivalent conditional determination process, where non-equi conditions are fetched from the joinInfo(ggenerated during the join construction) and treated as equivCond if they aren't in the list.
If that's the case, IMO a better approach would be to record the list of nullFilter in the JoinInfo. And use nullFilter to fix where you fundamentally want to fix the problem🤔️.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4260:
URL: https://github.com/apache/calcite/pull/4260#discussion_r2017706520
##########
core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java:
##########
@@ -62,10 +62,9 @@ protected JoinInfo(ImmutableIntList leftKeys, ImmutableIntList rightKeys,
public static JoinInfo of(RelNode left, RelNode right, RexNode condition) {
final List<Integer> leftKeys = new ArrayList<>();
final List<Integer> rightKeys = new ArrayList<>();
- final List<Boolean> filterNulls = new ArrayList<>();
final List<RexNode> nonEquiList = new ArrayList<>();
RelOptUtil.splitJoinCondition(left, right, condition, leftKeys, rightKeys,
- filterNulls, nonEquiList);
+ null, nonEquiList);
Review Comment:
Yes, but I would name the function `createWithStrictEquality`, or something like this, and explain how it differs from `of`.
I think that if all the tests pass you are fine - if you don't change public interfaces.
After your change `JoinInfo` will represent two slightly different data structures, depending on how it's created, so it is very important to document clearly how the result created by `of` differs from the result created by your function.
--
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-6904] IS_NOT_DISTINCT_FROM is converted error in EnumerableJoinRule [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4260:
URL: https://github.com/apache/calcite/pull/4260#issuecomment-2767057110
This is almost good to merge, I only made two suggestions for improving the documentation.
--
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