[GitHub] [calcite] danny0405 merged pull request #2257: Following 4364, fix the plann diff of TpcdsTest
danny0405 merged pull request #2257: URL: https://github.com/apache/calcite/pull/2257 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: Following 4364, fix the plann diff of TpcdsTest
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 c8b5622 Following 4364, fix the plann diff of TpcdsTest
c8b5622 is described below
commit c8b5622afc86294996ebec1ac323b4cbd3d478dc
Author: yuzhao.cyz
AuthorDate: Fri Nov 13 10:49:57 2020 +0800
Following 4364, fix the plann diff of TpcdsTest
---
.../src/test/java/org/apache/calcite/adapter/tpcds/TpcdsTest.java | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/plus/src/test/java/org/apache/calcite/adapter/tpcds/TpcdsTest.java
b/plus/src/test/java/org/apache/calcite/adapter/tpcds/TpcdsTest.java
index f0a868e..927167a 100644
--- a/plus/src/test/java/org/apache/calcite/adapter/tpcds/TpcdsTest.java
+++ b/plus/src/test/java/org/apache/calcite/adapter/tpcds/TpcdsTest.java
@@ -384,11 +384,9 @@ class TpcdsTest {
+ "LogicalSort(sort0=[$1], sort1=[$0], dir0=[ASC], dir1=[ASC],
fetch=[100])\n"
+ " LogicalAggregate(group=[{84, 90}], AGG1=[AVG($10)],
AGG2=[AVG($12)], AGG3=[AVG($19)], AGG4=[AVG($13)])\n"
+ "LogicalFilter(condition=[AND(=($0, $32), =($2, $89), "
-+ "=($7, $60), =($4, $23), SEARCH($24, Sarg['M']:CHAR(1)), "
-+ "SEARCH($25, Sarg['S']:CHAR(1)), "
-+ "SEARCH($26, Sarg['HIGH SCHOOL']:CHAR(11)), "
-+ "SEARCH($38, Sarg[1998]), SEARCH($84, Sarg['CA', 'MD', 'OK', 'OR', "
-+ "'TX', 'WA']:CHAR(2)))])\n"
++ "=($7, $60), =($4, $23), =($24, 'M'), "
++ "=($25, 'S'), =($26, 'HIGH SCHOOL'), =($38, 1998), "
++ "SEARCH($84, Sarg['CA', 'MD', 'OK', 'OR', 'TX', 'WA']:CHAR(2)))])\n"
+ " LogicalJoin(condition=[true], joinType=[inner])\n"
+ "LogicalTableScan(table=[[TPCDS, STORE_SALES]])\n"
+ "LogicalJoin(condition=[true], joinType=[inner])\n"
[calcite] branch master updated (0ce7685 -> ff4c16d)
This is an automated email from the ASF dual-hosted git repository. jhyde pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git. from 0ce7685 [CALCITE-4390] SqlMatchRecognize returns wrong operand list (Dawid Wysakowicz) add 6f94db0 [CALCITE-4380] Make class SqlNodeList implement List add 54a7bfa [CALCITE-4389] Calls to ROW and anonymous row operators sometimes print too many spaces add e819b46 [CALCITE-4394] When generating code for a function call, take the inferred types of the operands into account new ff4c16d [CALCITE-4383] In RelBuilder, optimize 'VALUES ... UNION ALL ... VALUES' to a single 'VALUES' with multiple rows The 1 revisions listed above as "new" are entirely new to this repository and will be described in separate emails. The revisions listed as "add" were already present in the repository and have only been added to this reference. Summary of changes: core/src/main/codegen/templates/Parser.jj | 10 +- .../calcite/adapter/enumerable/EnumUtils.java | 19 +-- .../calcite/adapter/enumerable/RexImpTable.java| 32 ++-- .../calcite/rel/rel2sql/RelToSqlConverter.java | 16 +- .../org/apache/calcite/runtime/SqlFunctions.java | 4 +- .../org/apache/calcite/sql/SqlCallBinding.java | 5 +- .../java/org/apache/calcite/sql/SqlDialect.java| 28 ++-- .../main/java/org/apache/calcite/sql/SqlHint.java | 12 +- .../java/org/apache/calcite/sql/SqlIdentifier.java | 12 ++ .../main/java/org/apache/calcite/sql/SqlKind.java | 3 + .../main/java/org/apache/calcite/sql/SqlNode.java | 6 +- .../java/org/apache/calcite/sql/SqlNodeList.java | 178 +++- .../java/org/apache/calcite/sql/SqlOperator.java | 70 ++-- .../main/java/org/apache/calcite/sql/SqlPivot.java | 4 +- .../apache/calcite/sql/SqlWindowTableFunction.java | 7 +- .../apache/calcite/sql/ddl/SqlCreateFunction.java | 2 +- .../java/org/apache/calcite/sql/fun/SqlCase.java | 7 +- .../apache/calcite/sql/fun/SqlCaseOperator.java| 3 +- .../calcite/sql/fun/SqlInternalOperators.java | 37 + .../calcite/sql/fun/SqlJsonObjectFunction.java | 25 ++- .../calcite/sql/fun/SqlLibraryOperators.java | 12 +- .../org/apache/calcite/sql/fun/SqlRowOperator.java | 1 - .../calcite/sql/fun/SqlStdOperatorTable.java | 2 + .../java/org/apache/calcite/sql/parser/Span.java | 8 + .../apache/calcite/sql/parser/SqlParserUtil.java | 2 +- .../sql/type/CompositeOperandTypeChecker.java | 18 +++ .../calcite/sql/type/SqlOperandTypeChecker.java| 8 + .../org/apache/calcite/sql/util/SqlShuttle.java| 8 +- .../org/apache/calcite/sql/validate/AggFinder.java | 7 + .../calcite/sql/validate/AliasNamespace.java | 4 +- .../apache/calcite/sql/validate/SqlValidator.java | 15 ++ .../calcite/sql/validate/SqlValidatorImpl.java | 74 + .../calcite/sql/validate/SqlValidatorUtil.java | 5 +- .../calcite/sql/validate/TableNamespace.java | 4 +- .../calcite/sql/validate/WithItemNamespace.java| 8 +- .../apache/calcite/sql/validate/WithNamespace.java | 2 +- .../validate/implicit/AbstractTypeCoercion.java| 2 +- .../apache/calcite/sql2rel/SqlToRelConverter.java | 84 -- .../calcite/sql2rel/StandardConvertletTable.java | 81 +- .../java/org/apache/calcite/tools/RelBuilder.java | 104 +++- .../org/apache/calcite/util/BuiltInMethod.java | 2 +- .../calcite/adapter/enumerable/EnumUtilsTest.java | 22 +-- .../rel/rel2sql/RelToSqlConverterStructsTest.java | 3 +- .../calcite/rel/rel2sql/RelToSqlConverterTest.java | 180 ++--- .../java/org/apache/calcite/sql/SqlNodeTest.java | 76 + .../apache/calcite/sql/parser/SqlParserTest.java | 2 +- .../parserextensiontesting/SqlCreateTable.java | 4 +- .../calcite/sql/test/SqlOperatorBaseTest.java | 56 --- .../apache/calcite/sql/type/SqlTypeUtilTest.java | 6 +- .../org/apache/calcite/test/JdbcAdapterTest.java | 40 ++--- .../java/org/apache/calcite/test/JdbcTest.java | 17 +- .../org/apache/calcite/test/RelBuilderTest.java| 80 - .../org/apache/calcite/test/RelOptRulesTest.java | 5 +- .../org/apache/calcite/test/RelOptRulesTest.xml| 10 +- .../apache/calcite/test/SqlToRelConverterTest.xml | 42 ++--- .../calcite/test/TypeCoercionConverterTest.xml | 167 ++- core/src/test/resources/sql/agg.iq | 85 +- core/src/test/resources/sql/join.iq| 40 + core/src/test/resources/sql/misc.iq| 33 ++-- core/src/test/resources/sql/outer.iq | 20 +-- core/src/test/resources/sql/sub-query.iq | 25 +-- core/src/test/resources/sql/winagg.iq | 20 +-- .../calcite/piglet/PigRelToSqlConverter.java | 2 +- .../java/org/apache/calcite/test/PigRelOpTest.java |
[calcite] 01/01: [CALCITE-4383] In RelBuilder, optimize 'VALUES ... UNION ALL ... VALUES' to a single 'VALUES' with multiple rows
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
commit ff4c16d1ea2192435e543fc9572ae3a44decbf79
Author: Julian Hyde
AuthorDate: Fri Nov 6 23:27:52 2020 -0800
[CALCITE-4383] In RelBuilder, optimize 'VALUES ... UNION ALL ... VALUES' to
a single 'VALUES' with multiple rows
---
.../apache/calcite/sql2rel/SqlToRelConverter.java | 21 +--
.../java/org/apache/calcite/tools/RelBuilder.java | 66 +++-
.../rel/rel2sql/RelToSqlConverterStructsTest.java | 3 +-
.../calcite/rel/rel2sql/RelToSqlConverterTest.java | 109 +-
.../org/apache/calcite/test/JdbcAdapterTest.java | 41 +++--
.../java/org/apache/calcite/test/JdbcTest.java | 17 ++-
.../org/apache/calcite/test/RelBuilderTest.java| 61 +++-
.../org/apache/calcite/test/RelOptRulesTest.java | 5 +-
.../org/apache/calcite/test/RelOptRulesTest.xml| 10 +-
.../apache/calcite/test/SqlToRelConverterTest.xml | 42 +++---
.../calcite/test/TypeCoercionConverterTest.xml | 167 ++---
core/src/test/resources/sql/agg.iq | 85 +--
core/src/test/resources/sql/join.iq| 40 +
core/src/test/resources/sql/misc.iq| 33 ++--
core/src/test/resources/sql/outer.iq | 20 +--
core/src/test/resources/sql/sub-query.iq | 25 +--
core/src/test/resources/sql/winagg.iq | 20 +--
17 files changed, 364 insertions(+), 401 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
index 187ff17..3a39960 100644
--- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
+++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
@@ -4330,7 +4330,6 @@ public class SqlToRelConverter {
return;
}
-final List unionRels = new ArrayList<>();
for (SqlNode rowConstructor1 : values.getOperandList()) {
SqlCall rowConstructor = (SqlCall) rowConstructor1;
Blackboard tmpBb = createBlackboard(bb.scope, null, false);
@@ -4347,22 +4346,14 @@ public class SqlToRelConverter {
(null == tmpBb.root)
? LogicalValues.createOneRow(cluster)
: tmpBb.root;
- unionRels.add(relBuilder.push(in)
- .project(Pair.left(exps), Pair.right(exps))
- .build());
+ relBuilder.push(in)
+ .project(Pair.left(exps), Pair.right(exps));
}
-if (unionRels.size() == 0) {
- throw new AssertionError("empty values clause");
-} else if (unionRels.size() == 1) {
- bb.setRoot(
- unionRels.get(0),
- true);
-} else {
- bb.setRoot(
- LogicalUnion.create(unionRels, true),
- true);
-}
+bb.setRoot(
+relBuilder.union(true, values.getOperandList().size())
+.build(),
+true);
}
//~ Inner Classes --
diff --git a/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
b/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
index 759b46c..24ac3be 100644
--- a/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
+++ b/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
@@ -131,6 +131,7 @@ import java.util.function.UnaryOperator;
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
+import static org.apache.calcite.sql.SqlKind.UNION;
import static org.apache.calcite.util.Static.RESOURCE;
/**
@@ -1561,6 +1562,22 @@ public class RelBuilder {
}
return this;
}
+
+// If the expressions are all literals, and the input is a Values with N
+// rows, replace with a Values with same tuple N times.
+if (config.simplifyValues()
+&& frame.rel instanceof Values
+&& nodeList.stream().allMatch(e -> e instanceof RexLiteral)) {
+ final Values values = (Values) build();
+ final RelDataTypeFactory.Builder typeBuilder =
getTypeFactory().builder();
+ Pair.forEach(fieldNameList, nodeList, (name, expr) ->
+ typeBuilder.add(name, expr.getType()));
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ final List tuple = (List) (List) nodeList;
+ return values(Collections.nCopies(values.tuples.size(), tuple),
+ typeBuilder.build());
+}
+
final RelNode project =
struct.projectFactory.createProject(frame.rel,
ImmutableList.copyOf(hints),
@@ -1610,6 +1627,20 @@ public class RelBuilder {
childProject.getInput(), childProject.getProjects(), rowType);
stack.push(new Frame(newInput.attachHints(childProject.getHints()),
frame.fields));
}
+ if (input instanceof Values && fieldNames != null) {
+// Rename columns of child values if desired field names are given.
+final Frame f
[calcite] 01/01: [CALCITE-4383] In RelBuilder, optimize 'VALUES ... UNION ALL ... VALUES' to a single 'VALUES' with multiple rows
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
commit ff4c16d1ea2192435e543fc9572ae3a44decbf79
Author: Julian Hyde
AuthorDate: Fri Nov 6 23:27:52 2020 -0800
[CALCITE-4383] In RelBuilder, optimize 'VALUES ... UNION ALL ... VALUES' to
a single 'VALUES' with multiple rows
---
.../apache/calcite/sql2rel/SqlToRelConverter.java | 21 +--
.../java/org/apache/calcite/tools/RelBuilder.java | 66 +++-
.../rel/rel2sql/RelToSqlConverterStructsTest.java | 3 +-
.../calcite/rel/rel2sql/RelToSqlConverterTest.java | 109 +-
.../org/apache/calcite/test/JdbcAdapterTest.java | 41 +++--
.../java/org/apache/calcite/test/JdbcTest.java | 17 ++-
.../org/apache/calcite/test/RelBuilderTest.java| 61 +++-
.../org/apache/calcite/test/RelOptRulesTest.java | 5 +-
.../org/apache/calcite/test/RelOptRulesTest.xml| 10 +-
.../apache/calcite/test/SqlToRelConverterTest.xml | 42 +++---
.../calcite/test/TypeCoercionConverterTest.xml | 167 ++---
core/src/test/resources/sql/agg.iq | 85 +--
core/src/test/resources/sql/join.iq| 40 +
core/src/test/resources/sql/misc.iq| 33 ++--
core/src/test/resources/sql/outer.iq | 20 +--
core/src/test/resources/sql/sub-query.iq | 25 +--
core/src/test/resources/sql/winagg.iq | 20 +--
17 files changed, 364 insertions(+), 401 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
index 187ff17..3a39960 100644
--- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
+++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
@@ -4330,7 +4330,6 @@ public class SqlToRelConverter {
return;
}
-final List unionRels = new ArrayList<>();
for (SqlNode rowConstructor1 : values.getOperandList()) {
SqlCall rowConstructor = (SqlCall) rowConstructor1;
Blackboard tmpBb = createBlackboard(bb.scope, null, false);
@@ -4347,22 +4346,14 @@ public class SqlToRelConverter {
(null == tmpBb.root)
? LogicalValues.createOneRow(cluster)
: tmpBb.root;
- unionRels.add(relBuilder.push(in)
- .project(Pair.left(exps), Pair.right(exps))
- .build());
+ relBuilder.push(in)
+ .project(Pair.left(exps), Pair.right(exps));
}
-if (unionRels.size() == 0) {
- throw new AssertionError("empty values clause");
-} else if (unionRels.size() == 1) {
- bb.setRoot(
- unionRels.get(0),
- true);
-} else {
- bb.setRoot(
- LogicalUnion.create(unionRels, true),
- true);
-}
+bb.setRoot(
+relBuilder.union(true, values.getOperandList().size())
+.build(),
+true);
}
//~ Inner Classes --
diff --git a/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
b/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
index 759b46c..24ac3be 100644
--- a/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
+++ b/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
@@ -131,6 +131,7 @@ import java.util.function.UnaryOperator;
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
+import static org.apache.calcite.sql.SqlKind.UNION;
import static org.apache.calcite.util.Static.RESOURCE;
/**
@@ -1561,6 +1562,22 @@ public class RelBuilder {
}
return this;
}
+
+// If the expressions are all literals, and the input is a Values with N
+// rows, replace with a Values with same tuple N times.
+if (config.simplifyValues()
+&& frame.rel instanceof Values
+&& nodeList.stream().allMatch(e -> e instanceof RexLiteral)) {
+ final Values values = (Values) build();
+ final RelDataTypeFactory.Builder typeBuilder =
getTypeFactory().builder();
+ Pair.forEach(fieldNameList, nodeList, (name, expr) ->
+ typeBuilder.add(name, expr.getType()));
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ final List tuple = (List) (List) nodeList;
+ return values(Collections.nCopies(values.tuples.size(), tuple),
+ typeBuilder.build());
+}
+
final RelNode project =
struct.projectFactory.createProject(frame.rel,
ImmutableList.copyOf(hints),
@@ -1610,6 +1627,20 @@ public class RelBuilder {
childProject.getInput(), childProject.getProjects(), rowType);
stack.push(new Frame(newInput.attachHints(childProject.getHints()),
frame.fields));
}
+ if (input instanceof Values && fieldNames != null) {
+// Rename columns of child values if desired field names are given.
+final Frame f
[calcite] branch master updated (0ce7685 -> ff4c16d)
This is an automated email from the ASF dual-hosted git repository. jhyde pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git. from 0ce7685 [CALCITE-4390] SqlMatchRecognize returns wrong operand list (Dawid Wysakowicz) add 6f94db0 [CALCITE-4380] Make class SqlNodeList implement List add 54a7bfa [CALCITE-4389] Calls to ROW and anonymous row operators sometimes print too many spaces add e819b46 [CALCITE-4394] When generating code for a function call, take the inferred types of the operands into account new ff4c16d [CALCITE-4383] In RelBuilder, optimize 'VALUES ... UNION ALL ... VALUES' to a single 'VALUES' with multiple rows The 1 revisions listed above as "new" are entirely new to this repository and will be described in separate emails. The revisions listed as "add" were already present in the repository and have only been added to this reference. Summary of changes: core/src/main/codegen/templates/Parser.jj | 10 +- .../calcite/adapter/enumerable/EnumUtils.java | 19 +-- .../calcite/adapter/enumerable/RexImpTable.java| 32 ++-- .../calcite/rel/rel2sql/RelToSqlConverter.java | 16 +- .../org/apache/calcite/runtime/SqlFunctions.java | 4 +- .../org/apache/calcite/sql/SqlCallBinding.java | 5 +- .../java/org/apache/calcite/sql/SqlDialect.java| 28 ++-- .../main/java/org/apache/calcite/sql/SqlHint.java | 12 +- .../java/org/apache/calcite/sql/SqlIdentifier.java | 12 ++ .../main/java/org/apache/calcite/sql/SqlKind.java | 3 + .../main/java/org/apache/calcite/sql/SqlNode.java | 6 +- .../java/org/apache/calcite/sql/SqlNodeList.java | 178 +++- .../java/org/apache/calcite/sql/SqlOperator.java | 70 ++-- .../main/java/org/apache/calcite/sql/SqlPivot.java | 4 +- .../apache/calcite/sql/SqlWindowTableFunction.java | 7 +- .../apache/calcite/sql/ddl/SqlCreateFunction.java | 2 +- .../java/org/apache/calcite/sql/fun/SqlCase.java | 7 +- .../apache/calcite/sql/fun/SqlCaseOperator.java| 3 +- .../calcite/sql/fun/SqlInternalOperators.java | 37 + .../calcite/sql/fun/SqlJsonObjectFunction.java | 25 ++- .../calcite/sql/fun/SqlLibraryOperators.java | 12 +- .../org/apache/calcite/sql/fun/SqlRowOperator.java | 1 - .../calcite/sql/fun/SqlStdOperatorTable.java | 2 + .../java/org/apache/calcite/sql/parser/Span.java | 8 + .../apache/calcite/sql/parser/SqlParserUtil.java | 2 +- .../sql/type/CompositeOperandTypeChecker.java | 18 +++ .../calcite/sql/type/SqlOperandTypeChecker.java| 8 + .../org/apache/calcite/sql/util/SqlShuttle.java| 8 +- .../org/apache/calcite/sql/validate/AggFinder.java | 7 + .../calcite/sql/validate/AliasNamespace.java | 4 +- .../apache/calcite/sql/validate/SqlValidator.java | 15 ++ .../calcite/sql/validate/SqlValidatorImpl.java | 74 + .../calcite/sql/validate/SqlValidatorUtil.java | 5 +- .../calcite/sql/validate/TableNamespace.java | 4 +- .../calcite/sql/validate/WithItemNamespace.java| 8 +- .../apache/calcite/sql/validate/WithNamespace.java | 2 +- .../validate/implicit/AbstractTypeCoercion.java| 2 +- .../apache/calcite/sql2rel/SqlToRelConverter.java | 84 -- .../calcite/sql2rel/StandardConvertletTable.java | 81 +- .../java/org/apache/calcite/tools/RelBuilder.java | 104 +++- .../org/apache/calcite/util/BuiltInMethod.java | 2 +- .../calcite/adapter/enumerable/EnumUtilsTest.java | 22 +-- .../rel/rel2sql/RelToSqlConverterStructsTest.java | 3 +- .../calcite/rel/rel2sql/RelToSqlConverterTest.java | 180 ++--- .../java/org/apache/calcite/sql/SqlNodeTest.java | 76 + .../apache/calcite/sql/parser/SqlParserTest.java | 2 +- .../parserextensiontesting/SqlCreateTable.java | 4 +- .../calcite/sql/test/SqlOperatorBaseTest.java | 56 --- .../apache/calcite/sql/type/SqlTypeUtilTest.java | 6 +- .../org/apache/calcite/test/JdbcAdapterTest.java | 40 ++--- .../java/org/apache/calcite/test/JdbcTest.java | 17 +- .../org/apache/calcite/test/RelBuilderTest.java| 80 - .../org/apache/calcite/test/RelOptRulesTest.java | 5 +- .../org/apache/calcite/test/RelOptRulesTest.xml| 10 +- .../apache/calcite/test/SqlToRelConverterTest.xml | 42 ++--- .../calcite/test/TypeCoercionConverterTest.xml | 167 ++- core/src/test/resources/sql/agg.iq | 85 +- core/src/test/resources/sql/join.iq| 40 + core/src/test/resources/sql/misc.iq| 33 ++-- core/src/test/resources/sql/outer.iq | 20 +-- core/src/test/resources/sql/sub-query.iq | 25 +-- core/src/test/resources/sql/winagg.iq | 20 +-- .../calcite/piglet/PigRelToSqlConverter.java | 2 +- .../java/org/apache/calcite/test/PigRelOpTest.java |
[GitHub] [calcite] qizhou92 commented on pull request #2236: [CALCITE-3913] Test correctness using formal verification (Qi Zhou)
qizhou92 commented on pull request #2236: URL: https://github.com/apache/calcite/pull/2236#issuecomment-726475805 Thank you ! It might be a stupid question. but what is the time requirement in general for checking RexNode implication, like ~10 milliseconds? ~1 milliseconds? ~0.1 milliseconds? Just want to get a sense. 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 opened a new pull request #2257: Following 4364, fix the plann diff of TpcdsTest
danny0405 opened a new pull request #2257: URL: https://github.com/apache/calcite/pull/2257 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] Aaaaaaron commented on a change in pull request #2253: [CALCITE-4385] Simplifies AND/OR condition that has common expressions, extract and eliminate/merge them as much as possible (
Aaron commented on a change in pull request #2253:
URL: https://github.com/apache/calcite/pull/2253#discussion_r522561420
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -1689,6 +1690,123 @@ RexNode simplifyAnd2ForUnknownAsFalse(List
terms,
}
}
+ /**
+ * Simplifies AND/OR condition that has common expressions,
+ * extract and eliminate/merge them as much as possible.
+ *
+ * (a OR b) AND (a OR b OR c OR d) => (a OR b)
+ * (a OR b OR c OR d) AND (a OR b OR e OR f) => (a OR b) OR ((c OR d)
AND (e OR f))
+
+ * (a AND b) OR (a AND b AND c) => (a AND b)
+ * (a AND b AND c AND d) OR (a AND b AND e AND f) =>
+ * (a AND b) AND ((c AND d) OR (e AND f))
+ *
+ * The difference between {@link #simplifyAnd} is that {@link
#simplifyAnd} mainly
+ * simplifies expressions whose answer can be determined without evaluating
both sides,
+ * like: FALSE AND (xxx) => FALSE
+ */
+ public RexNode eliminateCommonExprInCondition(RexNode call) {
+if (!(call instanceof RexCall)) {
+ return call;
+}
+// Simplify children recursively
+// so that when we simplify parent node
+// the child node should be simplified
+List operands = ((RexCall) call).getOperands();
+List simplifiedChildren = operands.stream()
+.map(this::eliminateCommonExprInCondition)
+.collect(Collectors.toList());
+
+switch (call.getKind()) {
+case AND: {
+ // Split the children to get the disjunctive predicates.
+ List> disjunctions = simplifiedChildren.stream()
+ .map(RelOptUtil::disjunctions)
+ .collect(Collectors.toList());
+
+ // Find the common predict among the list by retainAll.
+ List common = new ArrayList<>(disjunctions.get(0));
+ disjunctions.forEach(common::retainAll);
+
+ if (common.isEmpty()) {
+// no common, return the original but with child simplified
+return and(simplifiedChildren);
+ } else {
Review comment:
Sure~
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 #2189: [CALCITE-4251] Get the origin column, even if it is derived (xzh)
danny0405 commented on a change in pull request #2189:
URL: https://github.com/apache/calcite/pull/2189#discussion_r522559504
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java
##
@@ -338,25 +338,23 @@ public Double getPercentageOriginalRows(RelNode rel) {
}
/**
- * Determines the origin of a column, provided the column maps to a single
- * column that isn't derived.
+ * Determines the origin of a column.
*
* @see #getColumnOrigins(org.apache.calcite.rel.RelNode, int)
*
* @param rel the RelNode of the column
* @param column the offset of the column whose origin we are trying to
* determine
*
- * @return the origin of a column provided it's a simple column; otherwise,
- * returns null
+ * @return the origin of a column;
*/
public RelColumnOrigin getColumnOrigin(RelNode rel, int column) {
final Set origins = getColumnOrigins(rel, column);
if (origins == null || origins.size() != 1) {
return null;
}
final RelColumnOrigin origin = Iterables.getOnlyElement(origins);
-return origin.isDerived() ? null : origin;
+return origin;
Review comment:
The code that uses this method should also be tweaked, such as the
`LoptMultiJoin`.
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 #2250: Revert SEARCH
danny0405 commented on pull request #2250: URL: https://github.com/apache/calcite/pull/2250#issuecomment-726456328 -1 for this change, we and a discussion there and all people vote -1, please do not push the PR again. 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] xy2953396112 commented on a change in pull request #2189: [CALCITE-4251] Get the origin column, even if it is derived (xzh)
xy2953396112 commented on a change in pull request #2189:
URL: https://github.com/apache/calcite/pull/2189#discussion_r522525278
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java
##
@@ -338,25 +338,24 @@ public Double getPercentageOriginalRows(RelNode rel) {
}
/**
- * Determines the origin of a column, provided the column maps to a single
- * column that isn't derived.
+ * Determines the origin of a column, provided the column maps to a origin
+ * column.
*
* @see #getColumnOrigins(org.apache.calcite.rel.RelNode, int)
*
* @param rel the RelNode of the column
* @param column the offset of the column whose origin we are trying to
* determine
*
- * @return the origin of a column provided it's a simple column; otherwise,
- * returns null
+ * @return the origin of a column provided it's a simple column;
Review comment:
Thanks for review, update the doc.
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] xy2953396112 commented on pull request #2207: [CALCITE-4331] RelOptUtil#areRowTypesEqual should compare type nullability even for ANY types
xy2953396112 commented on pull request #2207: URL: https://github.com/apache/calcite/pull/2207#issuecomment-726422915 +1 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] vlsi commented on pull request #2236: [CALCITE-3913] Test correctness using formal verification (Qi Zhou)
vlsi commented on pull request #2236: URL: https://github.com/apache/calcite/pull/2236#issuecomment-726355820 I guess we want to keep RexImplicationChecker, so we don't want to use slow approaches like z3 there. On the other hand, z3 might be helpful for testing RexImplicationChecker. 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-avatica] joshelser commented on a change in pull request #130: [CALCITE-4379] Meta.Frame created with java float values in rows hits a ClassCastException in toProto()
joshelser commented on a change in pull request #130:
URL: https://github.com/apache/calcite-avatica/pull/130#discussion_r522439536
##
File path: core/src/test/java/org/apache/calcite/avatica/FrameTest.java
##
@@ -105,6 +106,84 @@ public void testMultipleRows() {
serializeAndTestEquality(singleRow);
}
+ protected void testValueRoundTrip(Object value) {
Review comment:
Can you please update `testSingleRow` in this class to also use this
helper method you've created?
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] qizhou92 commented on pull request #2236: [CALCITE-3913] Test correctness using formal verification (Qi Zhou)
qizhou92 commented on pull request #2236: URL: https://github.com/apache/calcite/pull/2236#issuecomment-726353203 Hi, I implement the function that verifies the equivalence of two rex node, including numerical value, and a boolean value. I use some test cases previously in RexProgromTest to verify the correctness of RexSimplify. I can keep working support more constructors such as CASE, or more types, such as DATE. another question I have is I found there is a class is called RexImplicationChecker, Z3 can do a better job to check the implication between two RexNode. For example, the current RexImplicationChecker seems cannot verify that Not (a <=30 ) ==> a > 10. I am wondering If that would be a good use for z3. 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 #2250: Revert SEARCH
rubenada commented on pull request #2250: URL: https://github.com/apache/calcite/pull/2250#issuecomment-726330905 Should we consider slowTests for such a big PR? 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] yanlin-Lynn commented on a change in pull request #2189: [CALCITE-4251] Get the origin column, even if it is derived (xzh)
yanlin-Lynn commented on a change in pull request #2189:
URL: https://github.com/apache/calcite/pull/2189#discussion_r522107178
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java
##
@@ -338,25 +338,24 @@ public Double getPercentageOriginalRows(RelNode rel) {
}
/**
- * Determines the origin of a column, provided the column maps to a single
- * column that isn't derived.
+ * Determines the origin of a column, provided the column maps to a origin
+ * column.
*
* @see #getColumnOrigins(org.apache.calcite.rel.RelNode, int)
*
* @param rel the RelNode of the column
* @param column the offset of the column whose origin we are trying to
* determine
*
- * @return the origin of a column provided it's a simple column; otherwise,
- * returns null
+ * @return the origin of a column provided it's a simple column;
Review comment:
The doc here does not match with the code.
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] yanlin-Lynn commented on a change in pull request #2189: [CALCITE-4251] Get the origin column, even if it is derived (xzh)
yanlin-Lynn commented on a change in pull request #2189:
URL: https://github.com/apache/calcite/pull/2189#discussion_r522106767
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java
##
@@ -338,25 +338,24 @@ public Double getPercentageOriginalRows(RelNode rel) {
}
/**
- * Determines the origin of a column, provided the column maps to a single
- * column that isn't derived.
+ * Determines the origin of a column, provided the column maps to a origin
+ * column.
Review comment:
The annotation here is in-correct.
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 #2253: [CALCITE-4385] Simplifies AND/OR condition that has common expressions, extract and eliminate/merge them as much as possible (J
rubenada commented on a change in pull request #2253:
URL: https://github.com/apache/calcite/pull/2253#discussion_r522040936
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -1689,6 +1690,123 @@ RexNode simplifyAnd2ForUnknownAsFalse(List
terms,
}
}
+ /**
+ * Simplifies AND/OR condition that has common expressions,
+ * extract and eliminate/merge them as much as possible.
+ *
+ * (a OR b) AND (a OR b OR c OR d) => (a OR b)
+ * (a OR b OR c OR d) AND (a OR b OR e OR f) => (a OR b) OR ((c OR d)
AND (e OR f))
+
+ * (a AND b) OR (a AND b AND c) => (a AND b)
+ * (a AND b AND c AND d) OR (a AND b AND e AND f) =>
+ * (a AND b) AND ((c AND d) OR (e AND f))
+ *
+ * The difference between {@link #simplifyAnd} is that {@link
#simplifyAnd} mainly
+ * simplifies expressions whose answer can be determined without evaluating
both sides,
+ * like: FALSE AND (xxx) => FALSE
+ */
+ public RexNode eliminateCommonExprInCondition(RexNode call) {
Review comment:
I uderstand @Aaron , thanks for the explanation.
IMHO the current PR as it is (a new method that is not used, apart from the
unit test), does not make much sense. I would prefer to see a PR combining 1 &
2 from my previous comment (i.e. this new method, used by a certain join rule,
so that we can see it "in action").
Having said that, if people think otherwise, I would not oppose to merging
the current PR as it 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
[GitHub] [calcite] rubenada commented on a change in pull request #2253: [CALCITE-4385] Simplifies AND/OR condition that has common expressions, extract and eliminate/merge them as much as possible (J
rubenada commented on a change in pull request #2253:
URL: https://github.com/apache/calcite/pull/2253#discussion_r522038393
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -1689,6 +1690,123 @@ RexNode simplifyAnd2ForUnknownAsFalse(List
terms,
}
}
+ /**
+ * Simplifies AND/OR condition that has common expressions,
+ * extract and eliminate/merge them as much as possible.
+ *
+ * (a OR b) AND (a OR b OR c OR d) => (a OR b)
+ * (a OR b OR c OR d) AND (a OR b OR e OR f) => (a OR b) OR ((c OR d)
AND (e OR f))
+
+ * (a AND b) OR (a AND b AND c) => (a AND b)
+ * (a AND b AND c AND d) OR (a AND b AND e AND f) =>
+ * (a AND b) AND ((c AND d) OR (e AND f))
+ *
+ * The difference between {@link #simplifyAnd} is that {@link
#simplifyAnd} mainly
+ * simplifies expressions whose answer can be determined without evaluating
both sides,
+ * like: FALSE AND (xxx) => FALSE
+ */
+ public RexNode eliminateCommonExprInCondition(RexNode call) {
+if (!(call instanceof RexCall)) {
+ return call;
+}
+// Simplify children recursively
+// so that when we simplify parent node
+// the child node should be simplified
+List operands = ((RexCall) call).getOperands();
+List simplifiedChildren = operands.stream()
+.map(this::eliminateCommonExprInCondition)
+.collect(Collectors.toList());
+
+switch (call.getKind()) {
+case AND: {
+ // Split the children to get the disjunctive predicates.
+ List> disjunctions = simplifiedChildren.stream()
+ .map(RelOptUtil::disjunctions)
+ .collect(Collectors.toList());
+
+ // Find the common predict among the list by retainAll.
+ List common = new ArrayList<>(disjunctions.get(0));
+ disjunctions.forEach(common::retainAll);
+
+ if (common.isEmpty()) {
+// no common, return the original but with child simplified
+return and(simplifiedChildren);
+ } else {
Review comment:
Personally, I think avoiding else after return would make this method's
code more clear.
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] xy2953396112 commented on a change in pull request #2189: [CALCITE-4251] Get the origin column, even if it is derived (xzh)
xy2953396112 commented on a change in pull request #2189:
URL: https://github.com/apache/calcite/pull/2189#discussion_r522033219
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java
##
@@ -351,11 +353,32 @@ public Double getPercentageOriginalRows(RelNode rel) {
* returns null
*/
public RelColumnOrigin getColumnOrigin(RelNode rel, int column) {
+return getColumnOrigin(rel, column, false);
+ }
+
+ /**
+ * Determines the origin of a column. If the param derived is false,
provided the column maps
+ * to a single column that isn't derived, otherwise the column is derived.
+ *
+ * @see #getColumnOrigins(org.apache.calcite.rel.RelNode, int)
+ *
+ * @param rel the RelNode of the column
+ * @param column the offset of the column whose origin we are trying to
+ * determine
+ * @param derived allow column derived
+ *
+ * @return the origin of a column provided it's a simple column; otherwise,
+ * returns null
+ */
+ public RelColumnOrigin getColumnOrigin(RelNode rel, int column, Boolean
derived) {
final Set origins = getColumnOrigins(rel, column);
if (origins == null || origins.size() != 1) {
return null;
}
final RelColumnOrigin origin = Iterables.getOnlyElement(origins);
+if (BooleanUtils.isTrue(derived)) {
+ return origin;
+}
return origin.isDerived() ? null : origin;
Review comment:
you are right. return the `origin` directly.
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] xy2953396112 commented on pull request #2189: [CALCITE-4251] Get the origin column, even if it is derived (xzh)
xy2953396112 commented on pull request #2189: URL: https://github.com/apache/calcite/pull/2189#issuecomment-726017100 > Update the commit log please, you should describe what you want to do briefly in the commit log. Overload is not a good commit log. Thanks, update the code. 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] liyafan82 commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
liyafan82 commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r522017061
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +198,35 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ private void compareTypesIgnoringNullability(
+ RelDataType type1, RelDataType type2, boolean expectedResult) {
+String msg = String.format(Locale.ROOT,
+"%s and %s are expected to be %s ignoring nullability.",
+type1.getFullTypeString(),
+type2.getFullTypeString(),
+expectedResult ? "equal" : "not equal");
+
+assertThat(msg,
+SqlTypeUtil.equalSansNullability(f.typeFactory, type1, type2),
is(expectedResult));
+assertThat(msg,
+SqlTypeUtil.equalSansNullability(type1, type2), is(expectedResult));
+ }
+
+ @Test public void testEqualSansNullability() {
+RelDataType bigIntType = f.sqlBigInt;
+RelDataType nullableBigIntType = f.sqlBigIntNullable;
+RelDataType varCharType = f.sqlVarchar;
+RelDataType bigIntType1 =
+f.typeFactory.createTypeWithNullability(nullableBigIntType, false);
+
+// different types
Review comment:
Revised. Thanks for your clarification.
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 #2255: [CALCITE-4393] ExceptionInInitializerError due to NPE in SqlCallBinding caused by circular dependency
rubenada commented on a change in pull request #2255:
URL: https://github.com/apache/calcite/pull/2255#discussion_r522006110
##
File path: core/src/main/java/org/apache/calcite/sql/SqlCallBinding.java
##
@@ -411,4 +411,8 @@ public CalciteException newValidationError(
public boolean isTypeCoercionEnabled() {
return validator.config().typeCoercionEnabled();
}
+
+ private SqlCall createDefaultCall() {
+return SqlStdOperatorTable.DEFAULT.createCall(SqlParserPos.ZERO);
Review comment:
Yes... but my fear is that it would be a more "obscure" solution when
looking at this code in the future: "why is this static nested class here, for
apparently no reason, just with a private static field...".
I guess it can be a good (and efficient) solution, but we'll need a clear
javadoc stating the purpose of the class and why it is needed (and cannot be
removed). It should be feasible, I'll commit this approach.
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] vlsi commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
vlsi commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r522001709
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +198,35 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ private void compareTypesIgnoringNullability(
+ RelDataType type1, RelDataType type2, boolean expectedResult) {
+String msg = String.format(Locale.ROOT,
+"%s and %s are expected to be %s ignoring nullability.",
+type1.getFullTypeString(),
+type2.getFullTypeString(),
+expectedResult ? "equal" : "not equal");
+
+assertThat(msg,
+SqlTypeUtil.equalSansNullability(f.typeFactory, type1, type2),
is(expectedResult));
+assertThat(msg,
+SqlTypeUtil.equalSansNullability(type1, type2), is(expectedResult));
+ }
+
+ @Test public void testEqualSansNullability() {
+RelDataType bigIntType = f.sqlBigInt;
+RelDataType nullableBigIntType = f.sqlBigIntNullable;
+RelDataType varCharType = f.sqlVarchar;
+RelDataType bigIntType1 =
+f.typeFactory.createTypeWithNullability(nullableBigIntType, false);
+
+// different types
Review comment:
I mean `different types` should be a comment parameter rather than a
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
liyafan82 commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r521997690
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +198,35 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ private void compareTypesIgnoringNullability(
+ RelDataType type1, RelDataType type2, boolean expectedResult) {
+String msg = String.format(Locale.ROOT,
+"%s and %s are expected to be %s ignoring nullability.",
+type1.getFullTypeString(),
+type2.getFullTypeString(),
+expectedResult ? "equal" : "not equal");
+
+assertThat(msg,
+SqlTypeUtil.equalSansNullability(f.typeFactory, type1, type2),
is(expectedResult));
+assertThat(msg,
+SqlTypeUtil.equalSansNullability(type1, type2), is(expectedResult));
+ }
+
+ @Test public void testEqualSansNullability() {
+RelDataType bigIntType = f.sqlBigInt;
+RelDataType nullableBigIntType = f.sqlBigIntNullable;
+RelDataType varCharType = f.sqlVarchar;
+RelDataType bigIntType1 =
+f.typeFactory.createTypeWithNullability(nullableBigIntType, false);
+
+// different types
Review comment:
I guess you mean the type factory should be a parameter? Revised
accordingly. 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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
liyafan82 commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r521997332
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +198,35 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ private void compareTypesIgnoringNullability(
+ RelDataType type1, RelDataType type2, boolean expectedResult) {
+String msg = String.format(Locale.ROOT,
+"%s and %s are expected to be %s ignoring nullability.",
+type1.getFullTypeString(),
+type2.getFullTypeString(),
+expectedResult ? "equal" : "unequal");
+
Review comment:
Sounds reasonable. Revised accordingly. Thank you.
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] vlsi commented on a change in pull request #2255: [CALCITE-4393] ExceptionInInitializerError due to NPE in SqlCallBinding caused by circular dependency
vlsi commented on a change in pull request #2255:
URL: https://github.com/apache/calcite/pull/2255#discussion_r521997200
##
File path: core/src/main/java/org/apache/calcite/sql/SqlCallBinding.java
##
@@ -411,4 +411,8 @@ public CalciteException newValidationError(
public boolean isTypeCoercionEnabled() {
return validator.config().typeCoercionEnabled();
}
+
+ private SqlCall createDefaultCall() {
+return SqlStdOperatorTable.DEFAULT.createCall(SqlParserPos.ZERO);
Review comment:
Just in case, a nested static class would avoid circular dependency as
well.
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 #2255: [CALCITE-4393] ExceptionInInitializerError due to NPE in SqlCallBinding caused by circular dependency
rubenada commented on a change in pull request #2255:
URL: https://github.com/apache/calcite/pull/2255#discussion_r521993824
##
File path: core/src/main/java/org/apache/calcite/sql/SqlCallBinding.java
##
@@ -411,4 +411,8 @@ public CalciteException newValidationError(
public boolean isTypeCoercionEnabled() {
return validator.config().typeCoercionEnabled();
}
+
+ private SqlCall createDefaultCall() {
+return SqlStdOperatorTable.DEFAULT.createCall(SqlParserPos.ZERO);
Review comment:
having a single static call caused a circular dependency, the purpose is
to avoid it (by creating the default call ad-hoc only when needed)
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 #2255: [CALCITE-4393] ExceptionInInitializerError due to NPE in SqlCallBinding caused by circular dependency
rubenada commented on a change in pull request #2255:
URL: https://github.com/apache/calcite/pull/2255#discussion_r521993135
##
File path: core/src/main/java/org/apache/calcite/sql/SqlCallBinding.java
##
@@ -411,4 +411,8 @@ public CalciteException newValidationError(
public boolean isTypeCoercionEnabled() {
return validator.config().typeCoercionEnabled();
}
+
+ private SqlCall createDefaultCall() {
Review comment:
method removed in the latest version
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 #2255: [CALCITE-4393] ExceptionInInitializerError due to NPE in SqlCallBinding caused by circular dependency
rubenada commented on a change in pull request #2255:
URL: https://github.com/apache/calcite/pull/2255#discussion_r521992807
##
File path: core/src/main/java/org/apache/calcite/sql/SqlCallBinding.java
##
@@ -145,10 +144,11 @@ public SqlCall getCall() {
}
final SqlOperandCountRange range = checker.getOperandCountRange();
final List list = Lists.newArrayList(operandList);
+ final SqlCall defaultCall = createDefaultCall();
while (list.size() < range.getMax()
&& checker.isOptional(list.size())
&& checker.isFixedParameters()) {
-list.add(DEFAULT_CALL);
+list.add(defaultCall);
Review comment:
Thanks, I was not aware that most case indeed did not require the
default call. 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
[calcite] branch master updated (5e9943a -> 0ce7685)
This is an automated email from the ASF dual-hosted git repository. rubenql pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git. from 5e9943a [CALCITE-4364] `a IN (1, 2) AND a = 1` should be simplified to `a = 1` add 0ce7685 [CALCITE-4390] SqlMatchRecognize returns wrong operand list (Dawid Wysakowicz) No new revisions were added by this update. Summary of changes: .../org/apache/calcite/sql/SqlMatchRecognize.java | 3 ++- .../org/apache/calcite/sql/parser/SqlParserTest.java | 19 +++ 2 files changed, 21 insertions(+), 1 deletion(-)
[calcite] branch master updated (5e9943a -> 0ce7685)
This is an automated email from the ASF dual-hosted git repository. rubenql pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git. from 5e9943a [CALCITE-4364] `a IN (1, 2) AND a = 1` should be simplified to `a = 1` add 0ce7685 [CALCITE-4390] SqlMatchRecognize returns wrong operand list (Dawid Wysakowicz) No new revisions were added by this update. Summary of changes: .../org/apache/calcite/sql/SqlMatchRecognize.java | 3 ++- .../org/apache/calcite/sql/parser/SqlParserTest.java | 19 +++ 2 files changed, 21 insertions(+), 1 deletion(-)
[GitHub] [calcite] vlsi commented on a change in pull request #2246: [CALCITE-4377] Fix the return nullability inference during rex simpli…
vlsi commented on a change in pull request #2246:
URL: https://github.com/apache/calcite/pull/2246#discussion_r521989610
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -900,13 +902,12 @@ private void validateStrongPolicy(RexNode rexNode) {
case ANY:
List operands = ((RexCall) rexNode).getOperands();
if (rexNode.getType().isNullable()) {
-assert operands.stream()
-.map(RexNode::getType)
-.anyMatch(RelDataType::isNullable);
+// Ignores the nullability change when all the operands are literals
+// which come from the simplification.
+assert
operands.stream().map(RexNode::getType).anyMatch(RelDataType::isNullable)
+|| operands.stream().allMatch(p -> RexUtil.isLiteral(p, false));
Review comment:
`case ANY` is defined as `return type is nullable if and only if one of
the arguments is null`.
It allows no exceptions like "ok, the return type might be null if one of
the arguments is literal".
That is why I believe `RexUtil.isLiteral` must not be 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
[GitHub] [calcite] zabetak commented on a change in pull request #2094: [CALCITE-3409] Add an interface in RelOptMaterializations…
zabetak commented on a change in pull request #2094:
URL: https://github.com/apache/calcite/pull/2094#discussion_r521986581
##
File path: core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##
@@ -197,6 +198,10 @@ public boolean isRuleExcluded(RelOptRule rule) {
return this;
}
+ @Override public void addExtraMaterializationRules(List rules) {
Review comment:
I also share @jcamachor view point. If we decide to allow customization
of this part its better to allow full-control.
Furthermore, to be consistent with the existing APIs of the planner, I would
opt for a per rule addition:
`addMaterializationRule(UnifyRule rule)`
Going one step further, is it really necessary to introduce a new API for
this? The point is that we want to add new rules to the planner so can't we use
the existing `addRule(RelOptRule)` interface? I guess we want to distinguish
those rules that are specific to materialization but this could be done in the
implementation of `addRule`.
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
[GitHub] [calcite] danny0405 commented on a change in pull request #2246: [CALCITE-4377] Fix the return nullability inference during rex simpli…
danny0405 commented on a change in pull request #2246:
URL: https://github.com/apache/calcite/pull/2246#discussion_r521986932
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -900,13 +902,12 @@ private void validateStrongPolicy(RexNode rexNode) {
case ANY:
List operands = ((RexCall) rexNode).getOperands();
if (rexNode.getType().isNullable()) {
-assert operands.stream()
-.map(RexNode::getType)
-.anyMatch(RelDataType::isNullable);
+// Ignores the nullability change when all the operands are literals
+// which come from the simplification.
+assert
operands.stream().map(RexNode::getType).anyMatch(RelDataType::isNullable)
+|| operands.stream().allMatch(p -> RexUtil.isLiteral(p, false));
Review comment:
The `nullness policy` is always there, we should make the validation
adapter to the simplification.
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 merged pull request #2254: [CALCITE-4390] Fix SqlMatchRecognize operand list
rubenada merged pull request #2254: URL: https://github.com/apache/calcite/pull/2254 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] vlsi commented on a change in pull request #2246: [CALCITE-4377] Fix the return nullability inference during rex simpli…
vlsi commented on a change in pull request #2246:
URL: https://github.com/apache/calcite/pull/2246#discussion_r521981405
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -900,13 +902,12 @@ private void validateStrongPolicy(RexNode rexNode) {
case ANY:
List operands = ((RexCall) rexNode).getOperands();
if (rexNode.getType().isNullable()) {
-assert operands.stream()
-.map(RexNode::getType)
-.anyMatch(RelDataType::isNullable);
+// Ignores the nullability change when all the operands are literals
+// which come from the simplification.
+assert
operands.stream().map(RexNode::getType).anyMatch(RelDataType::isNullable)
+|| operands.stream().allMatch(p -> RexUtil.isLiteral(p, false));
Review comment:
See https://issues.apache.org/jira/browse/CALCITE-4387
Simplify should be allowed to **both** widen and narrowing nullability.
I believe we must not alter the definition of `nullness policy`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
[GitHub] [calcite] vlsi commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
vlsi commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r521978173
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +198,35 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ private void compareTypesIgnoringNullability(
+ RelDataType type1, RelDataType type2, boolean expectedResult) {
+String msg = String.format(Locale.ROOT,
+"%s and %s are expected to be %s ignoring nullability.",
+type1.getFullTypeString(),
+type2.getFullTypeString(),
+expectedResult ? "equal" : "unequal");
+
Review comment:
Another issue here is that **both** asserts have the same message, so it
is hard to tell which fails.
I would suggest:
```java
assertThat(
"SqlTypeUtil.equalSansNullability(typeFactory, " + type1 + ", " +
type2 + "), comment: " + comment,
SqlTypeUtil.equalSansNullability(f.typeFactory, type1, type2),
is(expectedResult));
assertThat(
"SqlTypeUtil.equalSansNullability(" + type1 + ", " + type2 + "),
comment: " + comment,
SqlTypeUtil.equalSansNullability(type1, type2), is(expectedResult));
```
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] vlsi commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
vlsi commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r521979340
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +198,35 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ private void compareTypesIgnoringNullability(
+ RelDataType type1, RelDataType type2, boolean expectedResult) {
+String msg = String.format(Locale.ROOT,
+"%s and %s are expected to be %s ignoring nullability.",
+type1.getFullTypeString(),
+type2.getFullTypeString(),
+expectedResult ? "equal" : "not equal");
+
+assertThat(msg,
+SqlTypeUtil.equalSansNullability(f.typeFactory, type1, type2),
is(expectedResult));
+assertThat(msg,
+SqlTypeUtil.equalSansNullability(type1, type2), is(expectedResult));
+ }
+
+ @Test public void testEqualSansNullability() {
+RelDataType bigIntType = f.sqlBigInt;
+RelDataType nullableBigIntType = f.sqlBigIntNullable;
+RelDataType varCharType = f.sqlVarchar;
+RelDataType bigIntType1 =
+f.typeFactory.createTypeWithNullability(nullableBigIntType, false);
+
+// different types
Review comment:
This should a parameter to `compareTypesIgnoringNullability` method.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
vlsi commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r521978173
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +198,35 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ private void compareTypesIgnoringNullability(
+ RelDataType type1, RelDataType type2, boolean expectedResult) {
+String msg = String.format(Locale.ROOT,
+"%s and %s are expected to be %s ignoring nullability.",
+type1.getFullTypeString(),
+type2.getFullTypeString(),
+expectedResult ? "equal" : "unequal");
+
Review comment:
Another issue here is that **both** asserts have the same message, so it
is hard to tell which fails.
I would suggest:
```java
assertThat(
"SqlTypeUtil.equalSansNullability(typeFactory, " + type1 + ", " +
type2 + ")",
SqlTypeUtil.equalSansNullability(f.typeFactory, type1, type2),
is(expectedResult));
assertThat(
"SqlTypeUtil.equalSansNullability(" + type1 + ", " + type2 + ")",
SqlTypeUtil.equalSansNullability(type1, type2), is(expectedResult));
```
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] liyafan82 commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
liyafan82 commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r521970944
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +198,35 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ private void compareTypesIgnoringNullability(
+ RelDataType type1, RelDataType type2, boolean expectedResult) {
+String msg = String.format(Locale.ROOT,
+"%s and %s are expected to be %s ignoring nullability.",
+type1.getFullTypeString(),
+type2.getFullTypeString(),
+expectedResult ? "equal" : "unequal");
+
Review comment:
Changed to 'not equal'. 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
[GitHub] [calcite] danny0405 commented on a change in pull request #2246: [CALCITE-4377] Fix the return nullability inference during rex simpli…
danny0405 commented on a change in pull request #2246:
URL: https://github.com/apache/calcite/pull/2246#discussion_r521969779
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -900,13 +902,12 @@ private void validateStrongPolicy(RexNode rexNode) {
case ANY:
List operands = ((RexCall) rexNode).getOperands();
if (rexNode.getType().isNullable()) {
-assert operands.stream()
-.map(RexNode::getType)
-.anyMatch(RelDataType::isNullable);
+// Ignores the nullability change when all the operands are literals
+// which come from the simplification.
+assert
operands.stream().map(RexNode::getType).anyMatch(RelDataType::isNullable)
+|| operands.stream().allMatch(p -> RexUtil.isLiteral(p, false));
Review comment:
During the simplification, we drop the useless cast `cast(1 as nullable
INT)` which breaks the nullability. We have 2 choices:
1. Do not make any simplification that breaks the nullability
2. Make the validation strategy more relaxed
I choose the latter.
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 #2246: [CALCITE-4377] Fix the return nullability inference during rex simpli…
danny0405 commented on a change in pull request #2246:
URL: https://github.com/apache/calcite/pull/2246#discussion_r521969779
##
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##
@@ -900,13 +902,12 @@ private void validateStrongPolicy(RexNode rexNode) {
case ANY:
List operands = ((RexCall) rexNode).getOperands();
if (rexNode.getType().isNullable()) {
-assert operands.stream()
-.map(RexNode::getType)
-.anyMatch(RelDataType::isNullable);
+// Ignores the nullability change when all the operands are literals
+// which come from the simplification.
+assert
operands.stream().map(RexNode::getType).anyMatch(RelDataType::isNullable)
+|| operands.stream().allMatch(p -> RexUtil.isLiteral(p, false));
Review comment:
During the simplification, we drop the useless case `cast(1 as nullable
INT)` which breaks the nullability. We have 2 choices:
1. Do not make any simplification that breaks the nullability
2. Make the validation strategy more relaxed
I choose the latter.
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 merged pull request #2238: [CALCITE-4364] `a IN (1, 2) AND a = 1` should be simplified to `a = 1`
danny0405 merged pull request #2238: URL: https://github.com/apache/calcite/pull/2238 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-4364] `a IN (1, 2) AND a = 1` should be simplified to `a = 1`
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 5e9943a [CALCITE-4364] `a IN (1, 2) AND a = 1` should be simplified
to `a = 1`
5e9943a is described below
commit 5e9943aa1f51a97068fc37d53dea1d447570becc
Author: yuzhao.cyz
AuthorDate: Thu Nov 5 15:01:59 2020 +0800
[CALCITE-4364] `a IN (1, 2) AND a = 1` should be simplified to `a = 1`
---
.../java/org/apache/calcite/rex/RexSimplify.java | 90 ---
.../main/java/org/apache/calcite/rex/RexUtil.java | 30 -
.../java/org/apache/calcite/util/RangeSets.java| 12 ++
.../org/apache/calcite/rex/RexProgramTest.java | 128 ++---
.../org/apache/calcite/test/RelBuilderTest.java| 6 +-
.../java/org/apache/calcite/util/RangeSetTest.java | 32 ++
.../org/apache/calcite/test/RelOptRulesTest.xml| 10 +-
core/src/test/resources/sql/sub-query.iq | 2 +-
.../org/apache/calcite/test/DruidAdapter2IT.java | 8 +-
.../org/apache/calcite/test/DruidAdapterIT.java| 8 +-
10 files changed, 250 insertions(+), 76 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 65b3ba3..06acfb5 100644
--- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
+++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
@@ -1330,7 +1330,7 @@ public class RexSimplify {
final SargCollector sargCollector = new SargCollector(rexBuilder, true);
operands.forEach(t -> sargCollector.accept(t, terms));
-if (sargCollector.map.values().stream().anyMatch(b -> b.complexity() > 1))
{
+if (sargCollector.needToFix(unknownAs)) {
operands.clear();
terms.forEach(t -> operands.add(sargCollector.fix(rexBuilder, t)));
}
@@ -1797,7 +1797,7 @@ public class RexSimplify {
final SargCollector sargCollector = new SargCollector(rexBuilder, false);
final List newTerms = new ArrayList<>();
terms.forEach(t -> sargCollector.accept(t, newTerms));
-if (sargCollector.map.values().stream().anyMatch(b -> b.complexity() > 1))
{
+if (sargCollector.needToFix(unknownAs)) {
terms.clear();
newTerms.forEach(t -> terms.add(sargCollector.fix(rexBuilder, t)));
}
@@ -2591,6 +2591,12 @@ public class RexSimplify {
final Map map = new HashMap<>();
private final RexBuilder rexBuilder;
private final boolean negate;
+/**
+ * Count of the new terms after converting all the operands to
+ * {@code SEARCH} on a {@link Sarg}. It is used to decide whether
+ * the new terms are simpler.
+ */
+private int newTermsCount;
SargCollector(RexBuilder rexBuilder, boolean negate) {
this.rexBuilder = rexBuilder;
@@ -2601,6 +2607,7 @@ public class RexSimplify {
if (!accept_(term, newTerms)) {
newTerms.add(term);
}
+ newTermsCount = newTerms.size();
}
private boolean accept_(RexNode e, List newTerms) {
@@ -2710,14 +2717,76 @@ public class RexSimplify {
}
}
+/**
+ * Returns whether the merged {@code sarg} with given {@code unknownAs}
keeps the semantics,
+ * The merge can not go ahead if the semantics change.
+ *
+ * @return true if the semantics does not change
+ */
+private static boolean canMerge(Sarg sarg, RexUnknownAs unknownAs) {
+ final boolean isAllOrNone = sarg.isAll() || sarg.isNone();
+ final boolean containsNull = sarg.containsNull;
+ switch (unknownAs) {
+ case UNKNOWN:
+// "unknown as unknown" can not be simplified to
+// "IS NULL"/"IS NOT NULL"/"TRUE"/"FALSE"
+return !isAllOrNone;
+ case TRUE:
+// "unknown as true" can not be simplified to
+// "false" or "IS NOT NULL"
+return containsNull || !isAllOrNone;
+ case FALSE:
+// "unknown as false" can not be simplified to
+// "true" or "IS NULL"
+return !containsNull || !isAllOrNone;
+ default:
+return true;
+ }
+}
+
+/** Returns whether it is worth to fix and convert to {@code SEARCH}
calls. */
+boolean needToFix(RexUnknownAs unknownAs) {
+ // Fix and converts to SEARCH if:
+ // 1. A Sarg has complexity greater than 1;
+ // 2. The terms are reduced as simpler Sarg points;
+ // 3. The terms are reduced as simpler Sarg comparison.
+
+ // Ignore 'negate' just to be compatible with previous versions of this
+ // method. "build().complexity()" would be a better estimate, if we could
+ // switch to it breaking lots of plans.
+ final Collection builders = map.values();
+ if (builders.stream().anyMatch(b -> b.build(false).complexity() > 1)) {
+return true;
+ }
+ if (builders.size() == 1
+ && !canMerge(buil
[GitHub] [calcite] danny0405 commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
danny0405 commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r521961891
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +198,35 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ private void compareTypesIgnoringNullability(
+ RelDataType type1, RelDataType type2, boolean expectedResult) {
+String msg = String.format(Locale.ROOT,
+"%s and %s are expected to be %s ignoring nullability.",
+type1.getFullTypeString(),
+type2.getFullTypeString(),
+expectedResult ? "equal" : "unequal");
+
Review comment:
`unequal ` is not a valid word.
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] liyafan82 commented on a change in pull request #2256: [CALCITE-4392] The operation of checking types equal ignoring null can be more efficient
liyafan82 commented on a change in pull request #2256:
URL: https://github.com/apache/calcite/pull/2256#discussion_r521911562
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -197,4 +197,31 @@ private RelDataType struct(RelDataType...relDataTypes) {
}
return builder.build();
}
+
+ @Test public void testEqualSansNullability() {
+final RelDataTypeFactory factory = f.typeFactory;
+
+RelDataType bigIntType = f.sqlBigInt;
+RelDataType nullableBigIntType = f.sqlBigIntNullable;
+RelDataType varCharType = f.sqlVarchar;
+
+// different types
+assertThat(
+SqlTypeUtil.equalSansNullability(factory, bigIntType, varCharType),
is(false));
+assertThat(
+SqlTypeUtil.equalSansNullability(bigIntType, varCharType), is(false));
Review comment:
Done. Thanks for the good suggestion.
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
