[GitHub] [calcite] angelzouxin commented on a change in pull request #2116: [CALCITE-4188] support EnumerableBatchNestedLoopJoin inJdbcToEnumerab…
angelzouxin commented on a change in pull request #2116:
URL: https://github.com/apache/calcite/pull/2116#discussion_r476155337
##
File path:
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java
##
@@ -195,7 +235,14 @@ public Result implement(EnumerableRelImplementor
implementor, Prefer pref) {
}
private List toIndexesTableExpression(SqlString
sqlString) {
-return sqlString.getDynamicParameters().stream()
+return
Optional.ofNullable(sqlString.getDynamicParameters()).orElse(ImmutableList.of()).stream()
+.map(Expressions::constant)
+.collect(Collectors.toList());
+ }
+
+ private List toIndexCorrelateExpression(SqlString
sqlString,
+ StringBuilder querySqlSb, SqlDialect sqlDialect) {
+return sqlDialect.getDynamicTypeIndexs(querySqlSb,
sqlString.getSql()).stream()
Review comment:
you're right, i changed the implementation
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
[calcite] branch master updated: [CALCITE-4190] OR simplification incorrectly loses term
This is an automated email from the ASF dual-hosted git repository.
jhyde 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 401b018 [CALCITE-4190] OR simplification incorrectly loses term
401b018 is described below
commit 401b01897b9a3b588d38acb6459411c5f7805776
Author: Julian Hyde
AuthorDate: Sun Aug 23 14:47:18 2020 -0700
[CALCITE-4190] OR simplification incorrectly loses term
---
.../java/org/apache/calcite/rex/RexSimplify.java | 13 +--
.../org/apache/calcite/rex/RexProgramTest.java | 42 ++
2 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
index 5e92098..b48fd7e 100644
--- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
+++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
@@ -47,6 +47,7 @@ import com.google.common.collect.Sets;
import com.google.common.collect.TreeRangeSet;
import java.util.ArrayList;
+import java.util.BitSet;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
@@ -526,11 +527,19 @@ public class RexSimplify {
// may be unknown), because if either of them were true we would have
// stopped.
RexSimplify simplify = this;
+
+// 'doneTerms' prevents us from visiting a term in both first and second
+// loops. If we did this, the second visit would have a predicate saying
+// that 'term' is false. Effectively, we sort terms: visiting
+// 'allowedAsPredicate' terms in the first loop, and
+// non-'allowedAsPredicate' in the second. Each term is visited once.
+final BitSet doneTerms = new BitSet();
for (int i = 0; i < terms.size(); i++) {
final RexNode t = terms.get(i);
if (!simplify.allowedAsPredicateDuringOrSimplification(t)) {
continue;
}
+ doneTerms.set(i);
final RexNode t2 = simplify.simplify(t, unknownAs);
terms.set(i, t2);
final RexNode inverse =
@@ -542,8 +551,8 @@ public class RexSimplify {
}
for (int i = 0; i < terms.size(); i++) {
final RexNode t = terms.get(i);
- if (allowedAsPredicateDuringOrSimplification(t)) {
-continue;
+ if (doneTerms.get(i)) {
+continue; // we visited this term in the first loop
}
terms.set(i, simplify.simplify(t, unknownAs));
}
diff --git a/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
b/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
index dfd145c..92b6851 100644
--- a/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
+++ b/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
@@ -1562,6 +1562,48 @@ class RexProgramTest extends RexProgramTestBase {
"true");
}
+ @Test void testSimplifyRange() {
+final RexNode aRef = input(tInt(), 0);
+// ((0 < a and a <= 10) or a >= 15) and a <> 6 and a <> 12
+RexNode expr = and(
+or(
+and(lt(literal(0), aRef),
+le(aRef, literal(10))),
+ge(aRef, literal(15))),
+ne(aRef, literal(6)),
+ne(aRef, literal(12)));
+checkSimplifyUnchanged(expr);
+ }
+
+ @Test void testSimplifyRange2() {
+final RexNode aRef = input(tInt(true), 0);
+// a is null or a >= 15
+RexNode expr = or(isNull(aRef),
+ge(aRef, literal(15)));
+checkSimplifyUnchanged(expr);
+ }
+
+ /** Unit test for
+ * https://issues.apache.org/jira/browse/CALCITE-4190";>[CALCITE-4190]
+ * OR simplification incorrectly loses term. */
+ @Test void testSimplifyRange3() {
+final RexNode aRef = input(tInt(true), 0);
+// (0 < a and a <= 10) or a is null or (8 < a and a < 12) or a >= 15
+RexNode expr = or(
+and(lt(literal(0), aRef),
+le(aRef, literal(10))),
+isNull(aRef),
+and(lt(literal(8), aRef),
+lt(aRef, literal(12))),
+ge(aRef, literal(15)));
+// [CALCITE-4190] causes "or a >= 15" to disappear from the simplified
form.
+final String expected = "OR(IS NULL($0),"
++ " AND(<(0, $0), <=($0, 10)),"
++ " AND(<(8, $0), <($0, 12)),"
++ " >=($0, 15))";
+checkSimplify(expr, expected);
+ }
+
@Test void testSimplifyItemRangeTerms() {
RexNode item = item(input(tArray(tInt()), 3), literal(1));
// paranoid validation doesn't support array types, disable it for a moment
[GitHub] [calcite] liyafan82 opened a new pull request #2120: [CALCITE-4191] Improve the logic of creating aggregate calls
liyafan82 opened a new pull request #2120: URL: https://github.com/apache/calcite/pull/2120 According to the current code base, the only way to create AggregateCall objects is by calling one of the two AggregateCall#create methods (other create methods are deprecated). The two create methods have 9 and 11 parameters, respectively, 3 of which are booleans and 2 are ints. We find this makes the code less readable and error-prone, as some bugs are caused by specifying the wrong parameters. In this issue, we improve the related logic by the builder pattern, which results in the following benefits: 1. By creating the objects by the builder pattern, there is no need to maintain multiple overrides of the create methods. 2. There is no need to maintain multiple overrides of the copy methods, either. 3. The code becomes more readable and less error-prone, as it is less like to specify the wrong parameter. 4. Creating AggregateCall objects becomes easier, as the user does not have specify the default parameters repeatedly. Options 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
[GitHub] [calcite] danny0405 commented on a change in pull request #2110: [CALCITE-4177] Throw exception when deserialize SqlOperator fails, do not return null
danny0405 commented on a change in pull request #2110:
URL: https://github.com/apache/calcite/pull/2110#discussion_r476072339
##
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##
@@ -678,7 +678,8 @@ SqlOperator toOp(Map map) {
if (class_ != null) {
return AvaticaUtils.instantiatePlugin(SqlOperator.class, class_);
}
-return null;
+throw new RuntimeException("No operator for " + name + " with kind: "
++ kind + ", and syntax: " + syntax);
Review comment:
Is this a breaking change ? Do you know the background that it returns
null 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
[calcite] branch master updated: [CALCITE-4172] Expand columnar identifiers before resolving (James Starr)
This is an automated email from the ASF dual-hosted git repository.
danny0405 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 468b111 [CALCITE-4172] Expand columnar identifiers before resolving
(James Starr)
468b111 is described below
commit 468b111b3cae44efd31e60c4bafe0018c8821e9a
Author: James Starr
AuthorDate: Wed Aug 12 13:16:21 2020 -0700
[CALCITE-4172] Expand columnar identifiers before resolving (James Starr)
close apache/calcite#2108
---
.../calcite/sql/validate/SqlValidatorImpl.java | 2 +-
.../org/apache/calcite/test/SqlValidatorTest.java | 93 +-
2 files changed, 91 insertions(+), 4 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
index 9123f92..5782ee7 100644
--- a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
+++ b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
@@ -3997,7 +3997,6 @@ public class SqlValidatorImpl implements
SqlValidatorWithHints {
final String clause = "GROUP BY";
validateNoAggs(aggOrOverFinder, groupList, clause);
final SqlValidatorScope groupScope = getGroupScope(select);
-inferUnknownTypes(unknownType, groupScope, groupList);
// expand the expression in group list.
List expandedList = new ArrayList<>();
@@ -4007,6 +4006,7 @@ public class SqlValidatorImpl implements
SqlValidatorWithHints {
}
groupList = new SqlNodeList(expandedList, groupList.getParserPosition());
select.setGroupBy(groupList);
+inferUnknownTypes(unknownType, groupScope, groupList);
for (SqlNode groupItem : expandedList) {
validateGroupByItem(select, groupItem);
}
diff --git a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
index b9d7ed6..3623a18 100644
--- a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
+++ b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
@@ -25,31 +25,40 @@ import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.rel.type.RelDataTypeSystem;
import org.apache.calcite.runtime.CalciteContextException;
import org.apache.calcite.sql.SqlCollation;
+import org.apache.calcite.sql.SqlIdentifier;
import org.apache.calcite.sql.SqlNode;
import org.apache.calcite.sql.SqlOperator;
import org.apache.calcite.sql.SqlOperatorTable;
+import org.apache.calcite.sql.SqlSelect;
import org.apache.calcite.sql.SqlSpecialOperator;
import org.apache.calcite.sql.fun.SqlLibrary;
import org.apache.calcite.sql.fun.SqlLibraryOperatorTableFactory;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
import org.apache.calcite.sql.parser.SqlParseException;
import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.sql.test.SqlTestFactory;
+import org.apache.calcite.sql.test.SqlValidatorTester;
import org.apache.calcite.sql.type.ArraySqlType;
import org.apache.calcite.sql.type.SqlTypeFactoryImpl;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.sql.type.SqlTypeUtil;
+import org.apache.calcite.sql.util.SqlShuttle;
+import org.apache.calcite.sql.validate.SelectScope;
import org.apache.calcite.sql.validate.SqlAbstractConformance;
import org.apache.calcite.sql.validate.SqlConformance;
import org.apache.calcite.sql.validate.SqlConformanceEnum;
import org.apache.calcite.sql.validate.SqlDelegatingConformance;
import org.apache.calcite.sql.validate.SqlMonotonicity;
import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.sql.validate.SqlValidatorImpl;
+import org.apache.calcite.sql.validate.SqlValidatorScope;
import org.apache.calcite.sql.validate.SqlValidatorUtil;
import org.apache.calcite.test.catalog.CountingFactory;
import org.apache.calcite.testlib.annotations.LocaleEnUs;
import org.apache.calcite.util.Bug;
import org.apache.calcite.util.ImmutableBitSet;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Ordering;
@@ -62,7 +71,6 @@ import java.io.StringReader;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.charset.Charset;
-import java.util.Arrays;
import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
@@ -83,6 +91,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
+import static java.util.Arrays.asList;
+
/**
* Concrete child class of {@link SqlValidatorTestCase}, containing lots of
unit
* tests.
@@ -4082,7 +4092,7 @@ public class SqlValidatorTest extends
SqlValidatorTestCase {
[calcite] branch master updated: [CALCITE-4172] Expand columnar identifiers before resolving (James Starr)
This is an automated email from the ASF dual-hosted git repository.
danny0405 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 468b111 [CALCITE-4172] Expand columnar identifiers before resolving
(James Starr)
468b111 is described below
commit 468b111b3cae44efd31e60c4bafe0018c8821e9a
Author: James Starr
AuthorDate: Wed Aug 12 13:16:21 2020 -0700
[CALCITE-4172] Expand columnar identifiers before resolving (James Starr)
close apache/calcite#2108
---
.../calcite/sql/validate/SqlValidatorImpl.java | 2 +-
.../org/apache/calcite/test/SqlValidatorTest.java | 93 +-
2 files changed, 91 insertions(+), 4 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
index 9123f92..5782ee7 100644
--- a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
+++ b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
@@ -3997,7 +3997,6 @@ public class SqlValidatorImpl implements
SqlValidatorWithHints {
final String clause = "GROUP BY";
validateNoAggs(aggOrOverFinder, groupList, clause);
final SqlValidatorScope groupScope = getGroupScope(select);
-inferUnknownTypes(unknownType, groupScope, groupList);
// expand the expression in group list.
List expandedList = new ArrayList<>();
@@ -4007,6 +4006,7 @@ public class SqlValidatorImpl implements
SqlValidatorWithHints {
}
groupList = new SqlNodeList(expandedList, groupList.getParserPosition());
select.setGroupBy(groupList);
+inferUnknownTypes(unknownType, groupScope, groupList);
for (SqlNode groupItem : expandedList) {
validateGroupByItem(select, groupItem);
}
diff --git a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
index b9d7ed6..3623a18 100644
--- a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
+++ b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
@@ -25,31 +25,40 @@ import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.rel.type.RelDataTypeSystem;
import org.apache.calcite.runtime.CalciteContextException;
import org.apache.calcite.sql.SqlCollation;
+import org.apache.calcite.sql.SqlIdentifier;
import org.apache.calcite.sql.SqlNode;
import org.apache.calcite.sql.SqlOperator;
import org.apache.calcite.sql.SqlOperatorTable;
+import org.apache.calcite.sql.SqlSelect;
import org.apache.calcite.sql.SqlSpecialOperator;
import org.apache.calcite.sql.fun.SqlLibrary;
import org.apache.calcite.sql.fun.SqlLibraryOperatorTableFactory;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
import org.apache.calcite.sql.parser.SqlParseException;
import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.sql.test.SqlTestFactory;
+import org.apache.calcite.sql.test.SqlValidatorTester;
import org.apache.calcite.sql.type.ArraySqlType;
import org.apache.calcite.sql.type.SqlTypeFactoryImpl;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.sql.type.SqlTypeUtil;
+import org.apache.calcite.sql.util.SqlShuttle;
+import org.apache.calcite.sql.validate.SelectScope;
import org.apache.calcite.sql.validate.SqlAbstractConformance;
import org.apache.calcite.sql.validate.SqlConformance;
import org.apache.calcite.sql.validate.SqlConformanceEnum;
import org.apache.calcite.sql.validate.SqlDelegatingConformance;
import org.apache.calcite.sql.validate.SqlMonotonicity;
import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.sql.validate.SqlValidatorImpl;
+import org.apache.calcite.sql.validate.SqlValidatorScope;
import org.apache.calcite.sql.validate.SqlValidatorUtil;
import org.apache.calcite.test.catalog.CountingFactory;
import org.apache.calcite.testlib.annotations.LocaleEnUs;
import org.apache.calcite.util.Bug;
import org.apache.calcite.util.ImmutableBitSet;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Ordering;
@@ -62,7 +71,6 @@ import java.io.StringReader;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.charset.Charset;
-import java.util.Arrays;
import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
@@ -83,6 +91,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
+import static java.util.Arrays.asList;
+
/**
* Concrete child class of {@link SqlValidatorTestCase}, containing lots of
unit
* tests.
@@ -4082,7 +4092,7 @@ public class SqlValidatorTest extends
SqlValidatorTestCase {
[GitHub] [calcite] danny0405 closed pull request #2108: [CALCITE-4172] Expand columnar identifiers before resolution (James Starr)
danny0405 closed pull request #2108: URL: https://github.com/apache/calcite/pull/2108 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
[GitHub] [calcite] danny0405 commented on pull request #2108: [CALCITE-4172] Expand columnar identifiers before resolution (James Starr)
danny0405 commented on pull request #2108: URL: https://github.com/apache/calcite/pull/2108#issuecomment-679453129 > If there are no objections, could I get this merged. I think we could, would merge it soon ~ This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] jamesstarr commented on pull request #2108: [CALCITE-4172] Expand columnar identifiers before resolution (James Starr)
jamesstarr commented on pull request #2108: URL: https://github.com/apache/calcite/pull/2108#issuecomment-679407490 If there are no objections, could I get this merged. 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
[GitHub] [calcite] julianhyde commented on pull request #2119: [CALCITE-4183] FilterSetOpTransposeRule constructor should allow for …
julianhyde commented on pull request #2119: URL: https://github.com/apache/calcite/pull/2119#issuecomment-679361883 I am not sure we need this. I am not denying that the fix works. But something similar could be accomplished in client code without any changes to Calcite. Without the `withOperandFor` method the test case would be 1 or 2 lines longer - not nothing, is it worth it? I'm worried that people will thing that `withOperandFor` methods are the ONLY way to customize rules. And post CALCITE-3923, they're syntactic sugar but not necessary. 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
[GitHub] [calcite] amaliujia commented on a change in pull request #2116: [CALCITE-4188] support EnumerableBatchNestedLoopJoin inJdbcToEnumerab…
amaliujia commented on a change in pull request #2116:
URL: https://github.com/apache/calcite/pull/2116#discussion_r475832464
##
File path:
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java
##
@@ -195,7 +235,14 @@ public Result implement(EnumerableRelImplementor
implementor, Prefer pref) {
}
private List toIndexesTableExpression(SqlString
sqlString) {
-return sqlString.getDynamicParameters().stream()
+return
Optional.ofNullable(sqlString.getDynamicParameters()).orElse(ImmutableList.of()).stream()
+.map(Expressions::constant)
+.collect(Collectors.toList());
+ }
+
+ private List toIndexCorrelateExpression(SqlString
sqlString,
+ StringBuilder querySqlSb, SqlDialect sqlDialect) {
+return sqlDialect.getDynamicTypeIndexs(querySqlSb,
sqlString.getSql()).stream()
Review comment:
I am not sure whether `getDynamicTypeIndexs` should be in SqlDialect.
Is this the format of SQL that this function recognizes defined in SQL
standard? What kind of vendors support it? Is it a common used syntax?
##
File path:
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java
##
@@ -78,7 +89,7 @@ protected JdbcToEnumerableConverter(
}
@Override public RelOptCost computeSelfCost(RelOptPlanner planner,
- RelMetadataQuery mq) {
+ RelMetadataQuery mq) {
return super.computeSelfCost(planner, mq).multiplyBy(.1);
Review comment:
fix indents
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
[GitHub] [calcite] amaliujia commented on a change in pull request #2006: [CALCITE-4015] Pass through parent collation request on subset or sup…
amaliujia commented on a change in pull request #2006:
URL: https://github.com/apache/calcite/pull/2006#discussion_r475829864
##
File path: core/src/test/java/org/apache/calcite/rel/RelCollationTest.java
##
@@ -84,18 +84,36 @@
is(true));
}
- /** Unit test for {@link RelCollations#containsOrderless(List, List)}. */
+ /** Unit test for {@link RelCollations#collationsContainKeysOrderless(List,
List)}. */
Review comment:
Makes sense. Added the 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
[GitHub] [calcite] sbroeder opened a new pull request #2119: [CALCITE-4183] FilterSetOpTransposeRule constructor should allow for …
sbroeder opened a new pull request #2119: URL: https://github.com/apache/calcite/pull/2119 …user defined Filter and SetOp classes Added withOperandFor method to FilterSetOpTranspose.java Added new testFilterSetOpTranspose to RelOptRulesTest to utilize custome Filter and SetOp classes. 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
[GitHub] [calcite] amaliujia commented on pull request #2006: [CALCITE-4015] Pass through parent collation request on subset or sup…
amaliujia commented on pull request #2006: URL: https://github.com/apache/calcite/pull/2006#issuecomment-679262806 Ok I should have addressed existing comments. Thanks @rubenada for the review! @hsyuan do you want to take a final look? 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
[GitHub] [calcite] amaliujia commented on a change in pull request #2006: [CALCITE-4015] Pass through parent collation request on subset or sup…
amaliujia commented on a change in pull request #2006:
URL: https://github.com/apache/calcite/pull/2006#discussion_r475767057
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableMergeJoin.java
##
@@ -222,6 +277,30 @@ public static boolean isMergeJoinSupported(JoinRelType
joinType) {
return mapping;
}
+ private RelCollation extendCollation(RelCollation collation, List
keys) {
+List fieldsForNewCollation = new
ArrayList<>(keys.size());
+fieldsForNewCollation.addAll(collation.getFieldCollations());
+Set keySet = new HashSet<>(keys);
+for (RelFieldCollation rf : collation.getFieldCollations()) {
+ keySet.remove(rf.getFieldIndex());
+}
+for (Integer i : keySet) {
+ fieldsForNewCollation.add(new RelFieldCollation(i));
+}
+return RelCollations.of(fieldsForNewCollation);
+ }
+
+ private RelCollation removeCollationFieldsNotOnJoinKey(
Review comment:
`intersectCollationAndJoinKey` is a reasonable name for me.
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
[GitHub] [calcite] amaliujia commented on a change in pull request #2006: [CALCITE-4015] Pass through parent collation request on subset or sup…
amaliujia commented on a change in pull request #2006:
URL: https://github.com/apache/calcite/pull/2006#discussion_r475767057
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableMergeJoin.java
##
@@ -222,6 +277,30 @@ public static boolean isMergeJoinSupported(JoinRelType
joinType) {
return mapping;
}
+ private RelCollation extendCollation(RelCollation collation, List
keys) {
+List fieldsForNewCollation = new
ArrayList<>(keys.size());
+fieldsForNewCollation.addAll(collation.getFieldCollations());
+Set keySet = new HashSet<>(keys);
+for (RelFieldCollation rf : collation.getFieldCollations()) {
+ keySet.remove(rf.getFieldIndex());
+}
+for (Integer i : keySet) {
+ fieldsForNewCollation.add(new RelFieldCollation(i));
+}
+return RelCollations.of(fieldsForNewCollation);
+ }
+
+ private RelCollation removeCollationFieldsNotOnJoinKey(
Review comment:
`intersectCollation AndJoinKey` is a reasonable name for me.
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
[GitHub] [calcite] amaliujia commented on a change in pull request #2006: [CALCITE-4015] Pass through parent collation request on subset or sup…
amaliujia commented on a change in pull request #2006:
URL: https://github.com/apache/calcite/pull/2006#discussion_r475767057
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableMergeJoin.java
##
@@ -222,6 +277,30 @@ public static boolean isMergeJoinSupported(JoinRelType
joinType) {
return mapping;
}
+ private RelCollation extendCollation(RelCollation collation, List
keys) {
+List fieldsForNewCollation = new
ArrayList<>(keys.size());
+fieldsForNewCollation.addAll(collation.getFieldCollations());
+Set keySet = new HashSet<>(keys);
+for (RelFieldCollation rf : collation.getFieldCollations()) {
+ keySet.remove(rf.getFieldIndex());
+}
+for (Integer i : keySet) {
+ fieldsForNewCollation.add(new RelFieldCollation(i));
+}
+return RelCollations.of(fieldsForNewCollation);
+ }
+
+ private RelCollation removeCollationFieldsNotOnJoinKey(
Review comment:
`intersectCollecationAndJoinKey` is a reasonable name for me.
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
[GitHub] [calcite] amaliujia commented on a change in pull request #2006: [CALCITE-4015] Pass through parent collation request on subset or sup…
amaliujia commented on a change in pull request #2006:
URL: https://github.com/apache/calcite/pull/2006#discussion_r475759626
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableMergeJoin.java
##
@@ -222,6 +277,30 @@ public static boolean isMergeJoinSupported(JoinRelType
joinType) {
return mapping;
}
+ private RelCollation extendCollation(RelCollation collation, List
keys) {
+List fieldsForNewCollation = new
ArrayList<>(keys.size());
+fieldsForNewCollation.addAll(collation.getFieldCollations());
+Set keySet = new HashSet<>(keys);
+for (RelFieldCollation rf : collation.getFieldCollations()) {
+ keySet.remove(rf.getFieldIndex());
+}
+for (Integer i : keySet) {
+ fieldsForNewCollation.add(new RelFieldCollation(i));
+}
+return RelCollations.of(fieldsForNewCollation);
+ }
+
+ private RelCollation removeCollationFieldsNotOnJoinKey(
Review comment:
oops. I have missed this comment.
Will address this one and another comment.
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
[calcite] branch master updated: [CALCITE-4113] Support LEFT join in EnumerableMergeJoin
This is an automated email from the ASF dual-hosted git repository.
rubenql 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 672ed7a [CALCITE-4113] Support LEFT join in EnumerableMergeJoin
672ed7a is described below
commit 672ed7a1d0dbf87760d37e52b424f16bc8c43b4d
Author: rubenada
AuthorDate: Wed Aug 12 17:14:57 2020 +0100
[CALCITE-4113] Support LEFT join in EnumerableMergeJoin
---
.../apache/calcite/runtime/EnumerablesTest.java| 188 -
.../java/org/apache/calcite/test/JdbcTest.java | 12 +-
.../test/enumerable/EnumerableCorrelateTest.java | 1 +
.../test/enumerable/EnumerableHashJoinTest.java| 4 +
core/src/test/resources/sql/blank.iq | 24 +--
core/src/test/resources/sql/misc.iq| 36 ++--
core/src/test/resources/sql/sub-query.iq | 58 ---
.../apache/calcite/linq4j/EnumerableDefaults.java | 93 +-
8 files changed, 317 insertions(+), 99 deletions(-)
diff --git a/core/src/test/java/org/apache/calcite/runtime/EnumerablesTest.java
b/core/src/test/java/org/apache/calcite/runtime/EnumerablesTest.java
index 20ed84a..f615d92 100644
--- a/core/src/test/java/org/apache/calcite/runtime/EnumerablesTest.java
+++ b/core/src/test/java/org/apache/calcite/runtime/EnumerablesTest.java
@@ -212,6 +212,43 @@ class EnumerablesTest {
newArrayList(1, 1, 4, 4),
equalTo("[3]"),
JoinType.ANTI);
+
+// LEFT join tests:
+// Matching keys at start
+testIntersect(
+newArrayList(1, 3, 4),
+newArrayList(1, 4),
+equalTo("[1-1, 3-null, 4-4]"),
+equalTo("[1-1, 3-null, 4-4, null-null]"),
+JoinType.LEFT);
+// Matching key at start and end of right, not of left
+testIntersect(
+newArrayList(0, 1, 3, 4, 5),
+newArrayList(1, 4),
+equalTo("[0-null, 1-1, 3-null, 4-4, 5-null]"),
+equalTo("[0-null, 1-1, 3-null, 4-4, 5-null, null-null]"),
+JoinType.LEFT);
+// Matching key at start and end of left, not right
+testIntersect(
+newArrayList(1, 3, 4),
+newArrayList(0, 1, 4, 5),
+equalTo("[1-1, 3-null, 4-4]"),
+equalTo("[1-1, 3-null, 4-4, null-null]"),
+JoinType.LEFT);
+// Matching key not at start or end of left or right
+testIntersect(
+newArrayList(0, 2, 3, 4, 5),
+newArrayList(1, 3, 4, 6),
+equalTo("[0-null, 2-null, 3-3, 4-4, 5-null]"),
+equalTo("[0-null, 2-null, 3-3, 4-4, 5-null, null-null]"),
+JoinType.LEFT);
+// Matching duplicated keys
+testIntersect(
+newArrayList(1, 3, 4),
+newArrayList(1, 1, 4, 4),
+equalTo("[1-1, 1-1, 3-null, 4-4, 4-4]"),
+equalTo("[1-1, 1-1, 3-null, 4-4, 4-4, null-null]"),
+JoinType.LEFT);
}
@Test void testMergeJoin3() {
@@ -268,21 +305,57 @@ class EnumerablesTest {
new ArrayList<>(),
equalTo("[]"),
JoinType.ANTI);
+
+// LEFT join tests:
+// No overlap
+testIntersect(
+newArrayList(0, 2, 4),
+newArrayList(1, 3, 5),
+equalTo("[0-null, 2-null, 4-null]"),
+equalTo("[0-null, 2-null, 4-null, null-null]"),
+JoinType.LEFT);
+// Left empty
+testIntersect(
+new ArrayList<>(),
+newArrayList(1, 3, 4, 6),
+equalTo("[]"),
+equalTo("[null-null]"),
+JoinType.LEFT);
+// Right empty
+testIntersect(
+newArrayList(3, 7),
+new ArrayList<>(),
+equalTo("[3-null, 7-null]"),
+equalTo("[3-null, 7-null, null-null]"),
+JoinType.LEFT);
+// Both empty
+testIntersect(
+new ArrayList(),
+new ArrayList<>(),
+equalTo("[]"),
+equalTo("[null-null]"),
+JoinType.LEFT);
}
private static > void testIntersect(
List list0, List list1, org.hamcrest.Matcher matcher,
JoinType joinType) {
+testIntersect(list0, list1, matcher, matcher, joinType);
+ }
+
+ private static > void testIntersect(
+ List list0, List list1, org.hamcrest.Matcher matcher,
+ org.hamcrest.Matcher matcherNullLeft, JoinType joinType) {
assertThat(
intersect(list0, list1, joinType).toList().toString(),
matcher);
-// Repeat test with nulls at the end of left / right: result should not be
impacted
+// Repeat test with nulls at the end of left / right
// Null at the end of left
list0.add(null);
assertThat(
intersect(list0, list1, joinType).toList().toString(),
-matcher);
+matcherNullLeft);
// Null at the end of right
list0.remove(list0.size() - 1);
@@ -295,17 +368,27 @@ class EnumerablesTest {
list0.add(null);
assertThat(
intersect(list0, list1, joinType).toList().toString(),
-matcher);
+matcherNullLeft);
}
- private static > E
[GitHub] [calcite] rubenada merged pull request #2107: [CALCITE-4113] Support LEFT join in EnumerableMergeJoin
rubenada merged pull request #2107: URL: https://github.com/apache/calcite/pull/2107 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
[GitHub] [calcite] rubenada commented on pull request #2006: [CALCITE-4015] Pass through parent collation request on subset or sup…
rubenada commented on pull request #2006: URL: https://github.com/apache/calcite/pull/2006#issuecomment-679008064 @amaliujia thanks for your work. There are some remaining comments about some minor details, but overall PR LGTM @hsyuan do you want to take a final look? 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
[GitHub] [calcite] rubenada commented on a change in pull request #2006: [CALCITE-4015] Pass through parent collation request on subset or sup…
rubenada commented on a change in pull request #2006:
URL: https://github.com/apache/calcite/pull/2006#discussion_r475449330
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableMergeJoin.java
##
@@ -222,6 +277,30 @@ public static boolean isMergeJoinSupported(JoinRelType
joinType) {
return mapping;
}
+ private RelCollation extendCollation(RelCollation collation, List
keys) {
+List fieldsForNewCollation = new
ArrayList<>(keys.size());
+fieldsForNewCollation.addAll(collation.getFieldCollations());
+Set keySet = new HashSet<>(keys);
+for (RelFieldCollation rf : collation.getFieldCollations()) {
+ keySet.remove(rf.getFieldIndex());
+}
+for (Integer i : keySet) {
+ fieldsForNewCollation.add(new RelFieldCollation(i));
+}
+return RelCollations.of(fieldsForNewCollation);
+ }
+
+ private RelCollation removeCollationFieldsNotOnJoinKey(
Review comment:
friendly reminder about these comments
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2006: [CALCITE-4015] Pass through parent collation request on subset or sup…
rubenada commented on a change in pull request #2006:
URL: https://github.com/apache/calcite/pull/2006#discussion_r475448937
##
File path: core/src/test/java/org/apache/calcite/rel/RelCollationTest.java
##
@@ -84,18 +84,36 @@
is(true));
}
- /** Unit test for {@link RelCollations#containsOrderless(List, List)}. */
+ /** Unit test for {@link RelCollations#collationsContainKeysOrderless(List,
List)}. */
Review comment:
maybe we could also add some unit tests for the new method
`keysContainCollationsOrderless`?
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
[GitHub] [calcite] rubenada closed pull request #2107: [CALCITE-4113] Support LEFT join in EnumerableMergeJoin
rubenada closed pull request #2107: URL: https://github.com/apache/calcite/pull/2107 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
