[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r288848902
##
File path: core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java
##
@@ -116,6 +115,15 @@ protected FilterJoinRule(RelOptRuleOperand operand,
String id,
//~ Methods
+ /** Returns if it is needed to push the filter condition above join
+ * into the join condition.
+ */
+ private boolean needsPushInto(Join join) {
+// If the join force the join info to be based on column equality,
+// Or it is a non-correlated semijoin, returns false.
Review comment:
@hsyuan Oops, i missed your comment, feel free to update it as what you want
it to be.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r288848902
##
File path: core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java
##
@@ -116,6 +115,15 @@ protected FilterJoinRule(RelOptRuleOperand operand,
String id,
//~ Methods
+ /** Returns if it is needed to push the filter condition above join
+ * into the join condition.
+ */
+ private boolean needsPushInto(Join join) {
+// If the join force the join info to be based on column equality,
+// Or it is a non-correlated semijoin, returns false.
Review comment:
@hsyuan Oops, i missed your comment, feel free to update it as what you want
is to be.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r288057780
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3739,12 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality,
+ * this mostly depends on the physical implementation of the join. */
+ public static boolean forceEquiJoin(Join join) {
+return join.isSemiJoin() || join instanceof EquiJoin;
Review comment:
Thanks a lot @rubenada and @hsyuan , I saw that @hsyuan had fired an issue
to support non-equi semi join(also hash join and merge join), lets's solve the
ticket in 1.21.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions URL: https://github.com/apache/calcite/pull/1157#discussion_r285341786 ## File path: site/_docs/history.md ## @@ -39,6 +39,17 @@ Guava versions 19.0 to 27.1-jre; Apache Druid version 0.14.0-incubating; other software versions as specified in `pom.xml`. + Breaking Changes + +* Make `EnumerableMergeJoin` extend `Join` instead of `EquiJoin` +* `Correlate` use `JoinRelType` instead of `SemiJoinType` +* Rename `EnumerableThetaJoin` to `EnumerableNestedLoopJoin` +* Rename `EnumerableJoin` to `EnumerableHashJoin` +* Remove `SemiJoinFactory` from `RelBuilder`, method `semiJoin` now returns a `LogicalJoin` +with join type `JoinRelType.SEMI` instead of a `SemiJoin` +* Rules: `SemiJoinFilterTransposeRule`, `SemiJoinJoinTransposeRule`, `SemiJoinProjectTransposeRule` +and `SemiJoinRemoveRule` matches `LogicalJoin` with join type `SEMI` instead of `SemiJoin`. Review comment: Yep, 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285341533
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -94,6 +150,37 @@ public JoinRelType cancelNullsOnRight() {
return this;
}
}
+
+ /** Transform this JoinRelType to CorrelateJoinType. **/
+ public CorrelateJoinType toLinq4j() {
+switch (this) {
+case INNER:
+ return CorrelateJoinType.INNER;
+case LEFT:
+ return CorrelateJoinType.LEFT;
+case SEMI:
+ return CorrelateJoinType.SEMI;
+case ANTI:
+ return CorrelateJoinType.ANTI;
+}
+throw new IllegalStateException(
+"Unable to convert " + this + " to CorrelateJoinType");
+ }
+
+ public boolean projectsRight() {
+switch (this) {
+case INNER:
+case LEFT:
+case RIGHT:
+case FULL:
+ return true;
+case SEMI:
+case ANTI:
+ return false;
+}
+throw new IllegalStateException(
Review comment:
Thanks @zabetak, let's just keep is as now.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285324792
##
File path: core/src/main/java/org/apache/calcite/rel/core/EquiJoin.java
##
@@ -27,12 +27,33 @@
/**
* Base class for any join whose condition is based on column equality.
+ *
+ * For most of the cases, {@link JoinInfo#isEqui()} can already decide
+ * if the join condition is based on column equality.
+ *
+ * {@code EquiJoin} is an abstract class for inheritance of Calcite
enumerable
+ * joins and join implementation of other system. You should inherit the
{@code EquiJoin}
+ * if your join implementation does not support non-equi join conditions.
Calcite would
+ * eliminate some optimize logic for {@code EquiJoin} in some planning rules.
+ * e.g. {@link org.apache.calcite.rel.rules.FilterJoinRule} would not push
non-equi
+ * join conditions of the above filter into the join underneath if it is an
{@code EquiJoin}.
*/
public abstract class EquiJoin extends Join {
Review comment:
As i have notes in the comments, we need a class like this to let user tell
Calcite that their join should always keep condition as equi. Maybe we can have
a better way to do this, appreciate for your suggestions.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285322574
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -94,6 +150,37 @@ public JoinRelType cancelNullsOnRight() {
return this;
}
}
+
+ /** Transform this JoinRelType to CorrelateJoinType. **/
+ public CorrelateJoinType toLinq4j() {
+switch (this) {
+case INNER:
+ return CorrelateJoinType.INNER;
+case LEFT:
+ return CorrelateJoinType.LEFT;
+case SEMI:
+ return CorrelateJoinType.SEMI;
+case ANTI:
+ return CorrelateJoinType.ANTI;
+}
+throw new IllegalStateException(
+"Unable to convert " + this + " to CorrelateJoinType");
+ }
+
+ public boolean projectsRight() {
+switch (this) {
+case INNER:
+case LEFT:
+case RIGHT:
+case FULL:
+ return true;
+case SEMI:
+case ANTI:
+ return false;
+}
+throw new IllegalStateException(
Review comment:
@zabetak Oops, i mistaken it, but if i remove the `throw` java compiler will
complains error. We have 2 choice:
1. throw exception (cause we may extend the join type in future)
2. return a default value (i don't think we actually can have the default
here)
So, 1 seems more acceptable. What do you think ?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285324792
##
File path: core/src/main/java/org/apache/calcite/rel/core/EquiJoin.java
##
@@ -27,12 +27,33 @@
/**
* Base class for any join whose condition is based on column equality.
+ *
+ * For most of the cases, {@link JoinInfo#isEqui()} can already decide
+ * if the join condition is based on column equality.
+ *
+ * {@code EquiJoin} is an abstract class for inheritance of Calcite
enumerable
+ * joins and join implementation of other system. You should inherit the
{@code EquiJoin}
+ * if your join implementation does not support non-equi join conditions.
Calcite would
+ * eliminate some optimize logic for {@code EquiJoin} in some planning rules.
+ * e.g. {@link org.apache.calcite.rel.rules.FilterJoinRule} would not push
non-equi
+ * join conditions of the above filter into the join underneath if it is an
{@code EquiJoin}.
*/
public abstract class EquiJoin extends Join {
Review comment:
As i have notes in the comments, we need a class like this to let user tell
Calcite that their join should always keep condition as equi.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285324721
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3739,12 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality,
+ * this mostly depends on the physical implementation of the join. */
+ public static boolean forceEquiJoin(Join join) {
+return join.isSemiJoin() || join instanceof EquiJoin;
Review comment:
No, we can not do this, because `join.analyzeCondition().isEqui()` <>
`forceEquiJoin`, this method tell us if the join's condition must always keep
as equi but not returns whether it is current a equi-join.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285324655
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdPredicates.java
##
@@ -619,6 +620,7 @@ public RelOptPredicateList inferPredicates(
final RexBuilder rexBuilder = joinRel.getCluster().getRexBuilder();
switch (joinType) {
+ case SEMI:
Review comment:
I agree, separate SEMI and INNER logic, also remove the isSemiJoin
flag(useless now).
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285324373
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java
##
@@ -258,11 +262,17 @@ protected RelMdExpressionLineage() {}
}
// Right input references might need to be updated if there are
// table names clashes with left input
+final RelDataType fullRowType = SqlValidatorUtil.createJoinType(
+rexBuilder.getTypeFactory(),
+rel.getLeft().getRowType(),
+rel.getRight().getRowType(),
+null,
+ImmutableList.of());
Review comment:
Because `SEMI`(and `ANTI`) row type does not contains fields from right
input, use a full row type here will allow us to inferrer lineage for
expressions that reference the right fields.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285324183
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java
##
@@ -186,7 +189,8 @@ protected RelMdExpressionLineage() {}
// Extract input fields referenced by expression
final ImmutableBitSet inputFieldsUsed = extractInputRefs(outputExpression);
-if (rel.getJoinType() != JoinRelType.INNER) {
+if (rel.getJoinType().generatesNullsOnLeft()
+|| rel.getJoinType().generatesNullsOnRight()) {
Review comment:
We can use `isOuterJoin` to do the check, 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285324095
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdDistinctRowCount.java
##
@@ -140,35 +139,13 @@ public Double getDistinctRowCount(Filter rel,
RelMetadataQuery mq,
public Double getDistinctRowCount(Join rel, RelMetadataQuery mq,
ImmutableBitSet groupKey, RexNode predicate) {
-if (predicate == null || predicate.isAlwaysTrue()) {
- if (groupKey.isEmpty()) {
-return 1D;
- }
-}
return RelMdUtil.getJoinDistinctRowCount(mq, rel, rel.getJoinType(),
groupKey, predicate, false);
}
public Double getDistinctRowCount(SemiJoin rel, RelMetadataQuery mq,
Review comment:
Yep, 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285322638
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -43,6 +91,14 @@ public boolean generatesNullsOnLeft() {
return (this == RIGHT) || (this == FULL);
}
+ /**
+ * Returns whether a join of this type may generate NULL values, either on
the
+ * left-hand side or right-hand side.
+ */
+ public boolean generatesNulls() {
Review comment:
Yep, `isOuterJoin` seems a better name, 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285322574
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -94,6 +150,37 @@ public JoinRelType cancelNullsOnRight() {
return this;
}
}
+
+ /** Transform this JoinRelType to CorrelateJoinType. **/
+ public CorrelateJoinType toLinq4j() {
+switch (this) {
+case INNER:
+ return CorrelateJoinType.INNER;
+case LEFT:
+ return CorrelateJoinType.LEFT;
+case SEMI:
+ return CorrelateJoinType.SEMI;
+case ANTI:
+ return CorrelateJoinType.ANTI;
+}
+throw new IllegalStateException(
+"Unable to convert " + this + " to CorrelateJoinType");
+ }
+
+ public boolean projectsRight() {
+switch (this) {
+case INNER:
+case LEFT:
+case RIGHT:
+case FULL:
+ return true;
+case SEMI:
+case ANTI:
+ return false;
+}
+throw new IllegalStateException(
Review comment:
@zabetak Oops, i mistaken it, but if i remove the `throw` java compiler will
complains error. We have 2 choice:
1. throw exception (cause we may extend the join type in future)
2. return a default value (i don't think we actually can have the default
here)
So, 1 is more acceptable. What do you think ?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285322216
##
File path:
core/src/main/java/org/apache/calcite/rel/rules/SemiJoinProjectTransposeRule.java
##
@@ -63,42 +63,35 @@
*/
private SemiJoinProjectTransposeRule(RelBuilderFactory relBuilderFactory) {
super(
-operand(SemiJoin.class,
+operandJ(LogicalJoin.class, null, Join::isSemiJoin,
Review comment:
Thanks, @zabetak, i would add some notes in the breaking changes for the
rule that matches `SemiJoin` class specifically.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285322095
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableJoinRule.java
##
@@ -71,26 +71,25 @@
EnumerableRules.LOGGER.debug(e.toString());
return null;
}
+} else {
+ RelNode newRel;
+ try {
+newRel = EnumerableHashJoin.create(
+left,
+right,
+info.getEquiCondition(left, right, cluster.getRexBuilder()),
+join.getVariablesSet(),
+join.getJoinType());
+ } catch (InvalidRelException e) {
+EnumerableRules.LOGGER.debug(e.toString());
+return null;
+ }
+ if (!info.isEqui()) {
+newRel = new EnumerableFilter(cluster, newRel.getTraitSet(),
+newRel, info.getRemaining(cluster.getRexBuilder()));
+ }
Review comment:
@rubenada Let's add in the check when we really add in `ANTI` join
expression.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285152631
##
File path:
core/src/main/java/org/apache/calcite/rel/rules/SemiJoinProjectTransposeRule.java
##
@@ -63,42 +63,35 @@
*/
private SemiJoinProjectTransposeRule(RelBuilderFactory relBuilderFactory) {
super(
-operand(SemiJoin.class,
+operandJ(LogicalJoin.class, null, Join::isSemiJoin,
Review comment:
You are right, but usually (actually it is the only way) people will build a
`SemiJoin` from `RelBuilder#semiJoin` which will produce a `LogicalJoin` with
`SEMI` as join type now, so it expect to be still matched.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285157917
##
File path: core/src/main/java/org/apache/calcite/rel/core/RelFactories.java
##
@@ -376,40 +371,12 @@ RelNode createCorrelate(RelNode left, RelNode right,
private static class CorrelateFactoryImpl implements CorrelateFactory {
public RelNode createCorrelate(RelNode left, RelNode right,
CorrelationId correlationId, ImmutableBitSet requiredColumns,
-SemiJoinType joinType) {
+JoinRelType joinType) {
return LogicalCorrelate.create(left, right, correlationId,
requiredColumns, joinType);
}
}
- /**
- * Can create a semi-join of the appropriate type for a rule's calling
- * convention.
- */
- public interface SemiJoinFactory {
Review comment:
Initially i thought they are internal APIs that only used for `RelBuilder`,
but I agree keeping them will make better compatibility, add them back.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285155247
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -94,6 +150,37 @@ public JoinRelType cancelNullsOnRight() {
return this;
}
}
+
+ /** Transform this JoinRelType to CorrelateJoinType. **/
+ public CorrelateJoinType toLinq4j() {
+switch (this) {
+case INNER:
+ return CorrelateJoinType.INNER;
+case LEFT:
+ return CorrelateJoinType.LEFT;
+case SEMI:
+ return CorrelateJoinType.SEMI;
+case ANTI:
+ return CorrelateJoinType.ANTI;
+}
+throw new IllegalStateException(
+"Unable to convert " + this + " to CorrelateJoinType");
+ }
+
+ public boolean projectsRight() {
+switch (this) {
+case INNER:
+case LEFT:
+case RIGHT:
+case FULL:
+ return true;
+case SEMI:
+case ANTI:
+ return false;
+}
+throw new IllegalStateException(
Review comment:
I copied it from `SemiJoinType#toLinq4j`, why do you think it is weird ? I
think it is necessary cause `Correlate` could not have `RIGHT` as join type.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r285152631
##
File path:
core/src/main/java/org/apache/calcite/rel/rules/SemiJoinProjectTransposeRule.java
##
@@ -63,42 +63,35 @@
*/
private SemiJoinProjectTransposeRule(RelBuilderFactory relBuilderFactory) {
super(
-operand(SemiJoin.class,
+operandJ(LogicalJoin.class, null, Join::isSemiJoin,
Review comment:
You are right, but the rule's implementation all match `LogicalJoin` instead
of `SemiJoin`, we may need 2 implementations if we want to match `SemiJoin`
class. But how can i fix this ? I thought of a way, rename a new Rule class
which keep the old logic and make this Rule#Instance reference the old one, but
do we really need this ? how about put them in the breaking changes and notes
that this rule's match logic has 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r283692183
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdPredicates.java
##
@@ -578,6 +577,7 @@ public RelOptPredicateList inferPredicates(
final Set allExprs = new HashSet<>(this.allExprs);
final JoinRelType joinType = joinRel.getJoinType();
switch (joinType) {
+ case SEMI:
Review comment:
I'm not that sure, i add some `ANTI`s where `SEMI`s appears which i'm
confident that the modification is right, for the others, i would rather leave
it to the one who implements the `ANTI` join expression.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r283690931
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -94,6 +150,37 @@ public JoinRelType cancelNullsOnRight() {
return this;
}
}
+
+ /** Transform this JoinRelType to CorrelateJoinType. **/
+ public CorrelateJoinType toLinq4j() {
+switch (this) {
+case INNER:
+ return CorrelateJoinType.INNER;
+case LEFT:
+ return CorrelateJoinType.LEFT;
+case SEMI:
+ return CorrelateJoinType.SEMI;
+case ANTI:
+ return CorrelateJoinType.ANTI;
+}
+throw new IllegalStateException(
+"Unable to convert " + this + " to CorrelateJoinType");
+ }
+
+ public boolean projectsRight() {
Review comment:
@rubenada Julian suggests this name, you can see the details in the JIRA
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r283682678
##
File path: core/src/main/java/org/apache/calcite/rel/core/Join.java
##
@@ -230,6 +237,15 @@ public boolean isSemiJoinDone() {
return false;
}
+ /**
+ * Returns whether this Join is a semijoin.
+ *
+ * @return true if this is semi but without correlate variables.
+ */
+ public boolean isSemiJoin() {
Review comment:
@rubenada I think `Join#getJoinType().returnsJustFirstInput()` is ok for the
`ANTI` case, the reason i add a method for semiJoin is that there are so many
cases we have to make decision if it is a SemiJoin type(for rowType or for cost
computation or else), after we add in `ANTI` join, i think we just need to add
a new method `isAntiJoin`, i'm not sure if we can unify the logic for `ANTI`
join where SemiJoin appears(very probably not), if we can, i agree with you to
move the `returnsJustFirstInput()` method into `Join`.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r281179147
##
File path:
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableCorrelateTest.java
##
@@ -86,18 +88,17 @@
.query(
"select empid, name from emps e where e.deptno in (select d.deptno
from depts d)")
.withHook(Hook.PLANNER, (Consumer) planner -> {
- // force the semi-join to run via EnumerableCorrelate instead of
EnumerableJoin/SemiJoin
- planner.addRule(JoinToCorrelateRule.SEMI);
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
+ // force the semi-join to run via EnumerableCorrelate
+ // instead of EnumerableHashJoin/SemiJoin
+ planner.addRule(JoinToCorrelateRule.JOIN);
planner.removeRule(EnumerableRules.ENUMERABLE_SEMI_JOIN_RULE);
})
.explainContains(""
+ "EnumerableCalc(expr#0..2=[{inputs}], empid=[$t0], name=[$t2])\n"
-+ " EnumerableCorrelate(correlation=[$cor1], joinType=[semi],
requiredColumns=[{1}])\n"
++ " EnumerableHashJoin(condition=[=($1, $3)], joinType=[semi])\n"
Review comment:
@vlsi You are right, i have force the test cases to produce
`EnumerableCorrelate`, but because we deprecate `JoinToCorrelateRule#SEMI`(use
`JoinToCorrelateRule#INSTANCE` instead), now the `Correlate`(comes from inner
join) with an `agg` as one input is a cheaper plan.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r281179147
##
File path:
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableCorrelateTest.java
##
@@ -86,18 +88,17 @@
.query(
"select empid, name from emps e where e.deptno in (select d.deptno
from depts d)")
.withHook(Hook.PLANNER, (Consumer) planner -> {
- // force the semi-join to run via EnumerableCorrelate instead of
EnumerableJoin/SemiJoin
- planner.addRule(JoinToCorrelateRule.SEMI);
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
+ // force the semi-join to run via EnumerableCorrelate
+ // instead of EnumerableHashJoin/SemiJoin
+ planner.addRule(JoinToCorrelateRule.JOIN);
planner.removeRule(EnumerableRules.ENUMERABLE_SEMI_JOIN_RULE);
})
.explainContains(""
+ "EnumerableCalc(expr#0..2=[{inputs}], empid=[$t0], name=[$t2])\n"
-+ " EnumerableCorrelate(correlation=[$cor1], joinType=[semi],
requiredColumns=[{1}])\n"
++ " EnumerableHashJoin(condition=[=($1, $3)], joinType=[semi])\n"
Review comment:
@vlsi You are right, i have force the test cases to produce
`EnumerableCorrelate`, but because we deprecate `JoinToCorrelateRule#SEMI`, now
the `Correlate` with an `agg` as one input is a cheaper plan.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions URL: https://github.com/apache/calcite/pull/1157#discussion_r281101748 ## File path: core/src/main/java/org/apache/calcite/rel/rules/JoinToCorrelateRule.java ## @@ -77,18 +76,19 @@ */ public static final JoinToCorrelateRule JOIN = new JoinToCorrelateRule(LogicalJoin.class, RelFactories.LOGICAL_BUILDER, - "JoinToCorrelateRule", join -> SemiJoinType.of(join.getJoinType())); + "JoinToCorrelateRule", Join::getJoinType); - @Deprecated // to be removed (should use JOIN instead), kept for backwards compatibility + @Deprecated // To be removed (should use JOIN instead), kept for backwards compatibility Review comment: I agree, will deprecate JOIN instead of INSTANCE 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r281008775
##
File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMdUtil.java
##
@@ -659,15 +736,12 @@ public static Double getJoinRowCount(RelMetadataQuery
mq, Join join,
}
double product = left * right;
-// TODO: correlation factor
return product * mq.getSelectivity(join, condition);
}
/** Returns an estimate of the number of rows returned by a
* {@link SemiJoin}. */
- public static Double getSemiJoinRowCount(RelMetadataQuery mq, RelNode left,
- RelNode right, JoinRelType joinType, RexNode condition) {
-// TODO: correlation factor
+ public static Double getSemiJoinRowCount(RelMetadataQuery mq, RelNode left,
RexNode condition) {
Review comment:
I agree, will add the unused arguments back.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r281008385
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableJoinRule.java
##
@@ -71,26 +71,25 @@
EnumerableRules.LOGGER.debug(e.toString());
return null;
}
+} else {
+ RelNode newRel;
+ try {
+newRel = EnumerableHashJoin.create(
+left,
+right,
+info.getEquiCondition(left, right, cluster.getRexBuilder()),
+join.getVariablesSet(),
+join.getJoinType());
+ } catch (InvalidRelException e) {
+EnumerableRules.LOGGER.debug(e.toString());
+return null;
+ }
+ if (!info.isEqui()) {
+newRel = new EnumerableFilter(cluster, newRel.getTraitSet(),
+newRel, info.getRemaining(cluster.getRexBuilder()));
+ }
Review comment:
@rubenada Thx for the analysis, i also have this concern, but Calcite does
not have anti EnumerableXXX yet, without this patch, it is still not supported,
do you think we have the necessity to add an UnsupportedOperationException ? I
kind of curious about how Calcite support an anti join now ? Correct me if i'm
wrong.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r280266371
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
`forceEquiJoin` is just a tool method, i think it is okey cause the logic
need to be complete and consistent. For current Calcite, SemiJoin must only
have equi-join conditions and we need a method to handle this while `SemiJoin`
class is deprecated.
For `EquiJoin` class i would rather to keep it, cause other systems also
need it to tell Calcite that their joins can only have equi-join condition
predicates.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r280046818
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
I have add some comments for `EquiJoin` class, we need a class like this to
tell Clacite that the join definitely can not have non-equi join conditions,
either a tool method or a class, we choose the later, it is not about what is a
`EquiJoin` that much, we just need a tag class to let Clacite enumerables and
other system to mark that instance.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279991025
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
There is a discussion about what is an `EquiJoin` in the @dev mail list, see
[1].
[1]
https://lists.apache.org/list.html?d...@calcite.apache.org:lte=1M:What%20is%20the%20exactly%20definition%20as%20an%20equi%20join
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279990280
##
File path: core/src/main/java/org/apache/calcite/rel/core/Correlate.java
##
@@ -90,13 +90,24 @@ protected Correlate(
RelNode left,
RelNode right,
CorrelationId correlationId,
- ImmutableBitSet requiredColumns, SemiJoinType joinType) {
+ ImmutableBitSet requiredColumns,
+ JoinRelType joinType) {
Review comment:
I agree, add join type check.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279982199
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -94,6 +142,37 @@ public JoinRelType cancelNullsOnRight() {
return this;
}
}
+
+ /** Transform this JoinRelType to CorrelateJoinType. **/
+ public CorrelateJoinType toLinq4j() {
+switch (this) {
+case INNER:
+ return CorrelateJoinType.INNER;
+case LEFT:
+ return CorrelateJoinType.LEFT;
+case SEMI:
+ return CorrelateJoinType.SEMI;
+case ANTI:
+ return CorrelateJoinType.ANTI;
+}
+throw new IllegalStateException(
+"Unable to convert " + this + " to CorrelateJoinType");
+ }
Review comment:
I think we can, would Deprecate `CorrelateJoinType` in calcite-linq4j
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279990187
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableJoinRule.java
##
@@ -71,26 +71,25 @@
EnumerableRules.LOGGER.debug(e.toString());
return null;
}
+} else {
+ RelNode newRel;
+ try {
+newRel = EnumerableHashJoin.create(
+left,
+right,
+info.getEquiCondition(left, right, cluster.getRexBuilder()),
Review comment:
+1, will do it in another patch.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279990063
##
File path:
core/src/main/java/org/apache/calcite/rel/rules/JoinToCorrelateRule.java
##
@@ -77,27 +75,19 @@
*/
public static final JoinToCorrelateRule JOIN =
new JoinToCorrelateRule(LogicalJoin.class, RelFactories.LOGICAL_BUILDER,
- "JoinToCorrelateRule", join ->
SemiJoinType.of(join.getJoinType()));
+ "JoinToCorrelateRule", Join::getJoinType);
@Deprecated // to be removed (should use JOIN instead), kept for backwards
compatibility
public static final JoinToCorrelateRule INSTANCE = JOIN;
- /**
- * Rule that converts a {@link org.apache.calcite.rel.core.SemiJoin}
- * into a {@link org.apache.calcite.rel.logical.LogicalCorrelate}
- */
- public static final JoinToCorrelateRule SEMI =
- new JoinToCorrelateRule(SemiJoin.class, RelFactories.LOGICAL_BUILDER,
Review comment:
I agree, add it back.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279983230
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
I agree, will keep `EquiJoin` class, and make decision based on inheritance
of it.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279982792
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -94,6 +142,37 @@ public JoinRelType cancelNullsOnRight() {
return this;
}
}
+
+ /** Transform this JoinRelType to CorrelateJoinType. **/
+ public CorrelateJoinType toLinq4j() {
+switch (this) {
+case INNER:
+ return CorrelateJoinType.INNER;
+case LEFT:
+ return CorrelateJoinType.LEFT;
+case SEMI:
+ return CorrelateJoinType.SEMI;
+case ANTI:
+ return CorrelateJoinType.ANTI;
+}
+throw new IllegalStateException(
+"Unable to convert " + this + " to CorrelateJoinType");
+ }
Review comment:
Oops, the `calcite-linq4j` module can not see `JoinRelType`, it is a
independent module apart from `calcite-core`, actually calcite-core depends on
`calcite-linq4j`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279982199
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -94,6 +142,37 @@ public JoinRelType cancelNullsOnRight() {
return this;
}
}
+
+ /** Transform this JoinRelType to CorrelateJoinType. **/
+ public CorrelateJoinType toLinq4j() {
+switch (this) {
+case INNER:
+ return CorrelateJoinType.INNER;
+case LEFT:
+ return CorrelateJoinType.LEFT;
+case SEMI:
+ return CorrelateJoinType.SEMI;
+case ANTI:
+ return CorrelateJoinType.ANTI;
+}
+throw new IllegalStateException(
+"Unable to convert " + this + " to CorrelateJoinType");
+ }
Review comment:
I think we can, would Deprecate `CorrelateJoinType` in calcite-linq4j
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279592323
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableCorrelate.java
##
@@ -76,13 +76,13 @@ public static EnumerableCorrelate create(
@Override public EnumerableCorrelate copy(RelTraitSet traitSet,
RelNode left, RelNode right, CorrelationId correlationId,
- ImmutableBitSet requiredColumns, SemiJoinType joinType) {
+ ImmutableBitSet requiredColumns, JoinRelType joinType) {
return new EnumerableCorrelate(getCluster(),
traitSet, left, right, correlationId, requiredColumns, joinType);
}
public Result implement(EnumerableRelImplementor implementor,
- Prefer pref) {
+ Prefer pref) {
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279592295
##
File path: core/src/main/java/org/apache/calcite/rel/rules/LoptMultiJoin.java
##
@@ -404,7 +404,7 @@ public Integer getJoinRemovalFactor(int dimIdx) {
* @return the semijoin that allows the join of a dimension table to be
* removed
*/
- public SemiJoin getJoinRemovalSemiJoin(int dimIdx) {
+ public LogicalJoin getJoinRemovalSemiJoin(int dimIdx) {
Review comment:
I don't think this is a breaking change, these methods are only for internal
use.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279590981
##
File path: core/src/main/java/org/apache/calcite/rel/core/Join.java
##
@@ -230,6 +237,15 @@ public boolean isSemiJoinDone() {
return false;
}
+ /**
+ * Returns whether this Join is a semijoin.
+ *
+ * @return true if this is semi but without correlate variables.
+ */
+ public boolean isSemiJoin() {
Review comment:
Cause the semiJoin decision happens everywhere, it is so special cause it
only outputs one side.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279590886
##
File path:
core/src/main/java/org/apache/calcite/rel/rules/SemiJoinProjectTransposeRule.java
##
@@ -63,42 +63,35 @@
*/
private SemiJoinProjectTransposeRule(RelBuilderFactory relBuilderFactory) {
super(
-operand(SemiJoin.class,
+operandJ(LogicalJoin.class, null, Join::isSemiJoin,
Review comment:
I have add the condition `Join::isSemiJoin`, they are thought to be
equivalent.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279324687
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableThetaJoin.java
##
@@ -45,6 +46,7 @@
/** Implementation of {@link org.apache.calcite.rel.core.Join} in
* {@link org.apache.calcite.adapter.enumerable.EnumerableConvention
enumerable calling convention}
* that allows conditions that are not just {@code =} (equals). */
+@Deprecated // to be removed before 2.0
Review comment:
Okey, have made changes:
1. Rename EnumerableThetaJoin to EnumerableNestedLoopJoin
2.Revert change to EnumerableCorrelate
BTW, sub-query.iq does not have better plan for null correlate now :(
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279324687
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableThetaJoin.java
##
@@ -45,6 +46,7 @@
/** Implementation of {@link org.apache.calcite.rel.core.Join} in
* {@link org.apache.calcite.adapter.enumerable.EnumerableConvention
enumerable calling convention}
* that allows conditions that are not just {@code =} (equals). */
+@Deprecated // to be removed before 2.0
Review comment:
Okey, have made changes:
1. Rename EnumerableThetaJoin to EnumerableNestedLoopJoin
2. Revert change to EnumerableCorrelate
BTW, sub-query.iq does not have better plan for null correlate now :(
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279246435
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableThetaJoin.java
##
@@ -45,6 +46,7 @@
/** Implementation of {@link org.apache.calcite.rel.core.Join} in
* {@link org.apache.calcite.adapter.enumerable.EnumerableConvention
enumerable calling convention}
* that allows conditions that are not just {@code =} (equals). */
+@Deprecated // to be removed before 2.0
Review comment:
The rename is ok, cause it is indeed a NestedLoopJoin and there is on other
NestedLoopJoin class in our code, After i rename EnumerableThetaJoin to
NestedLoopJoin, these 2 NestedLoopJoin logic are merged. There is no confusing
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279240026
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableThetaJoin.java
##
@@ -45,6 +46,7 @@
/** Implementation of {@link org.apache.calcite.rel.core.Join} in
* {@link org.apache.calcite.adapter.enumerable.EnumerableConvention
enumerable calling convention}
* that allows conditions that are not just {@code =} (equals). */
+@Deprecated // to be removed before 2.0
Review comment:
@hsyuan i hope to do it in another patch, cause this patch already modified
too much files, split it to another patch looks more clear, :), i can do it
also in this patch if it take no more burden for reviewing.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279227866
##
File path: core/src/main/java/org/apache/calcite/rel/core/Join.java
##
@@ -230,6 +237,17 @@ public boolean isSemiJoinDone() {
return false;
}
+ /**
+ * Returns whether this LogicalJoin is a semi-join but does
+ * not comes from decorrelate.
+ *
+ * @return true if this is semi but without correlate variables.
+ */
+ public boolean isNonCorrelateSemiJoin() {
+return (this.variablesSet == null || this.variablesSet.size() == 0)
+&& joinType == JoinRelType.SEMI;
+ }
Review comment:
Wow, my mistake, i was talking about removing variablesSet from Join.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279226492
##
File path: core/src/main/java/org/apache/calcite/rel/core/Join.java
##
@@ -230,6 +237,17 @@ public boolean isSemiJoinDone() {
return false;
}
+ /**
+ * Returns whether this LogicalJoin is a semi-join but does
+ * not comes from decorrelate.
+ *
+ * @return true if this is semi but without correlate variables.
+ */
+ public boolean isNonCorrelateSemiJoin() {
+return (this.variablesSet == null || this.variablesSet.size() == 0)
+&& joinType == JoinRelType.SEMI;
+ }
Review comment:
@hsyuan Removing variables set from Correlate is another huge change, if we
really decide to do it, i'm suggesting to put it in another patch. And i'm glad
to take it.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279226373
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableHashJoin.java
##
@@ -161,31 +142,48 @@ public static EnumerableJoin create(
} else {
rowCount += rightRowCount;
}
-return planner.getCostFactory().makeCost(rowCount, 0, 0);
+if (isNonCorrelatedSemiJoin()) {
Review comment:
If this is true, i think we can remove the variables set check and just use
analyzeCondition().getJoinType == JoinRelType.SEMI is enough
I will also have to add a assert in the Join constructor to check that we
don't allow SEMI join type + variablesSet.nonEmpty
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279223481
##
File path: core/src/main/java/org/apache/calcite/rel/core/Join.java
##
@@ -230,6 +237,17 @@ public boolean isSemiJoinDone() {
return false;
}
+ /**
+ * Returns whether this LogicalJoin is a semi-join but does
+ * not comes from decorrelate.
+ *
+ * @return true if this is semi but without correlate variables.
+ */
+ public boolean isNonCorrelateSemiJoin() {
+return (this.variablesSet == null || this.variablesSet.size() == 0)
+&& joinType == JoinRelType.SEMI;
+ }
Review comment:
I mean the Correlate actually could have a filter condition, which can be
pushed into it.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279187850
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -16,13 +16,61 @@
*/
package org.apache.calcite.rel.core;
+import org.apache.calcite.linq4j.CorrelateJoinType;
+
import java.util.Locale;
/**
* Enumeration of join types.
*/
public enum JoinRelType {
- INNER, LEFT, RIGHT, FULL;
+ /**
+ * Inner join.
+ */
+ INNER,
+
+ /**
+ * Left-outer join.
+ */
+ LEFT,
+
+ /**
+ * Right-outer join.
+ */
+ RIGHT,
Review comment:
+1 to treat it as future, thx @zabetak
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279187820
##
File path: core/src/main/java/org/apache/calcite/rel/core/Join.java
##
@@ -230,6 +237,17 @@ public boolean isSemiJoinDone() {
return false;
}
+ /**
+ * Returns whether this LogicalJoin is a semi-join but does
+ * not comes from decorrelate.
+ *
+ * @return true if this is semi but without correlate variables.
+ */
+ public boolean isNonCorrelateSemiJoin() {
+return (this.variablesSet == null || this.variablesSet.size() == 0)
+&& joinType == JoinRelType.SEMI;
+ }
Review comment:
I agree to remove variables also, as far as i can see, the variables are
used when building a Correlate[1], If we see Correlate as (Join + variableSet),
remove variableSet from Join may be the more correct way to go.
Another thing i see is that the Correlate do not have join condition, which
eliminate it's ability.
[1]https://github.com/apache/calcite/blob/a3f81bb7b088fd8c1d0c1df3b0f2b0cf122633de/core/src/main/java/org/apache/calcite/tools/RelBuilder.java#L1729
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279149970
##
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##
@@ -16,13 +16,61 @@
*/
package org.apache.calcite.rel.core;
+import org.apache.calcite.linq4j.CorrelateJoinType;
+
import java.util.Locale;
/**
* Enumeration of join types.
*/
public enum JoinRelType {
- INNER, LEFT, RIGHT, FULL;
+ /**
+ * Inner join.
+ */
+ INNER,
+
+ /**
+ * Left-outer join.
+ */
+ LEFT,
+
+ /**
+ * Right-outer join.
+ */
+ RIGHT,
Review comment:
We can remove it if we always rewrote a right join to left, but how can we
describe a right join type in the plan ?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279149934
##
File path:
core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java
##
@@ -347,7 +346,7 @@ public JoinReduceExpressionsRule(Class
joinClass,
matchNullability)) {
return;
}
- if (join instanceof EquiJoin) {
+ if (RelOptUtil.forceEquiJoin(join)) {
Review comment:
Cause for current implementation there are some Enumerable joins that do not
support non-equi join condition, this is just a sanity check, see @dev discuss:
https://lists.apache.org/list.html?d...@calcite.apache.org:lte=1M:support%20non-equi%20join
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279149741
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableJoinRule.java
##
@@ -71,26 +71,25 @@
EnumerableRules.LOGGER.debug(e.toString());
return null;
}
+} else {
+ RelNode newRel;
+ try {
+newRel = EnumerableHashJoin.create(
+left,
+right,
+info.getEquiCondition(left, right, cluster.getRexBuilder()),
+join.getVariablesSet(),
+join.getJoinType());
+ } catch (InvalidRelException e) {
+EnumerableRules.LOGGER.debug(e.toString());
+return null;
+ }
+ if (!info.isEqui()) {
+newRel = new EnumerableFilter(cluster, newRel.getTraitSet(),
+newRel, info.getRemaining(cluster.getRexBuilder()));
+ }
Review comment:
For the current implementation, the SemiJoin's join info is definitely equi,
so it is okey to not shift the join keys, actually if the join only outputs
oneside, we could not even add a filter here if the filter keys comes from the
non-output side.
We can merge the EquiJoinInfo and NonEquiJoinInfo into JoinInfo, but this
has non relationship with code snippet here. Actually for the first commit i
merged them, but seems not that point.
I agree that we should add a check here for adding filer, although the
current implementation can guarantee 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279149741
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableJoinRule.java
##
@@ -71,26 +71,25 @@
EnumerableRules.LOGGER.debug(e.toString());
return null;
}
+} else {
+ RelNode newRel;
+ try {
+newRel = EnumerableHashJoin.create(
+left,
+right,
+info.getEquiCondition(left, right, cluster.getRexBuilder()),
+join.getVariablesSet(),
+join.getJoinType());
+ } catch (InvalidRelException e) {
+EnumerableRules.LOGGER.debug(e.toString());
+return null;
+ }
+ if (!info.isEqui()) {
+newRel = new EnumerableFilter(cluster, newRel.getTraitSet(),
+newRel, info.getRemaining(cluster.getRexBuilder()));
+ }
Review comment:
For the current implementation, the SemiJoin's join info is definitely equi,
so it is okey to not shift the join keys, actually if the join only outputs
oneside, we could not even add a filter here if the filter keys comes from the
non-output side.
We can merge the EquiJoinInfo and NonEquiJoinInfo into JoinInfo, but this
has non relationship with code snippet here. Actually for the first commit i
merged them, but seems not that pointless.
I agree that we should add a check here for adding filer, although the
current implementation can guarantee 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279149384
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableHashJoin.java
##
@@ -161,31 +142,48 @@ public static EnumerableJoin create(
} else {
rowCount += rightRowCount;
}
-return planner.getCostFactory().makeCost(rowCount, 0, 0);
+if (isNonCorrelatedSemiJoin()) {
+ return planner.getCostFactory().makeCost(rowCount, 0,
0).multiplyBy(.01d);
Review comment:
I don't know the implementation details either, just copy it from
EnumerableSemiJoin, the oldest commit about it is 2014 and seems just a code
refactor, so i just keep it as before.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279149207
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableHashJoin.java
##
@@ -161,31 +142,48 @@ public static EnumerableJoin create(
} else {
rowCount += rightRowCount;
}
-return planner.getCostFactory().makeCost(rowCount, 0, 0);
+if (isNonCorrelatedSemiJoin()) {
Review comment:
If it is a semi-join type, It is definitely not correlated in the current
implementation, but i still want to keep this check, cause i don't want it to
be a implicit rule that guaranteed by the internal design, after all, correlate
counts to SEMI-JOIN, although we never decorrelate to such join type.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279149109
##
File path: core/src/main/java/org/apache/calcite/rel/core/Join.java
##
@@ -230,6 +237,17 @@ public boolean isSemiJoinDone() {
return false;
}
+ /**
+ * Returns whether this LogicalJoin is a semi-join but does
+ * not comes from decorrelate.
+ *
+ * @return true if this is semi but without correlate variables.
+ */
+ public boolean isNonCorrelateSemiJoin() {
+return (this.variablesSet == null || this.variablesSet.size() == 0)
+&& joinType == JoinRelType.SEMI;
+ }
Review comment:
Another reason i see is that Calcite supports converting from a Correlate to
Join (both for logical nodes and physical), i can't see another way how to keep
these variables into a transformed join.
If Correlate is just a correlate and have it's self implementation, the
concept is more clear, and i supports @hsyuan 's idea but we don't now.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r279148975
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableThetaJoin.java
##
@@ -45,6 +46,7 @@
/** Implementation of {@link org.apache.calcite.rel.core.Join} in
* {@link org.apache.calcite.adapter.enumerable.EnumerableConvention
enumerable calling convention}
* that allows conditions that are not just {@code =} (equals). */
+@Deprecated // to be removed before 2.0
Review comment:
I agree to rename EnumerableThetaJoin as EnumerableNestedLoopJoin and
enhance the implementation of the current EnumerableNestedLoopJoin, i would do
it in another patch.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276970943
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
@rubenada same as SemiJoin, i just deprecate it, cause the JoinInfo already
can describe if the join is a equi-join.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276927441
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
@hsyuan @rubenada I have rename the method name to forceEquiJoin, cause now
some join Enumerable nodes force the join condition must be equi-join, like
semi/hash/merge join. So some rule needs this info to prevent some unexpected
behavior.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276927000
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
@hsyuan @rubenada I have rename the method name to forceEquiJoin, cause now
some join Enumerable nodes force the join condition must be equi-join, like
semi/hash/merge join. So some rule needs this info to prevent some unexpected
behavior.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276927000
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
@hsyuan @rubenada I have rename the method name to forceEquiJoin, cause now
some join Enumerable nodes force the join condition must be equi-join, like
semi/hash/merge join. So some rule needs this info to prevent some unexpected
behavior.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276601185
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
There are 2 rules `ReduceExpressionsRule` and `FilterJoinRule` that use this
method to make some decision when planning, this method returns what the
original EquiJoin is, cause now the join.analyzeCondition().isEqui() has wider
scope that this method returns. I thought maybe we should move the logic to the
rules or tweak the rules logic.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276601185
##
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##
@@ -3734,6 +3740,13 @@ private Exists(RelNode r, boolean indicator, boolean
outerJoin) {
this.outerJoin = outerJoin;
}
}
+
+ /** Check if it is the join whose condition is based on column equality. */
+ public static boolean isEquiJoin(Join join) {
+return join.isNonCorrelateSemiJoin()
+|| join instanceof EnumerableHashJoin
+|| join instanceof EnumerableMergeJoin;
+ }
Review comment:
There are 2 rules that use this method to make some decision when planning,
this method returns what the original EquiJoin is, cause now the
join.analyzeCondition().isEqui() has wider scope that this method returns. I
thought maybe we should move the logic to the rules or tweak the rules logic.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276599667
##
File path: core/src/main/java/org/apache/calcite/rel/core/Join.java
##
@@ -230,6 +237,17 @@ public boolean isSemiJoinDone() {
return false;
}
+ /**
+ * Returns whether this LogicalJoin is a semi-join but does
+ * not comes from decorrelate.
+ *
+ * @return true if this is semi but without correlate variables.
+ */
+ public boolean isNonCorrelateSemiJoin() {
+return (this.variablesSet == null || this.variablesSet.size() == 0)
+&& joinType == JoinRelType.SEMI;
+ }
Review comment:
In the beginning, i add this method to distinguish the `SemiJoin`
transformed from `Correlate` and `SemiJoin` from `SemiJoinRule` or
`RelBuilder`. Cause i thought that the join comes from a `Correlate` can make a
JoinRelType.SEMI join type.
But in current code, Calcite will never transform from Correlate to join
with `JoinRelType.SEMI`(only left and inner join type), so this method actually
can simplify to just decide if the join type is `JoinRelType.SEMI`.
But i still keep the variables check and i think correlate should have the
ability to get a semi join type join.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276591409
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableNestedLoopJoin.java
##
@@ -0,0 +1,245 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.adapter.enumerable;
+
+import org.apache.calcite.linq4j.tree.BlockBuilder;
+import org.apache.calcite.linq4j.tree.Expression;
+import org.apache.calcite.linq4j.tree.Expressions;
+import org.apache.calcite.linq4j.tree.ParameterExpression;
+import org.apache.calcite.linq4j.tree.Primitive;
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelOptCost;
+import org.apache.calcite.plan.RelOptPlanner;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.RelCollationTraitDef;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.RelWriter;
+import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.rel.metadata.RelMdCollation;
+import org.apache.calcite.rel.metadata.RelMetadataQuery;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.util.BuiltInMethod;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.Util;
+
+import com.google.common.collect.ImmutableList;
+
+import java.lang.reflect.Modifier;
+import java.lang.reflect.Type;
+import java.util.Set;
+
+/** Implementation of {@link org.apache.calcite.rel.core.Correlate} in
+ * {@link org.apache.calcite.adapter.enumerable.EnumerableConvention
enumerable calling convention}. */
+public class EnumerableNestedLoopJoin extends Join
+implements EnumerableRel {
+ private final ImmutableBitSet requiredColumns;
+
+ public EnumerableNestedLoopJoin(RelOptCluster cluster, RelTraitSet traits,
+ RelNode left, RelNode right, RexNode condition, Set
variablesSet,
+ ImmutableBitSet requiredColumns, JoinRelType joinType) {
+super(cluster, traits, left, right, condition, variablesSet, joinType);
+this.requiredColumns = requiredColumns;
+ }
+
+ /** Creates an EnumerableNestedLoopJoin. */
+ public static EnumerableNestedLoopJoin create(
+ RelNode left,
+ RelNode right,
+ RexNode condition,
+ Set variablesSet,
+ ImmutableBitSet requiredColumns,
+ JoinRelType joinType) {
+assert variablesSet.size() == 0 || variablesSet.size() == 1;
+final RelOptCluster cluster = left.getCluster();
+final RelMetadataQuery mq = cluster.getMetadataQuery();
+final RelTraitSet traitSet =
+cluster.traitSetOf(EnumerableConvention.INSTANCE)
+.replaceIfs(RelCollationTraitDef.INSTANCE,
+() -> RelMdCollation.enumerableCorrelate(mq, left, right,
joinType));
+return new EnumerableNestedLoopJoin(
+cluster,
+traitSet,
+left,
+right,
+condition,
+variablesSet,
+requiredColumns,
+joinType);
+ }
+
+ @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+ RelMetadataQuery mq) {
+double rowCount = mq.getRowCount(this);
+
+// Right-hand input is the "build", and hopefully small, input.
+final double rightRowCount = right.estimateRowCount(mq);
+final double leftRowCount = left.estimateRowCount(mq);
+
+if (isNonCorrelateSemiJoin()) {
+ if (Double.isInfinite(leftRowCount)) {
+rowCount = leftRowCount;
+ } else {
+rowCount += Util.nLogN(leftRowCount);
+ }
+ if (Double.isInfinite(rightRowCount)) {
+rowCount = rightRowCount;
+ } else {
+rowCount += rightRowCount;
+ }
+ return planner.getCostFactory().makeCost(rowCount, 0,
0).multiplyBy(.01d);
+} else {
+ if (Double.isInfinite(leftRowCount) || Double.isInfinite(rightRowCount))
{
+return planner.getCostFactory().makeInfiniteCost();
+ }
+
+ Double restartCount = mq.getRowCount(getLeft());
+ // RelMetadataQuery.getCumulativeCost(getRight()); does not work for
+ // RelSubset, so we ask
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276591409
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableNestedLoopJoin.java
##
@@ -0,0 +1,245 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.adapter.enumerable;
+
+import org.apache.calcite.linq4j.tree.BlockBuilder;
+import org.apache.calcite.linq4j.tree.Expression;
+import org.apache.calcite.linq4j.tree.Expressions;
+import org.apache.calcite.linq4j.tree.ParameterExpression;
+import org.apache.calcite.linq4j.tree.Primitive;
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelOptCost;
+import org.apache.calcite.plan.RelOptPlanner;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.RelCollationTraitDef;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.RelWriter;
+import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.rel.metadata.RelMdCollation;
+import org.apache.calcite.rel.metadata.RelMetadataQuery;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.util.BuiltInMethod;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.Util;
+
+import com.google.common.collect.ImmutableList;
+
+import java.lang.reflect.Modifier;
+import java.lang.reflect.Type;
+import java.util.Set;
+
+/** Implementation of {@link org.apache.calcite.rel.core.Correlate} in
+ * {@link org.apache.calcite.adapter.enumerable.EnumerableConvention
enumerable calling convention}. */
+public class EnumerableNestedLoopJoin extends Join
+implements EnumerableRel {
+ private final ImmutableBitSet requiredColumns;
+
+ public EnumerableNestedLoopJoin(RelOptCluster cluster, RelTraitSet traits,
+ RelNode left, RelNode right, RexNode condition, Set
variablesSet,
+ ImmutableBitSet requiredColumns, JoinRelType joinType) {
+super(cluster, traits, left, right, condition, variablesSet, joinType);
+this.requiredColumns = requiredColumns;
+ }
+
+ /** Creates an EnumerableNestedLoopJoin. */
+ public static EnumerableNestedLoopJoin create(
+ RelNode left,
+ RelNode right,
+ RexNode condition,
+ Set variablesSet,
+ ImmutableBitSet requiredColumns,
+ JoinRelType joinType) {
+assert variablesSet.size() == 0 || variablesSet.size() == 1;
+final RelOptCluster cluster = left.getCluster();
+final RelMetadataQuery mq = cluster.getMetadataQuery();
+final RelTraitSet traitSet =
+cluster.traitSetOf(EnumerableConvention.INSTANCE)
+.replaceIfs(RelCollationTraitDef.INSTANCE,
+() -> RelMdCollation.enumerableCorrelate(mq, left, right,
joinType));
+return new EnumerableNestedLoopJoin(
+cluster,
+traitSet,
+left,
+right,
+condition,
+variablesSet,
+requiredColumns,
+joinType);
+ }
+
+ @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+ RelMetadataQuery mq) {
+double rowCount = mq.getRowCount(this);
+
+// Right-hand input is the "build", and hopefully small, input.
+final double rightRowCount = right.estimateRowCount(mq);
+final double leftRowCount = left.estimateRowCount(mq);
+
+if (isNonCorrelateSemiJoin()) {
+ if (Double.isInfinite(leftRowCount)) {
+rowCount = leftRowCount;
+ } else {
+rowCount += Util.nLogN(leftRowCount);
+ }
+ if (Double.isInfinite(rightRowCount)) {
+rowCount = rightRowCount;
+ } else {
+rowCount += rightRowCount;
+ }
+ return planner.getCostFactory().makeCost(rowCount, 0,
0).multiplyBy(.01d);
+} else {
+ if (Double.isInfinite(leftRowCount) || Double.isInfinite(rightRowCount))
{
+return planner.getCostFactory().makeInfiniteCost();
+ }
+
+ Double restartCount = mq.getRowCount(getLeft());
+ // RelMetadataQuery.getCumulativeCost(getRight()); does not work for
+ // RelSubset, so we ask
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276497848
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableJoinRule.java
##
@@ -71,26 +72,34 @@
EnumerableRules.LOGGER.debug(e.toString());
return null;
}
-}
-RelNode newRel;
-try {
- newRel = EnumerableJoin.create(
+} else if (info.isEqui() && join.isNonCorrelateSemiJoin()) {
+ // This is a semi-join.
+ return EnumerableNestedLoopJoin.create(
Review comment:
This question is gone if we move semiJoin implementation to
EnumerableHashJoin.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276497024
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableNestedLoopJoin.java
##
@@ -0,0 +1,245 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.adapter.enumerable;
+
+import org.apache.calcite.linq4j.tree.BlockBuilder;
+import org.apache.calcite.linq4j.tree.Expression;
+import org.apache.calcite.linq4j.tree.Expressions;
+import org.apache.calcite.linq4j.tree.ParameterExpression;
+import org.apache.calcite.linq4j.tree.Primitive;
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.RelOptCost;
+import org.apache.calcite.plan.RelOptPlanner;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.RelCollationTraitDef;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.RelWriter;
+import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.rel.metadata.RelMdCollation;
+import org.apache.calcite.rel.metadata.RelMetadataQuery;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.util.BuiltInMethod;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.Util;
+
+import com.google.common.collect.ImmutableList;
+
+import java.lang.reflect.Modifier;
+import java.lang.reflect.Type;
+import java.util.Set;
+
+/** Implementation of {@link org.apache.calcite.rel.core.Correlate} in
+ * {@link org.apache.calcite.adapter.enumerable.EnumerableConvention
enumerable calling convention}. */
+public class EnumerableNestedLoopJoin extends Join
+implements EnumerableRel {
+ private final ImmutableBitSet requiredColumns;
+
+ public EnumerableNestedLoopJoin(RelOptCluster cluster, RelTraitSet traits,
+ RelNode left, RelNode right, RexNode condition, Set
variablesSet,
+ ImmutableBitSet requiredColumns, JoinRelType joinType) {
+super(cluster, traits, left, right, condition, variablesSet, joinType);
+this.requiredColumns = requiredColumns;
+ }
+
+ /** Creates an EnumerableNestedLoopJoin. */
+ public static EnumerableNestedLoopJoin create(
+ RelNode left,
+ RelNode right,
+ RexNode condition,
+ Set variablesSet,
+ ImmutableBitSet requiredColumns,
+ JoinRelType joinType) {
+assert variablesSet.size() == 0 || variablesSet.size() == 1;
+final RelOptCluster cluster = left.getCluster();
+final RelMetadataQuery mq = cluster.getMetadataQuery();
+final RelTraitSet traitSet =
+cluster.traitSetOf(EnumerableConvention.INSTANCE)
+.replaceIfs(RelCollationTraitDef.INSTANCE,
+() -> RelMdCollation.enumerableCorrelate(mq, left, right,
joinType));
+return new EnumerableNestedLoopJoin(
+cluster,
+traitSet,
+left,
+right,
+condition,
+variablesSet,
+requiredColumns,
+joinType);
+ }
+
+ @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+ RelMetadataQuery mq) {
+double rowCount = mq.getRowCount(this);
+
+// Right-hand input is the "build", and hopefully small, input.
+final double rightRowCount = right.estimateRowCount(mq);
+final double leftRowCount = left.estimateRowCount(mq);
+
+if (isNonCorrelateSemiJoin()) {
+ if (Double.isInfinite(leftRowCount)) {
+rowCount = leftRowCount;
+ } else {
+rowCount += Util.nLogN(leftRowCount);
+ }
+ if (Double.isInfinite(rightRowCount)) {
+rowCount = rightRowCount;
+ } else {
+rowCount += rightRowCount;
+ }
+ return planner.getCostFactory().makeCost(rowCount, 0,
0).multiplyBy(.01d);
+} else {
+ if (Double.isInfinite(leftRowCount) || Double.isInfinite(rightRowCount))
{
+return planner.getCostFactory().makeInfiniteCost();
+ }
+
+ Double restartCount = mq.getRowCount(getLeft());
+ // RelMetadataQuery.getCumulativeCost(getRight()); does not work for
+ // RelSubset, so we ask
[GitHub] [calcite] danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
danny0405 commented on a change in pull request #1157: [CALCITE-2969] Improve
design of join-like relational expressions
URL: https://github.com/apache/calcite/pull/1157#discussion_r276496556
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableThetaJoin.java
##
@@ -45,6 +46,7 @@
/** Implementation of {@link org.apache.calcite.rel.core.Join} in
* {@link org.apache.calcite.adapter.enumerable.EnumerableConvention
enumerable calling convention}
* that allows conditions that are not just {@code =} (equals). */
+@Deprecated // to be removed before 2.0
Review comment:
Theta join can be implement through other like join, e.g. NestedLoopJoin, i
want to refactor it also, but not in this patch, i agree to remove the
annotation now
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
