[GitHub] [calcite] chunweilei commented on a change in pull request #1186: [CALCITE-3003] AssertionError when GROUP BY nested field
chunweilei commented on a change in pull request #1186: [CALCITE-3003] AssertionError when GROUP BY nested field URL: https://github.com/apache/calcite/pull/1186#discussion_r279174752 ## File path: core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorUtil.java ## @@ -836,7 +836,7 @@ private static ImmutableBitSet analyzeGroupExpr(SqlValidatorScope scope, SqlIdentifier expr = (SqlIdentifier) expandedGroupExpr; // column references should be fully qualified. - assert expr.names.size() == 2; + assert expr.names.size() >= 2; String originalRelName = expr.names.get(0); String originalFieldName = expr.names.get(1); Review comment: Maybe we should check whether the first name is the name of a table alias in scope. 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] my7ym opened a new pull request #1186: [CALCITE-3003] AssertionError when GROUP BY nested field
my7ym opened a new pull request #1186: [CALCITE-3003] AssertionError when GROUP BY nested field URL: https://github.com/apache/calcite/pull/1186 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] vineetgarg02 opened a new pull request #1185: [CALCITE-3012] areColumnsUnique for FULL OUTER JOIN could return wrong answer when ignoreNulls is false
vineetgarg02 opened a new pull request #1185: [CALCITE-3012] areColumnsUnique for FULL OUTER JOIN could return wrong answer when ignoreNulls is false URL: https://github.com/apache/calcite/pull/1185 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] hsyuan commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
hsyuan 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_r279158905
##
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:
Yes, we can rename EnumerableThetaJoin to EnumerableNestedLoopJoin, and keep
EnumerableCorrelate as the LogicalCorrelate's physical implementation, to make
things easier.
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] hsyuan commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
hsyuan 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_r279158703
##
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:
NestedLoop can represent both Join and Correlate. Say for query
```
select * from R, S where r1 > s1;
```
We can have plan:
```
NLJ (r1 > s1)
+- R
+- S
for (r in R) {
for (s in S) {
if (condition(r, s) is true)
output r,s;
}
}
```
or
```
NLJ (true)
+- R
+- Filter (r1 > s1)
+- S
for (r in R) {
while (s = scanNext(innerRel, r))
output r,s;
}
```
The difference is can we start fetching inner tuple without knowing the
tuple from outer. It is impossible for the 2nd, which is a Correlate
implementation. It is especially true if the inner is a index scan.
It is true that we can always use the second one to implement it, depending
on the implementation, that's why we have JoinToCorrelate rule.
But logically I think we'd better not mix them.
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] hsyuan commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
hsyuan 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_r279157499
##
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:
@danny0405 The purpose of transforming a correlate to a join is getting rid
of Variables.
@zabetak It is in RelDecorrelator, which is not rule based, sadly. Some
SubqueryRemoveRule will transform subquery into Correlate, some
SubqueryRemoveRule will directly transform subquery into a 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
[calcite] branch master updated: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei, Alexander Shilov)
This is an automated email from the ASF dual-hosted git repository.
zabetak pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/master by this push:
new a3f81bb [CALCITE-2998] RexCopier should support all rex types
(Chunwei Lei, Alexander Shilov)
a3f81bb is described below
commit a3f81bb7b088fd8c1d0c1df3b0f2b0cf122633de
Author: Chunwei Lei
AuthorDate: Sun Apr 14 18:40:21 2019 +0800
[CALCITE-2998] RexCopier should support all rex types (Chunwei Lei,
Alexander Shilov)
Close apache/calcite#1164
Close apache/calcite#969
---
.../java/org/apache/calcite/rex/RexCopier.java | 22 ++-
.../org/apache/calcite/rex/RexBuilderTest.java | 155 +
2 files changed, 165 insertions(+), 12 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/rex/RexCopier.java
b/core/src/main/java/org/apache/calcite/rex/RexCopier.java
index 7b66086..f207750 100644
--- a/core/src/main/java/org/apache/calcite/rex/RexCopier.java
+++ b/core/src/main/java/org/apache/calcite/rex/RexCopier.java
@@ -24,9 +24,6 @@ import org.apache.calcite.rel.type.RelDataType;
* This is useful when copying objects from one type factory or builder to
* another.
*
- * Due to the laziness of the author, not all Rex types are supported at
- * present.
- *
* @see RexBuilder#copy(RexNode)
*/
class RexCopier extends RexShuttle {
@@ -52,11 +49,10 @@ class RexCopier extends RexShuttle {
}
public RexNode visitOver(RexOver over) {
-throw new UnsupportedOperationException();
- }
-
- public RexWindow visitWindow(RexWindow window) {
-throw new UnsupportedOperationException();
+final boolean[] update = null;
+return new RexOver(copy(over.getType()), over.getAggOperator(),
+visitList(over.getOperands(), update), visitWindow(over.getWindow()),
+over.isDistinct(), over.ignoreNulls());
}
public RexNode visitCall(final RexCall call) {
@@ -67,7 +63,7 @@ class RexCopier extends RexShuttle {
}
public RexNode visitCorrelVariable(RexCorrelVariable variable) {
-throw new UnsupportedOperationException();
+return builder.makeCorrel(copy(variable.getType()), variable.id);
}
public RexNode visitFieldAccess(RexFieldAccess fieldAccess) {
@@ -80,7 +76,7 @@ class RexCopier extends RexShuttle {
}
public RexNode visitLocalRef(RexLocalRef localRef) {
-throw new UnsupportedOperationException();
+return new RexLocalRef(localRef.getIndex(), copy(localRef.getType()));
}
public RexNode visitLiteral(RexLiteral literal) {
@@ -90,11 +86,13 @@ class RexCopier extends RexShuttle {
}
public RexNode visitDynamicParam(RexDynamicParam dynamicParam) {
-throw new UnsupportedOperationException();
+return builder.makeDynamicParam(copy(dynamicParam.getType()),
+dynamicParam.getIndex());
}
public RexNode visitRangeRef(RexRangeRef rangeRef) {
-throw new UnsupportedOperationException();
+return builder.makeRangeReference(copy(rangeRef.getType()),
+rangeRef.getOffset(), false);
}
}
diff --git a/core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java
b/core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java
index b8b30f0..780469a 100644
--- a/core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java
+++ b/core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java
@@ -17,10 +17,15 @@
package org.apache.calcite.rex;
import org.apache.calcite.avatica.util.ByteString;
+import org.apache.calcite.rel.core.CorrelationId;
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.rel.type.RelDataTypeSystem;
import org.apache.calcite.sql.SqlCollation;
+import org.apache.calcite.sql.SqlWindow;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.BasicSqlType;
import org.apache.calcite.sql.type.SqlTypeFactoryImpl;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.util.DateString;
@@ -30,6 +35,9 @@ import org.apache.calcite.util.TimestampString;
import org.apache.calcite.util.TimestampWithTimeZoneString;
import org.apache.calcite.util.Util;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+
import org.junit.Test;
import java.math.BigDecimal;
@@ -44,6 +52,7 @@ import static org.hamcrest.core.Is.is;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
/**
@@ -51,6 +60,30 @@ import static org.junit.Assert.fail;
*/
public class RexBuilderTest {
+ private static final int PRECISION = 256;
+
+ /**
+ * MySqlTypeFactoryImpl provides a specific implementation of
+ * {@link SqlType
[GitHub] [calcite] zabetak closed pull request #969: [CALCITE-2738] Add support for copying of RexDynamicParam and RexRangeRef to RexCopier
zabetak closed pull request #969: [CALCITE-2738] Add support for copying of RexDynamicParam and RexRangeRef to RexCopier URL: https://github.com/apache/calcite/pull/969 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] zabetak closed pull request #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei)
zabetak closed pull request #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei) URL: https://github.com/apache/calcite/pull/1164 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] chunweilei commented on issue #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei)
chunweilei commented on issue #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei) URL: https://github.com/apache/calcite/pull/1164#issuecomment-487284735 Update the PR to remove the `visitWindow` method in `RexCopier`. Thanks for your review, @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] zabetak commented on a change in pull request #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei)
zabetak commented on a change in pull request #1164: [CALCITE-2998] RexCopier
should support all rex types (Chunwei Lei)
URL: https://github.com/apache/calcite/pull/1164#discussion_r279153574
##
File path: core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java
##
@@ -44,13 +52,38 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
/**
* Test for {@link RexBuilder}.
*/
public class RexBuilderTest {
+ private static final int PRECISION = 256;
+
+ /**
+ * MySqlTypeFactoryImpl provides a specific implementation of
+ * {@link SqlTypeFactoryImpl} which sets precision to 256 for VARCHAR.
+ */
+ private static class MySqlTypeFactoryImpl extends SqlTypeFactoryImpl {
+
+MySqlTypeFactoryImpl(RelDataTypeSystem typeSystem) {
+ super(typeSystem);
+}
+
+@Override public RelDataType createTypeWithNullability(
+final RelDataType type,
+final boolean nullable) {
+ if (type.getSqlTypeName() == SqlTypeName.VARCHAR) {
Review comment:
Indeed you are right, let's keep it as is!
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] zabetak commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
zabetak 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_r279153158
##
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:
It should be always possible to express a right join as a left. If that's
too complicated to do in this PR we can treat it in the future ;)
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] zabetak commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
zabetak 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_r279153072
##
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:
Currently we have two concepts `Join` and `Correlate`. If we say that the
`Join` can have correlated variables then I don't see why we need to keep
`Correlate`.
The discussion so far (and the current PR) proposes to keep `Correlate` so
that's why I think that `Join` should not deal with correlated variables.
> Calcite supports converting from a Correlate to Join ...
@danny0405 can you explain a bit more where this is happening? I know that
the opposite is done with JoinToCorrelateRule but I don't remember seeing
CorrelateToJoinRule (or something similar). We can certainly add such a rule
but I argue that in this case we should be able to get rid of correlated
variables.
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] xpleaf opened a new pull request #1184: [CALCITE-3027] Support like query in Elasticsearch
xpleaf opened a new pull request #1184: [CALCITE-3027] Support like query in
Elasticsearch
URL: https://github.com/apache/calcite/pull/1184
In Elasticsearch, fuzzy matching is implemented by wildcard query:
```
GET /company/_search
{
"query": {
"constant_score": {
"filter": {
"wildcard":{
"name_text":"*Alle_"
}
}
}
}
}
```
> The symbols % and _ in sql are equivalent to the symbols * and ? in es,
respectively.
So in this PR, I added a new QueryBuilder called WildcardQueryBuilder to
support wildcard queries, then to achieve like query in sql.
JIRA: https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-3027
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] chunweilei commented on a change in pull request #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei)
chunweilei commented on a change in pull request #1164: [CALCITE-2998]
RexCopier should support all rex types (Chunwei Lei)
URL: https://github.com/apache/calcite/pull/1164#discussion_r279152055
##
File path: core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java
##
@@ -44,13 +52,38 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
/**
* Test for {@link RexBuilder}.
*/
public class RexBuilderTest {
+ private static final int PRECISION = 256;
+
+ /**
+ * MySqlTypeFactoryImpl provides a specific implementation of
+ * {@link SqlTypeFactoryImpl} which sets precision to 256 for VARCHAR.
+ */
+ private static class MySqlTypeFactoryImpl extends SqlTypeFactoryImpl {
+
+MySqlTypeFactoryImpl(RelDataTypeSystem typeSystem) {
+ super(typeSystem);
+}
+
+@Override public RelDataType createTypeWithNullability(
+final RelDataType type,
+final boolean nullable) {
+ if (type.getSqlTypeName() == SqlTypeName.VARCHAR) {
Review comment:
I am afraid it cannot work. The whole call stack is as follows:
```
RexCopier#copy
=> RelDataTypeFactoryImpl#copy
=> SqlTypeFactoryImpl#createTypeWithNullability
=> BasicSqlType#createWithNullability
=> new BasicSqlType(...)
```
You can see it does not use `getDefaultPrecision` in
`RelDataTypeSystemImpl`. Another advantage of creating `MySqlTypeFactoryImpl`
is that we can easily tell it is a special SqlTypeFactory for test.
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] chunweilei commented on a change in pull request #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei)
chunweilei commented on a change in pull request #1164: [CALCITE-2998]
RexCopier should support all rex types (Chunwei Lei)
URL: https://github.com/apache/calcite/pull/1164#discussion_r279152055
##
File path: core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java
##
@@ -44,13 +52,38 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
/**
* Test for {@link RexBuilder}.
*/
public class RexBuilderTest {
+ private static final int PRECISION = 256;
+
+ /**
+ * MySqlTypeFactoryImpl provides a specific implementation of
+ * {@link SqlTypeFactoryImpl} which sets precision to 256 for VARCHAR.
+ */
+ private static class MySqlTypeFactoryImpl extends SqlTypeFactoryImpl {
+
+MySqlTypeFactoryImpl(RelDataTypeSystem typeSystem) {
+ super(typeSystem);
+}
+
+@Override public RelDataType createTypeWithNullability(
+final RelDataType type,
+final boolean nullable) {
+ if (type.getSqlTypeName() == SqlTypeName.VARCHAR) {
Review comment:
I am afraid it cannot work. The whole call stack is as follows:
```
RexCopier#copy
=> RelDataTypeFactoryImpl#copy
=> SqlTypeFactoryImpl#createTypeWithNullability
=> BasicSqlType#createWithNullability
=> new BasicSqlType(...)
```
You can see it does not use `getDefaultPrecision` in `RelDataTypeSystemImpl`.
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] chunweilei commented on a change in pull request #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei)
chunweilei commented on a change in pull request #1164: [CALCITE-2998]
RexCopier should support all rex types (Chunwei Lei)
URL: https://github.com/apache/calcite/pull/1164#discussion_r279151435
##
File path: core/src/main/java/org/apache/calcite/rex/RexCopier.java
##
@@ -52,11 +53,30 @@ private RelDataType copy(RelDataType type) {
}
public RexNode visitOver(RexOver over) {
-throw new UnsupportedOperationException();
+final boolean[] update = null;
+return new RexOver(copy(over.getType()), over.getAggOperator(),
+visitList(over.getOperands(), update), visitWindow(over.getWindow()),
+over.isDistinct(), over.ignoreNulls());
}
public RexWindow visitWindow(RexWindow window) {
Review comment:
Yes, we can. I keep it on purpose to tell we don't forget this method. I
will remove it since it seems redundant.
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] julianhyde commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
julianhyde 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_r279147198
##
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 can’t imagine how to achieve a nested-loop join without a correlating
variable. The left side sets it for each row, and the right side restarts and
applies some condition that uses the variable. So we’re just arguing over
nomenclature.
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] zabetak commented on a change in pull request #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei)
zabetak commented on a change in pull request #1164: [CALCITE-2998] RexCopier
should support all rex types (Chunwei Lei)
URL: https://github.com/apache/calcite/pull/1164#discussion_r279147199
##
File path: core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java
##
@@ -44,13 +52,38 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
/**
* Test for {@link RexBuilder}.
*/
public class RexBuilderTest {
+ private static final int PRECISION = 256;
+
+ /**
+ * MySqlTypeFactoryImpl provides a specific implementation of
+ * {@link SqlTypeFactoryImpl} which sets precision to 256 for VARCHAR.
+ */
+ private static class MySqlTypeFactoryImpl extends SqlTypeFactoryImpl {
+
+MySqlTypeFactoryImpl(RelDataTypeSystem typeSystem) {
+ super(typeSystem);
+}
+
+@Override public RelDataType createTypeWithNullability(
+final RelDataType type,
+final boolean nullable) {
+ if (type.getSqlTypeName() == SqlTypeName.VARCHAR) {
Review comment:
A better way to do this would be to subclass `RelDataTypeSystemImpl` and
override `getDefaultPrecision` method which is meant exactly for this. Then
you can simply instantiate SqlTypeFactoryImpl with the custom type system.
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] zabetak commented on a change in pull request #1164: [CALCITE-2998] RexCopier should support all rex types (Chunwei Lei)
zabetak commented on a change in pull request #1164: [CALCITE-2998] RexCopier
should support all rex types (Chunwei Lei)
URL: https://github.com/apache/calcite/pull/1164#discussion_r279147130
##
File path: core/src/main/java/org/apache/calcite/rex/RexCopier.java
##
@@ -52,11 +53,30 @@ private RelDataType copy(RelDataType type) {
}
public RexNode visitOver(RexOver over) {
-throw new UnsupportedOperationException();
+final boolean[] update = null;
+return new RexOver(copy(over.getType()), over.getAggOperator(),
+visitList(over.getOperands(), update), visitWindow(over.getWindow()),
+over.isDistinct(), over.ignoreNulls());
}
public RexWindow visitWindow(RexWindow window) {
Review comment:
I guess you could remove entirely the method, instead of calling super.
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] zabetak commented on a change in pull request #1157: [CALCITE-2969] Improve design of join-like relational expressions
zabetak 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_r279146938
##
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:
Do we really need both `LEFT` and `RIGHT` join types?
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
