[CALCITE-3089] Deprecate EquiJoin #1247

hsyuan · 2019-05-31T17:40:42Z

JIRA: https://issues.apache.org/jira/browse/CALCITE-3089

This patch is intended for v1.21.0

hsyuan · 2019-05-31T17:46:58Z

core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java

@@ -49,7 +50,8 @@
 public abstract class FilterJoinRule extends RelOptRule {
  /** Predicate that always returns true. With this predicate, every filter
   * will be pushed into the ON clause. */
-  public static final Predicate TRUE_PREDICATE = (join, joinType, exp) -> true;
+  public static final Predicate TRUE_PREDICATE = (join, joinType, exp) ->
+      join.getConvention() != EnumerableConvention.INSTANCE;


This will be changed back once CALCITE-2973 is done.

Maybe add a TODO comment in the code to not forget about it?

I don't think we should add in a Calcite convention to a logical rule,

We thought it is logical rule. Unfortunately, this rule also applies to physical operators. And the default Enumerable operators don't support hash join with non-equi conditions, that is why we have to rule it out.

Are we likely to see a fix to CALCITE-2973 reasonably soon? I agree that the code should not depend on EnumerableConvention, but we can live with it short-term.

I checked with Lai Zhou, who said the fix to CALCITE-2973 can be done in a week.

By the way, even if this is a short-term change, let's rename TRUE_PREDICATE.

It's bad form to redefine TRUE (or FALSE, 0, 1, Pi or most especially Planck's constant).

I saw you renamed TRUE_PREDICATE to TRUE. That doesn't solve the problem. It was called TRUE_PREDICATE because it always returned true. Now it doesn't, so its name is wrong.

Maybe leave TRUE_PREDICATE as is (i.e. returning true), deprecate it, and use an inline lambda.

danny0405 · 2019-06-01T01:49:58Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableJoinRule.java

+          join.getVariablesSet(),
+          join.getJoinType());
+      RexNode nonEqui = info.getRemaining(cluster.getRexBuilder());
+      if (!nonEqui.isAlwaysTrue()) {


This if check is redundant.

I don't think so.

This check is necessary. But I prefer use !info.isEqui() since it is more clear?.

danny0405 · 2019-06-01T01:51:35Z

core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java

    this.leftKeys = Objects.requireNonNull(leftKeys);
    this.rightKeys = Objects.requireNonNull(rightKeys);
+    this.nonEqui = nonEqui;
    assert leftKeys.size() == rightKeys.size();


nonEquiConditions or remaining ?

nonEquiConditions might be better.

danny0405 · 2019-06-01T01:53:12Z

core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java

+  public boolean isEqui() {
+    return (nonEqui == null || nonEqui.isAlwaysTrue())
+        && leftKeys.size() == rightKeys.size();
+  }


leftKeys.size() should always equals with rightKeys.size()

Correct, this is sanity check.

danny0405 · 2019-06-01T01:55:09Z

core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java

-          // no longer an equi-join. SemiJoin is an example of this.
-          return;
-        }
-      }


We can not remove this, the behavior would be non-consistent, how about other systems use this rule and their join does not support non-equi conditions ?

Then the other systems should update their own implementation, as EquiJoin is deprecated.

chunweilei · 2019-07-02T09:19:55Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableJoinRule.java

+          join.getVariablesSet(),
+          join.getJoinType());
+      RexNode nonEqui = info.getRemaining(cluster.getRexBuilder());
+      if (!nonEqui.isAlwaysTrue()) {


This check is necessary. But I prefer use !info.isEqui() since it is more clear?.

chunweilei · 2019-07-02T09:22:06Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableJoinRule.java

@@ -58,35 +58,26 @@
    final RelNode right = newInputs.get(1);
    final JoinInfo info = JoinInfo.of(left, right, join.getCondition());
    if (!info.isEqui() && join.getJoinType() != JoinRelType.INNER) {


use join.analyzeCondition()?

chunweilei · 2019-08-06T01:20:07Z

LGTM

chunweilei

+1

rubenada · 2019-08-06T07:57:22Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableHashJoin.java

@@ -56,8 +55,7 @@ protected EnumerableHashJoin(
      RelNode right,
      RexNode condition,
      Set<CorrelationId> variablesSet,
-      JoinRelType joinType)
-      throws InvalidRelException {
+      JoinRelType joinType) {


Would it make sense to add in this constructor a check on the condition (to verify that it is actually an equi-join, which is required by the underlying implementation algorithm)?

if (!JoinInfo.of(left, right, condition).isEqui()) throw new IllegalArgumentException("...");

or maybe at least an assert
assert JoinInfo.of(left, right, condition).isEqui();
Idem for EnumerableMergeJoin.

Yes, make sense. Will do

danny0405

The change for the rules are kind of breaking, i'm -1 on this.

hsyuan · 2019-08-07T07:03:03Z

I don't think it is a breaking change, comparing with other changes like removing SemiJoin.

danny0405 · 2019-08-07T08:26:13Z

I don't think it is a breaking change, comparing with other changes like removing SemiJoin.

I don't think so, removing SemiJoin have a fallback, user can use the Join instead, but with the rules change in this patch, there is no way to fix the regression if they use these rules from Calcite, users only have 2 way to fix this

copy the whole rule out and add a logic what you removed
change their physical operators to allow non-equal join condition

Either is not an easy way to go.

hsyuan · 2019-08-07T08:30:47Z

The other system's physical implementation should not limit what Calcite can do or can't do. Otherwise, there is no way to move forward.

danny0405 · 2019-08-07T08:33:34Z

The other system's physical implementation should not limit what Calcite can do or can't do. Otherwise, there is no way to move forward.

I agree with the point that other system's physical implementation should not limit what Calcite can do , but we at least let user choose whether they what to push non-equal join conditions into the join operator, just like Calcite itself, apparently, not every join operator support non-equal join conditions.

hsyuan · 2019-08-07T08:45:07Z

The user can limit the rule to only apply on logical operators, instead of both logical operators and physical operators. And it makes more sense to apply on logical operators only, in which case, the problem you stated is not a concern anymore.

rubenada · 2019-08-07T09:20:07Z

@danny0405 @hsyuan What if we added a new method to Join, something like: public boolean supportsNonEquiJoin() with a default implementation that just return true, and then each physical Join implementation that supports only EquiJoin conditions (like EnumerableHashJoin / EnumerableMergeJoin) will have to override it to return false. This method could be use as verification check by the rules, replacing somehow the current RelOptUtil#forceEquiJoin (which is removed by the current patch).

danny0405 · 2019-08-07T10:27:09Z

@danny0405 @hsyuan What if we added a new method to Join, something like: public boolean supportsNonEquiJoin() with a default implementation that just return true, and then each physical Join implementation that supports only EquiJoin conditions (like EnumerableHashJoin / EnumerableMergeJoin) will have to override it to return false. This method could be use as verification check by the rules, replacing somehow the current RelOptUtil#forceEquiJoin (which is removed by the current patch).

Thanks, @rubenada , adding method to Join does not seem a good idea, what we only need is just let user control the behavior of the 2 rules.

rubenada · 2019-08-07T10:31:10Z

Ok @danny0405 , I understand

eolivelli · 2019-08-07T18:15:51Z

@amitvc Please take a look, I guess you are interested in this change

julianhyde

Is this a breaking change?That is, is there code that compiles and runs against 1.20 that will fail to compile and run against 1.21? If so, let's give this a special mention in the release notes.

This is a useful refactoring; thanks for pushing it through.

julianhyde · 2019-08-09T21:00:31Z

core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java

@@ -49,7 +50,8 @@
 public abstract class FilterJoinRule extends RelOptRule {
  /** Predicate that always returns true. With this predicate, every filter
   * will be pushed into the ON clause. */
-  public static final Predicate TRUE_PREDICATE = (join, joinType, exp) -> true;
+  public static final Predicate TRUE_PREDICATE = (join, joinType, exp) ->
+      join.getConvention() != EnumerableConvention.INSTANCE;


Are we likely to see a fix to CALCITE-2973 reasonably soon? I agree that the code should not depend on EnumerableConvention, but we can live with it short-term.

julianhyde · 2019-08-09T21:05:36Z

core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java

  public final ImmutableIntList leftKeys;
  public final ImmutableIntList rightKeys;
+  protected final RexNode nonEquiCondition;


My instinct is to make everything non-null. I don't like the test nonEquiCondition == null || nonEquiCondition.isAlwaysTrue(), for instance.

Consider making nonEquiCondition a conjunctive list (immutable, final and not null); if the list is empty, that means true. We do this in several other places, and it is useful, because caller can assume that ANDs have been flattened.

getRemaining is a public method and returns RexNode. If we make nonEquiCondition a conjunctive list, we have to convert to a RexNode every time we call getRemainng. How about checking the nullness of input nonEquiCondition in the JoinInfo constructor, set it to literal TRUE rexnode if it is null? At the same time, add @Nonnull to getRemaining.

Revoke what I just said, the is no way to pass RexBuilder in the constructor.

I don't think you should allow availability of RexBuilder to sway the design. When a JoinInfo is created, there is always a RexBuilder available, and that RexBuilder (and its RelDataTypeFactory) remain valid for the lifetime of the JoinInfo.

Representing conditions as conjunctive lists seems to be very convenient. For example, consider the RelOptUtil.splitJoinCondition method, which is the source of JoinInfo.remaining. The last thing it does is to convert a conjunctive list into a RexNode. Other callers of that method either ignore the return, or immediately convert the RexNode back into a conjunctive list.

Make sense. I will change nonEquiCondition to conjunctive list. But looks like we have to keep public RexNode getRemaining(RexBuilder rexBuilder) unchanged, since it is a public API.

hsyuan · 2019-08-13T04:42:54Z

@julianhyde I addressed your comments. Thanks!

danny0405 · 2019-08-13T12:13:28Z

core/src/main/java/org/apache/calcite/rel/core/JoinInfo.java

-  public abstract RexNode getRemaining(RexBuilder rexBuilder);
+  public RexNode getRemaining(RexBuilder rexBuilder) {
+    return RexUtil.composeConjunction(rexBuilder, nonEquiConditions);
+  }


It's so weird that we pass in nonEquiConditions into the constructor and return a RexNode here, can we have a better solution ?

I agree, but given this is a public API, we can't change it to returning list instead of a RexNode. What we can do is deprecating this method, and introduce another one returning conjunction list.

I'm +1 on deprecate this method and introduce another one returning conjunction list.

Close apache#1247

hsyuan commented May 31, 2019

View reviewed changes

danny0405 reviewed Jun 1, 2019

View reviewed changes

chunweilei reviewed Jul 2, 2019

View reviewed changes

chunweilei approved these changes Aug 6, 2019

View reviewed changes

chunweilei added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Aug 6, 2019

rubenada reviewed Aug 6, 2019

View reviewed changes

danny0405 requested changes Aug 7, 2019

View reviewed changes

julianhyde reviewed Aug 9, 2019

View reviewed changes

hsyuan added 2 commits August 12, 2019 16:02

[CALCITE-3089] Deprecate EquiJoin

83aa543

Address comments

b26ae02

danny0405 reviewed Aug 13, 2019

View reviewed changes

hsyuan added 2 commits August 13, 2019 11:37

Address comments

34e4a73

Reinstate TRUE predicate

046708b

julianhyde pushed a commit to julianhyde/calcite that referenced this pull request Aug 15, 2019

[CALCITE-3089] Deprecate EquiJoin

5c89c2a

Close apache#1247

julianhyde pushed a commit to julianhyde/calcite that referenced this pull request Aug 16, 2019

[CALCITE-3089] Deprecate EquiJoin

0988d99

Close apache#1247

julianhyde pushed a commit to julianhyde/calcite that referenced this pull request Aug 16, 2019

[CALCITE-3089] Deprecate EquiJoin

7a68894

Close apache#1247

julianhyde pushed a commit to julianhyde/calcite that referenced this pull request Aug 20, 2019

[CALCITE-3089] Deprecate EquiJoin

5818ea0

Close apache#1247

asfgit closed this in 36e3109 Aug 20, 2019

wangxlong pushed a commit to wangxlong/calcite that referenced this pull request Feb 13, 2020

[CALCITE-3089] Deprecate EquiJoin

11f612f

Close apache#1247

[CALCITE-3089] Deprecate EquiJoin #1247

[CALCITE-3089] Deprecate EquiJoin #1247

Conversation

hsyuan commented May 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunweilei commented Aug 6, 2019

chunweilei left a comment

Choose a reason for hiding this comment

rubenada Aug 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 left a comment

Choose a reason for hiding this comment

hsyuan commented Aug 7, 2019

danny0405 commented Aug 7, 2019

hsyuan commented Aug 7, 2019

danny0405 commented Aug 7, 2019

hsyuan commented Aug 7, 2019

rubenada commented Aug 7, 2019

danny0405 commented Aug 7, 2019

rubenada commented Aug 7, 2019

eolivelli commented Aug 7, 2019

julianhyde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsyuan commented Aug 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsyuan commented May 31, 2019 •

edited

rubenada Aug 6, 2019 •

edited