[GitHub] [calcite] danny0405 opened a new pull request #1920: [CALCITE-3894] The Union operation between DATE with TIMESTAMP return…
danny0405 opened a new pull request #1920: [CALCITE-3894] The Union operation between DATE with TIMESTAMP return… URL: https://github.com/apache/calcite/pull/1920 …s a wrong result The RelDataTypeFactory#leastRestrictive finds the common type for IN, CASE and SET operations. For common type with DATE and TIMESTAMP, it returns DATE. The root cause is that rules in SqlTypeAssignmentRule decide that DATE is assignable from TIMESTAMP, which is actually wrong. Although in Java, this assignment makes sense but that does not mean it's true in SQL, because DATE and TIMESTAMP have different time unit. Fix the rules in SqlTypeAssignmentRule and let the implicit type coercion rule get involved. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] danny0405 closed pull request #1919: [CALCITE-3894] The Union operation between DATE with TIMESTAMP return…
danny0405 closed pull request #1919: [CALCITE-3894] The Union operation between DATE with TIMESTAMP return… URL: https://github.com/apache/calcite/pull/1919 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] DonnyZone closed pull request #1904: [CALCITE-3893] [CALCITE-3895] SQL with GROUP_ID may generate wrong plan
DonnyZone closed pull request #1904: [CALCITE-3893] [CALCITE-3895] SQL with GROUP_ID may generate wrong plan URL: https://github.com/apache/calcite/pull/1904 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[calcite] branch CALCITE-3894 updated (8654d4d -> d63438d)
This is an automated email from the ASF dual-hosted git repository. danny0405 pushed a change to branch CALCITE-3894 in repository https://gitbox.apache.org/repos/asf/calcite.git. discard 8654d4d [CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a wrong result add d63438d [CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a wrong result This update added new revisions after undoing existing revisions. That is to say, some revisions that were in the old version of the branch are not in the new version. This situation occurs when a user --force pushes a change and generates a repository containing something like this: * -- * -- B -- O -- O -- O (8654d4d) \ N -- N -- N refs/heads/CALCITE-3894 (d63438d) You should already have received notification emails for all of the O revisions, and so the following emails describe only the N revisions from the common base, B. Any revisions marked "omit" are not gone; other references still refer to them. Any revisions marked "discard" are gone forever. No new revisions were added by this update. Summary of changes:
[GitHub] [calcite] chunweilei commented on a change in pull request #1917: [CALCITE-3910] Enhance ProjectJoinTransposeRule to support SemiJoin and AntiJoin
chunweilei commented on a change in pull request #1917: [CALCITE-3910] Enhance ProjectJoinTransposeRule to support SemiJoin and AntiJoin URL: https://github.com/apache/calcite/pull/1917#discussion_r409268046 ## File path: core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml ## @@ -7350,6 +7350,44 @@ LogicalProject(NAME=[$1]) LogicalFilter(condition=[>($0, 10)]) LogicalTableScan(table=[[CATALOG, SALES, DEPT]]) LogicalTableScan(table=[[CATALOG, SALES, DEPT]]) +]]> + + + + + + + +
[GitHub] [calcite] danny0405 opened a new pull request #1919: [CALCITE-3894] The Union operation between DATE with TIMESTAMP return…
danny0405 opened a new pull request #1919: [CALCITE-3894] The Union operation between DATE with TIMESTAMP return… URL: https://github.com/apache/calcite/pull/1919 …s a wrong result The RelDataTypeFactory#leastRestrictive finds the common type for IN, CASE and SET operations. For common type with DATE and TIMESTAMP, it returns DATE. The root cause is that rules in SqlTypeAssignmentRule decide that DATE is assignable from TIMESTAMP and TIMESTAMP is assignable from DATE, which is actually wrong. Although in Java, this assignment makes sense but that does not mean it's true in SQL, because DATE and TIMESTAMP have different time unit. Fix the rules in SqlTypeAssignmentRule and let the implicit type coercion rule trigger. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[calcite] 01/01: [CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a wrong result
This is an automated email from the ASF dual-hosted git repository.
danny0405 pushed a commit to branch CALCITE-3894
in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 8654d4d8c9c20685c80ff633e505302be86c9697
Author: yuzhao.cyz
AuthorDate: Thu Apr 16 11:48:40 2020 +0800
[CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a
wrong result
The RelDataTypeFactory#leastRestrictive finds the common type for IN,
CASE and SET operations. For common type with DATE and TIMESTAMP, it
returns DATE. The root cause is that rules in SqlTypeAssignmentRule
decide that DATE is assignable from TIMESTAMP and TIMESTAMP is
assignable from DATE, which is actually wrong. Although in Java, this
assignment makes sense but that does not mean it's true in SQL, because DATE
and TIMESTAMP have different time unit.
Fix the rules in SqlTypeAssignmentRule and let the implicit type
coercion rule trigger.
---
.../apache/calcite/sql/type/SqlTypeAssignmentRule.java | 2 --
.../test/java/org/apache/calcite/test/JdbcTest.java| 14 ++
.../apache/calcite/test/TypeCoercionConverterTest.java | 7 +++
.../java/org/apache/calcite/test/TypeCoercionTest.java | 18 ++
.../apache/calcite/test/TypeCoercionConverterTest.xml | 13 +
5 files changed, 52 insertions(+), 2 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java
b/core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java
index 86be0fa..c345105 100644
--- a/core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java
+++ b/core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java
@@ -162,13 +162,11 @@ public class SqlTypeAssignmentRule implements
SqlTypeMappingRule {
// DATE is assignable from...
rule.clear();
rule.add(SqlTypeName.DATE);
-rule.add(SqlTypeName.TIMESTAMP);
rules.add(SqlTypeName.DATE, rule);
// TIME is assignable from...
rule.clear();
rule.add(SqlTypeName.TIME);
-rule.add(SqlTypeName.TIMESTAMP);
rules.add(SqlTypeName.TIME, rule);
// TIME WITH LOCAL TIME ZONE is assignable from...
diff --git a/core/src/test/java/org/apache/calcite/test/JdbcTest.java
b/core/src/test/java/org/apache/calcite/test/JdbcTest.java
index bbf3071..5e36781 100644
--- a/core/src/test/java/org/apache/calcite/test/JdbcTest.java
+++ b/core/src/test/java/org/apache/calcite/test/JdbcTest.java
@@ -7401,6 +7401,20 @@ public class JdbcTest {
.runs();
}
+ /**
+ * Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-3894;>[CALCITE-3894]
+ * The Union operation between DATE with TIMESTAMP returns a wrong
result.
+ */
+ @Test public void testUnionDateTime() {
+CalciteAssert.AssertThat assertThat = CalciteAssert.that();
+String query = "select * from (\n"
++ "select \"id\" from (VALUES(DATE '2018-02-03')) \"foo\"(\"id\")\n"
++ "union\n"
++ "select \"id\" from (VALUES(TIMESTAMP '2008-03-31 12:23:34'))
\"foo\"(\"id\"))";
+assertThat.query(query).returns("id=2008-03-31 12:23:34\nid=2018-02-03
00:00:00\n");
+ }
+
private static String sums(int n, boolean c) {
final StringBuilder b = new StringBuilder();
for (int i = 0; i < n; i++) {
diff --git
a/core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
b/core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
index 03a5fbb..029f44f 100644
--- a/core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
+++ b/core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
@@ -61,6 +61,13 @@ class TypeCoercionConverterTest extends SqlToRelTestBase {
+ "from (values (true, true, true))");
}
+ /** Test cases for {@link TypeCoercion#inOperationCoercion}. */
+ @Test void testInDateTimestamp() {
+checkPlanEquals("select (t1_timestamp, t1_date)\n"
++ "in ((DATE '2020-04-16', TIMESTAMP '2020-04-16 11:40:53'))\n"
++ "from t1");
+ }
+
/** Test cases for
* {@link
org.apache.calcite.sql.validate.implicit.TypeCoercionImpl#booleanEquality}. */
@Test void testBooleanEquality() {
diff --git a/core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
b/core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
index b41c7f6..ddd0a47 100644
--- a/core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
+++ b/core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
@@ -483,6 +483,12 @@ class TypeCoercionTest extends SqlValidatorTestCase {
+ "union select t1_int from t1")
.columnType("VARCHAR NOT NULL");
+// date union timestamp
+sql("select t1_date, t1_timestamp from t1\n"
++ "union select t2_timestamp, t2_date from t2")
+.type("RecordType(TIMESTAMP(0) NOT NULL T1_DATE,"
++ " TIMESTAMP(0) NOT NULL T1_TIMESTAMP) NOT NULL");
+
[calcite] branch CALCITE-3894 updated (f343263 -> 8654d4d)
This is an automated email from the ASF dual-hosted git repository. danny0405 pushed a change to branch CALCITE-3894 in repository https://gitbox.apache.org/repos/asf/calcite.git. discard f343263 [CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a wrong result new 8654d4d [CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a wrong result This update added new revisions after undoing existing revisions. That is to say, some revisions that were in the old version of the branch are not in the new version. This situation occurs when a user --force pushes a change and generates a repository containing something like this: * -- * -- B -- O -- O -- O (f343263) \ N -- N -- N refs/heads/CALCITE-3894 (8654d4d) You should already have received notification emails for all of the O revisions, and so the following emails describe only the N revisions from the common base, B. Any revisions marked "omit" are not gone; other references still refer to them. Any revisions marked "discard" are gone forever. 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:
[calcite] branch CALCITE-3894 created (now f343263)
This is an automated email from the ASF dual-hosted git repository. danny0405 pushed a change to branch CALCITE-3894 in repository https://gitbox.apache.org/repos/asf/calcite.git. at f343263 [CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a wrong result This branch includes the following new commits: new f343263 [CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a wrong result 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.
[calcite] 01/01: [CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a wrong result
This is an automated email from the ASF dual-hosted git repository.
danny0405 pushed a commit to branch CALCITE-3894
in repository https://gitbox.apache.org/repos/asf/calcite.git
commit f34326382b28128dee9fb7fc4fd8f48ab111e271
Author: yuzhao.cyz
AuthorDate: Thu Apr 16 11:48:40 2020 +0800
[CALCITE-3894] The Union operation between DATE with TIMESTAMP returns a
wrong result
The RelDataTypeFactory#leastRestrictive finds the common type for IN,
CASE and SET operations. For common type with DATE and TIMESTAMP, it
returns DATE. The root cause is that rules in SqlTypeAssignmentRule
decide that DATE is assignable from TIMESTAMP and TIMESTAMP is
assignable from DATE, which is actually wrong. Although in Java, this
assignment makes sense but that does not mean it's true in SQL, because DATE
and TIMESTAMP have different time unit.
---
.../apache/calcite/sql/type/SqlTypeAssignmentRule.java | 2 --
.../test/java/org/apache/calcite/test/JdbcTest.java| 14 ++
.../apache/calcite/test/TypeCoercionConverterTest.java | 7 +++
.../java/org/apache/calcite/test/TypeCoercionTest.java | 18 ++
.../apache/calcite/test/TypeCoercionConverterTest.xml | 13 +
5 files changed, 52 insertions(+), 2 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java
b/core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java
index 86be0fa..c345105 100644
--- a/core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java
+++ b/core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java
@@ -162,13 +162,11 @@ public class SqlTypeAssignmentRule implements
SqlTypeMappingRule {
// DATE is assignable from...
rule.clear();
rule.add(SqlTypeName.DATE);
-rule.add(SqlTypeName.TIMESTAMP);
rules.add(SqlTypeName.DATE, rule);
// TIME is assignable from...
rule.clear();
rule.add(SqlTypeName.TIME);
-rule.add(SqlTypeName.TIMESTAMP);
rules.add(SqlTypeName.TIME, rule);
// TIME WITH LOCAL TIME ZONE is assignable from...
diff --git a/core/src/test/java/org/apache/calcite/test/JdbcTest.java
b/core/src/test/java/org/apache/calcite/test/JdbcTest.java
index bbf3071..5e36781 100644
--- a/core/src/test/java/org/apache/calcite/test/JdbcTest.java
+++ b/core/src/test/java/org/apache/calcite/test/JdbcTest.java
@@ -7401,6 +7401,20 @@ public class JdbcTest {
.runs();
}
+ /**
+ * Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-3894;>[CALCITE-3894]
+ * The Union operation between DATE with TIMESTAMP returns a wrong
result.
+ */
+ @Test public void testUnionDateTime() {
+CalciteAssert.AssertThat assertThat = CalciteAssert.that();
+String query = "select * from (\n"
++ "select \"id\" from (VALUES(DATE '2018-02-03')) \"foo\"(\"id\")\n"
++ "union\n"
++ "select \"id\" from (VALUES(TIMESTAMP '2008-03-31 12:23:34'))
\"foo\"(\"id\"))";
+assertThat.query(query).returns("id=2008-03-31 12:23:34\nid=2018-02-03
00:00:00\n");
+ }
+
private static String sums(int n, boolean c) {
final StringBuilder b = new StringBuilder();
for (int i = 0; i < n; i++) {
diff --git
a/core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
b/core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
index 03a5fbb..029f44f 100644
--- a/core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
+++ b/core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
@@ -61,6 +61,13 @@ class TypeCoercionConverterTest extends SqlToRelTestBase {
+ "from (values (true, true, true))");
}
+ /** Test cases for {@link TypeCoercion#inOperationCoercion}. */
+ @Test void testInDateTimestamp() {
+checkPlanEquals("select (t1_timestamp, t1_date)\n"
++ "in ((DATE '2020-04-16', TIMESTAMP '2020-04-16 11:40:53'))\n"
++ "from t1");
+ }
+
/** Test cases for
* {@link
org.apache.calcite.sql.validate.implicit.TypeCoercionImpl#booleanEquality}. */
@Test void testBooleanEquality() {
diff --git a/core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
b/core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
index b41c7f6..ddd0a47 100644
--- a/core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
+++ b/core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
@@ -483,6 +483,12 @@ class TypeCoercionTest extends SqlValidatorTestCase {
+ "union select t1_int from t1")
.columnType("VARCHAR NOT NULL");
+// date union timestamp
+sql("select t1_date, t1_timestamp from t1\n"
++ "union select t2_timestamp, t2_date from t2")
+.type("RecordType(TIMESTAMP(0) NOT NULL T1_DATE,"
++ " TIMESTAMP(0) NOT NULL T1_TIMESTAMP) NOT NULL");
+
// intersect
sql("select t1_int, t1_decimal, t1_smallint, t1_double from t1 "
+
[GitHub] [calcite] xndai commented on a change in pull request #1910: [CALCITE-3915] Add rule listener to report rule attempts and time at …
xndai commented on a change in pull request #1910: [CALCITE-3915] Add rule listener to report rule attempts and time at … URL: https://github.com/apache/calcite/pull/1910#discussion_r409156274 ## File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java ## @@ -167,7 +137,13 @@ /** * Listener for this planner, or null if none set. */ - RelOptListener listener; + MulticastRelOptListener listener; + + /** + * Listener for couting the attempts of each rule. + * Only enabled under DEBUG. + */ + RuleAttemptsListener ruleAttemptsListener; Review comment: Getting RuleAttemptsListener out of MulticastRelOptListener would require a down casting and relay on the implementation detail that the RuleAttemptsListener is always the first listener, which is tricky. And the dump() method is specific to the RuleAttemptsListener, and I don't think it should become part of the interface. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] xndai commented on a change in pull request #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation
xndai commented on a change in pull request #1918: [CALCITE-3926]
CannotPlanException when an empty LogicalValues requires a certain collation
URL: https://github.com/apache/calcite/pull/1918#discussion_r409070045
##
File path: core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java
##
@@ -400,7 +401,13 @@ private static boolean isEmpty(RelNode node) {
public void onMatch(RelOptRuleCall call) {
SingleRel single = call.rel(0);
- call.transformTo(call.builder().push(single).empty().build());
+ RelNode emptyValues = call.builder().push(single).empty().build();
+ if (single instanceof Sort) {
+emptyValues = emptyValues.copy(
+emptyValues.getTraitSet().replace(((Sort) single).getCollation()),
+Collections.emptyList());
+ }
Review comment:
Agree with @hsyuan . This trait replacement is a one-off thing just for
PruneEmptyRule. I am not convinced it should be available in RelBuilder.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation
hsyuan commented on a change in pull request #1918: [CALCITE-3926]
CannotPlanException when an empty LogicalValues requires a certain collation
URL: https://github.com/apache/calcite/pull/1918#discussion_r409054562
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableValues.java
##
@@ -65,6 +65,19 @@ public static EnumerableValues create(RelOptCluster cluster,
return new EnumerableValues(cluster, rowType, tuples, traitSet);
}
+ /** Creates an EnumerableValues. */
+ public static EnumerableValues create(Values input) {
+final RelOptCluster cluster = input.getCluster();
+final ImmutableList> tuples = input.getTuples();
+final RelDataType rowType = input.getRowType();
+final RelTraitSet traitSet =
+input.getTraitSet()
+.replace(EnumerableConvention.INSTANCE)
+.replaceIf(RelDistributionTraitDef.INSTANCE,
+() -> RelMdDistribution.values(rowType, tuples));
+return new EnumerableValues(cluster, rowType, tuples, traitSet);
+ }
Review comment:
Instead of doing this, can we just modify copy(traitset, inputs) method to
just return `new EnumerableValues(getCluster(), rowType, tuples, traitSet);`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation
hsyuan commented on a change in pull request #1918: [CALCITE-3926]
CannotPlanException when an empty LogicalValues requires a certain collation
URL: https://github.com/apache/calcite/pull/1918#discussion_r409044262
##
File path: core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java
##
@@ -400,7 +401,13 @@ private static boolean isEmpty(RelNode node) {
public void onMatch(RelOptRuleCall call) {
SingleRel single = call.rel(0);
- call.transformTo(call.builder().push(single).empty().build());
+ RelNode emptyValues = call.builder().push(single).empty().build();
+ if (single instanceof Sort) {
+emptyValues = emptyValues.copy(
+emptyValues.getTraitSet().replace(((Sort) single).getCollation()),
+Collections.emptyList());
+ }
Review comment:
we can try. But if only empty() considers the trait logic, the other methods
don't, that might look odd.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rubenada commented on a change in pull request #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation
rubenada commented on a change in pull request #1918: [CALCITE-3926]
CannotPlanException when an empty LogicalValues requires a certain collation
URL: https://github.com/apache/calcite/pull/1918#discussion_r409037055
##
File path: core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java
##
@@ -400,7 +401,13 @@ private static boolean isEmpty(RelNode node) {
public void onMatch(RelOptRuleCall call) {
SingleRel single = call.rel(0);
- call.transformTo(call.builder().push(single).empty().build());
+ RelNode emptyValues = call.builder().push(single).empty().build();
+ if (single instanceof Sort) {
+emptyValues = emptyValues.copy(
+emptyValues.getTraitSet().replace(((Sort) single).getCollation()),
+Collections.emptyList());
+ }
Review comment:
Maybe we should put the trait logic in RelBuilder#empty ?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation
hsyuan commented on a change in pull request #1918: [CALCITE-3926]
CannotPlanException when an empty LogicalValues requires a certain collation
URL: https://github.com/apache/calcite/pull/1918#discussion_r409027990
##
File path: core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java
##
@@ -400,7 +401,13 @@ private static boolean isEmpty(RelNode node) {
public void onMatch(RelOptRuleCall call) {
SingleRel single = call.rel(0);
- call.transformTo(call.builder().push(single).empty().build());
+ RelNode emptyValues = call.builder().push(single).empty().build();
+ if (single instanceof Sort) {
+emptyValues = emptyValues.copy(
+emptyValues.getTraitSet().replace(((Sort) single).getCollation()),
+Collections.emptyList());
+ }
Review comment:
Not only sort, but all the operators can have the same issue.
We should update emptyValues to have the same traits with single. Just
collation is not enough, should be all the traits.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation
hsyuan commented on a change in pull request #1918: [CALCITE-3926]
CannotPlanException when an empty LogicalValues requires a certain collation
URL: https://github.com/apache/calcite/pull/1918#discussion_r409028413
##
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##
@@ -3426,4 +3426,24 @@ private void checkExpandTable(RelBuilder builder,
Matcher matcher) {
builder.literal(5));
assertThat(call.toStringRaw(), is("BETWEEN ASYMMETRIC($0, 1, 5)"));
}
+
+ /** Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-3926;>[CALCITE-3926]
+ * CannotPlanException when an empty LogicalValues requires a certain
collation. */
+ @Test void testEmptyValuesWithCollation() throws Exception {
+final RelBuilder builder = RelBuilder.create(config().build());
+final RelNode root =
+builder
+.scan("DEPT")
+.filter(builder.literal(false))
+.sort(
+builder.field("DNAME"),
+builder.field("DEPTNO"))
+.build();
+try (PreparedStatement preparedStatement = RelRunners.run(root)) {
+ final String s =
CalciteAssert.toString(preparedStatement.executeQuery());
+ final String result = "";
+ assertThat(s, is(result));
Review comment:
Can you also compare the plan?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rubenada commented on issue #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation
rubenada commented on issue #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation URL: https://github.com/apache/calcite/pull/1918#issuecomment-614178985 Maybe the same fix should be applied to `PruneEmptyRules.SORT_FETCH_ZERO_INSTANCE` ... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] rubenada opened a new pull request #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation
rubenada opened a new pull request #1918: [CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation URL: https://github.com/apache/calcite/pull/1918 Jira: [CALCITE-3926](https://issues.apache.org/jira/browse/CALCITE-3926) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on issue #1775: [CALCITE-3758] FilterTableScanRule generate wrong mapping for filter …
hsyuan commented on issue #1775: [CALCITE-3758] FilterTableScanRule generate wrong mapping for filter … URL: https://github.com/apache/calcite/pull/1775#issuecomment-614131171 Will do. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] jinxing64 commented on issue #1775: [CALCITE-3758] FilterTableScanRule generate wrong mapping for filter …
jinxing64 commented on issue #1775: [CALCITE-3758] FilterTableScanRule generate wrong mapping for filter … URL: https://github.com/apache/calcite/pull/1775#issuecomment-614130152 @julianhyde @hsyuan Could you please take a look at this ? :D This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #1915: [CALCITE-3921] Support serialize TableModify to json string and deserialize from it
yanlin-Lynn commented on a change in pull request #1915: [CALCITE-3921] Support
serialize TableModify to json string and deserialize from it
URL: https://github.com/apache/calcite/pull/1915#discussion_r408808777
##
File path:
core/src/main/java/org/apache/calcite/rel/externalize/RelJsonReader.java
##
@@ -199,6 +199,9 @@ public boolean getBoolean(String tag, boolean default_) {
public List getExpressionList(String tag) {
@SuppressWarnings("unchecked")
final List jsonNodes = (List) jsonRel.get(tag);
+if (jsonNodes == null) {
+ return null;
Review comment:
when serialize TableModify, the 'updateColumnList' and
'sourceExpressionList' may be null.
For exampl, for this relnode tree
```
LogicalTableModify(table=[[scott, EMP]], operation=[INSERT],
flattened=[false])
LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],
SAL=[$5], COMM=[$6], DEPTNO=[$7])
LogicalTableScan(table=[[scott, EMP]])
```
the serialized json string is
```
{
"rels": [
{
"id": "0",
"relOp": "LogicalTableScan",
"table": [
"scott",
"EMP"
],
"inputs": []
},
{
"id": "1",
"relOp": "LogicalProject",
"fields": [
"EMPNO",
"ENAME",
"JOB",
"MGR",
"HIREDATE",
"SAL",
"COMM",
"DEPTNO"
],
"exprs": [
{
"input": 0,
"name": "$0"
},
{
"input": 1,
"name": "$1"
},
{
"input": 2,
"name": "$2"
},
{
"input": 3,
"name": "$3"
},
{
"input": 4,
"name": "$4"
},
{
"input": 5,
"name": "$5"
},
{
"input": 6,
"name": "$6"
},
{
"input": 7,
"name": "$7"
}
]
},
{
"id": "2",
"relOp": "LogicalTableModify",
"table": [
"scott",
"EMP"
],
"operation": "INSERT",
"flattened": false
}
]
}
```
When trying to get updateColumnList by
`input.getExpressionList("sourceExpressionList")`, the jsonNodes will be null.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[calcite] branch master updated: [CALCITE-3924] Fix flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly (neoReMinD)
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 dfb842e [CALCITE-3924] Fix flakey test to handle TIMESTAMP and
TIMESTAMP(0) correctly (neoReMinD)
dfb842e is described below
commit dfb842e55e1fa7037c8a731341010ed1c0cfb6f7
Author: xu
AuthorDate: Wed Apr 15 09:27:45 2020 +0800
[CALCITE-3924] Fix flakey test to handle TIMESTAMP and TIMESTAMP(0)
correctly (neoReMinD)
The whole if block should be removed. Not only the DATETIME_TYPES has
this problem, all the types that allows precision could have such a
problem, the root cause is that BasicSqlType#toString and digest has
different
handling of printing non specified precision.
One way to solve is to not fire caching of types without precision and
with default precision.
close apache/calcite#1916
---
.../org/apache/calcite/sql/type/BasicSqlType.java | 13 ---
.../calcite/sql/type/SqlTypeFactoryTest.java | 42 ++
2 files changed, 42 insertions(+), 13 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java
b/core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java
index f8323c5..bea494e 100644
--- a/core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java
+++ b/core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java
@@ -179,19 +179,6 @@ public class BasicSqlType extends AbstractSqlType {
boolean printPrecision = precision != PRECISION_NOT_SPECIFIED;
boolean printScale = scale != SCALE_NOT_SPECIFIED;
-// for the digest, print the precision when defaulted,
-// since (for instance) TIME is equivalent to TIME(0).
-if (withDetail) {
- // -1 means there is no default value for precision
- if (typeName.allowsPrec()
- && typeSystem.getDefaultPrecision(typeName) > -1) {
-printPrecision = true;
- }
- if (typeName.getDefaultScale() > -1) {
-printScale = true;
- }
-}
-
if (printPrecision) {
sb.append('(');
sb.append(getPrecision());
diff --git
a/core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java
b/core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java
index e573413..f1c63fd 100644
--- a/core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java
+++ b/core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java
@@ -170,4 +170,46 @@ class SqlTypeFactoryTest {
}
}
+ /** Test case for
+ * https://issues.apache.org/jira/browse/CALCITE-3924;>[CALCITE-3924]
+ * Fix flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly. */
+ @Test void testCreateSqlTypeWithPrecision() {
+SqlTypeFixture f = new SqlTypeFixture();
+checkCreateSqlTypeWithPrecision(f.typeFactory, SqlTypeName.TIME);
+checkCreateSqlTypeWithPrecision(f.typeFactory, SqlTypeName.TIMESTAMP);
+checkCreateSqlTypeWithPrecision(f.typeFactory,
SqlTypeName.TIME_WITH_LOCAL_TIME_ZONE);
+checkCreateSqlTypeWithPrecision(f.typeFactory,
SqlTypeName.TIMESTAMP_WITH_LOCAL_TIME_ZONE);
+ }
+
+ private void checkCreateSqlTypeWithPrecision(
+ RelDataTypeFactory typeFactory, SqlTypeName sqlTypeName) {
+RelDataType ts = typeFactory.createSqlType(sqlTypeName);
+RelDataType tsWithoutPrecision = typeFactory.createSqlType(sqlTypeName,
-1);
+RelDataType tsWithPrecision0 = typeFactory.createSqlType(sqlTypeName, 0);
+RelDataType tsWithPrecision1 = typeFactory.createSqlType(sqlTypeName, 1);
+RelDataType tsWithPrecision2 = typeFactory.createSqlType(sqlTypeName, 2);
+RelDataType tsWithPrecision3 = typeFactory.createSqlType(sqlTypeName, 3);
+// for instance, 8 exceeds max precision for timestamp which is 3
+RelDataType tsWithPrecision8 = typeFactory.createSqlType(sqlTypeName, 8);
+
+assertThat(ts.toString(), is(sqlTypeName.getName() + "(0)"));
+assertThat(ts.getFullTypeString(), is(sqlTypeName.getName() + "(0) NOT
NULL"));
+assertThat(tsWithoutPrecision.toString(), is(sqlTypeName.getName()));
+assertThat(tsWithoutPrecision.getFullTypeString(),
is(sqlTypeName.getName() + " NOT NULL"));
+assertThat(tsWithPrecision0.toString(), is(sqlTypeName.getName() + "(0)"));
+assertThat(tsWithPrecision0.getFullTypeString(), is(sqlTypeName.getName()
+ "(0) NOT NULL"));
+assertThat(tsWithPrecision1.toString(), is(sqlTypeName.getName() + "(1)"));
+assertThat(tsWithPrecision1.getFullTypeString(), is(sqlTypeName.getName()
+ "(1) NOT NULL"));
+assertThat(tsWithPrecision2.toString(), is(sqlTypeName.getName() + "(2)"));
+assertThat(tsWithPrecision2.getFullTypeString(), is(sqlTypeName.getName()
+ "(2) NOT NULL"));
+assertThat(tsWithPrecision3.toString(), is(sqlTypeName.getName() + "(3)"));
+assertThat(tsWithPrecision3.getFullTypeString(),
[GitHub] [calcite] danny0405 closed pull request #1916: [CALCITE-3924] Fix flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly
danny0405 closed pull request #1916: [CALCITE-3924] Fix flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly URL: https://github.com/apache/calcite/pull/1916 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] jinxing64 commented on issue #1775: [CALCITE-3758] FilterTableScanRule generate wrong mapping for filter …
jinxing64 commented on issue #1775: [CALCITE-3758] FilterTableScanRule generate wrong mapping for filter … URL: https://github.com/apache/calcite/pull/1775#issuecomment-613939989 hmm,, test failed, it seems to be flaky and working by https://github.com/apache/calcite/pull/1916 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[calcite] branch master updated: [CALCITE-3881] SqlFunctions#addMonths yields incorrect results in some corner case (Zhenghua Gao)
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 43261e4 [CALCITE-3881] SqlFunctions#addMonths yields incorrect
results in some corner case (Zhenghua Gao)
43261e4 is described below
commit 43261e4094a37ce23eb181a6a8f653dabc4db599
Author: Zhenghua Gao
AuthorDate: Mon Mar 30 16:26:38 2020 +0800
[CALCITE-3881] SqlFunctions#addMonths yields incorrect results in some
corner case (Zhenghua Gao)
SqlFunctions#addMonths use DateTimeUtils#ymdToUnixDate to calculate the
JDN(julian day number). But in some corner cases it yields incorrent
results. The root cause is: the algorithm of DateTimeUtils#ymdToUnixDate
requires reasonable month(1 to 12)[1], but SqlFunctions#addMonths may
pass in a month out of the reasonable range. This PR will fix it.
close apache/calcite#1890
---
.../java/org/apache/calcite/runtime/SqlFunctions.java | 12 +---
.../apache/calcite/sql/test/SqlOperatorBaseTest.java| 17 +
.../java/org/apache/calcite/test/SqlFunctionsTest.java | 2 ++
3 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
index 43934f3..4b89963 100644
--- a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
+++ b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
@@ -2702,9 +2702,15 @@ public class SqlFunctions {
int y0 = (int) DateTimeUtils.unixDateExtract(TimeUnitRange.YEAR, date);
int m0 = (int) DateTimeUtils.unixDateExtract(TimeUnitRange.MONTH, date);
int d0 = (int) DateTimeUtils.unixDateExtract(TimeUnitRange.DAY, date);
-int y = m / 12;
-y0 += y;
-m0 += m - y * 12;
+m0 += m;
+int deltaYear = (int) DateTimeUtils.floorDiv(m0, 12);
+y0 += deltaYear;
+m0 = (int) DateTimeUtils.floorMod(m0, 12);
+if (m0 == 0) {
+ y0 -= 1;
+ m0 += 12;
+}
+
int last = lastDay(y0, m0);
if (d0 > last) {
d0 = last;
diff --git
a/core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
b/core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
index 638682f..b631878 100644
--- a/core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
+++ b/core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
@@ -1990,6 +1990,9 @@ public abstract class SqlOperatorBaseTest {
tester.checkScalar("{fn TIMESTAMPDIFF(HOUR,"
+ " TIMESTAMP '2014-03-29 12:34:56',"
+ " TIMESTAMP '2014-03-29 12:34:56')}", "0", "INTEGER NOT NULL");
+tester.checkScalar("{fn TIMESTAMPDIFF(MONTH,"
++ " TIMESTAMP '2019-09-01 00:00:00',"
++ " TIMESTAMP '2020-03-01 00:00:00')}", "6", "INTEGER NOT NULL");
if (Bug.CALCITE_2539_FIXED) {
tester.checkFails("{fn WEEK(DATE '2014-12-10')}",
@@ -8283,6 +8286,14 @@ public abstract class SqlOperatorBaseTest {
+ "timestamp '2014-02-24 12:42:25', "
+ "timestamp '2016-02-24 12:42:25')",
"24", "INTEGER NOT NULL");
+tester.checkScalar("timestampdiff(MONTH, "
++ "timestamp '2019-09-01 00:00:00', "
++ "timestamp '2020-03-01 00:00:00')",
+"6", "INTEGER NOT NULL");
+tester.checkScalar("timestampdiff(MONTH, "
++ "timestamp '2019-09-01 00:00:00', "
++ "timestamp '2016-08-01 00:00:00')",
+"-37", "INTEGER NOT NULL");
tester.checkScalar("timestampdiff(QUARTER, "
+ "timestamp '2014-02-24 12:42:25', "
+ "timestamp '2016-02-24 12:42:25')",
@@ -8305,6 +8316,12 @@ public abstract class SqlOperatorBaseTest {
"timestampdiff(MONTH, date '2016-03-15', date '2016-06-14')",
"2",
"INTEGER NOT NULL");
+tester.checkScalar("timestampdiff(MONTH, date '2019-09-01', date
'2020-03-01')",
+"6",
+"INTEGER NOT NULL");
+tester.checkScalar("timestampdiff(MONTH, date '2019-09-01', date
'2016-08-01')",
+"-37",
+"INTEGER NOT NULL");
tester.checkScalar(
"timestampdiff(DAY, date '2016-06-15', date '2016-06-14')",
"-1",
diff --git a/core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java
b/core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java
index 75779b1..bb494d4 100644
--- a/core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java
+++ b/core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java
@@ -298,6 +298,8 @@ class SqlFunctionsTest {
checkAddMonths(2016, 3, 31, 2016, 2, 29, -1);
checkAddMonths(2016, 3, 31, 2116, 3, 31, 1200);
checkAddMonths(2016, 2, 28, 2116, 2, 28, 1200);
+checkAddMonths(2019, 9, 1, 2020, 3, 1, 6);
+checkAddMonths(2019, 9, 1, 2016, 8, 1, -37);
}
private void checkAddMonths(int y0, int
[GitHub] [calcite] danny0405 closed pull request #1890: [CALCITE-3881] SqlFunctions#addMonths yields incorrect results in som…
danny0405 closed pull request #1890: [CALCITE-3881] SqlFunctions#addMonths yields incorrect results in som… URL: https://github.com/apache/calcite/pull/1890 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1916: [CALCITE-3924] Fix flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly
neoremind commented on a change in pull request #1916: [CALCITE-3924] Fix
flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly
URL: https://github.com/apache/calcite/pull/1916#discussion_r408653713
##
File path: core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java
##
@@ -180,8 +180,10 @@ protected void generateTypeString(StringBuilder sb,
boolean withDetail) {
boolean printScale = scale != SCALE_NOT_SPECIFIED;
// for the digest, print the precision when defaulted,
-// since (for instance) TIME is equivalent to TIME(0).
-if (withDetail) {
+// but for TIME or TIMESTAMP, although (for instance)
+// TIME is equivalent to TIME(0), the digest of TIME
+// and TIME(0) should be different literally.
+if (withDetail && !SqlTypeName.DATETIME_TYPES.contains(typeName)) {
// -1 means there is no default value for precision
Review comment:
comment addressed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 opened a new pull request #1917: [CALCITE-3910] Enhance ProjectJoinTransposeRule to support SemiJoin and AntiJoin
liyafan82 opened a new pull request #1917: [CALCITE-3910] Enhance
ProjectJoinTransposeRule to support SemiJoin and AntiJoin
URL: https://github.com/apache/calcite/pull/1917
Currently, ProjectJoinTransposeRule does not support push project pass
SemiJoin and AntiJoin.
```
public void onMatch(RelOptRuleCall call) {
Project origProj = call.rel(0);
final Join join = call.rel(1);
if (!join.getJoinType().projectsRight()) {
return; // TODO: support SemiJoin / AntiJoin
}
...
...
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1916: [CALCITE-3924] Fix flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly
neoremind commented on a change in pull request #1916: [CALCITE-3924] Fix
flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly
URL: https://github.com/apache/calcite/pull/1916#discussion_r408625051
##
File path: core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java
##
@@ -180,8 +180,10 @@ protected void generateTypeString(StringBuilder sb,
boolean withDetail) {
boolean printScale = scale != SCALE_NOT_SPECIFIED;
// for the digest, print the precision when defaulted,
-// since (for instance) TIME is equivalent to TIME(0).
-if (withDetail) {
+// but for TIME or TIMESTAMP, although (for instance)
+// TIME is equivalent to TIME(0), the digest of TIME
+// and TIME(0) should be different literally.
+if (withDetail && !SqlTypeName.DATETIME_TYPES.contains(typeName)) {
// -1 means there is no default value for precision
Review comment:
The code is very old, back to the first commit in 2012, considering minimum
change, I add this check. Disabling `canonize` for some types with conditions
would also add more logic. So I am thinking that removing the whole condition
block (all unit tests will pass) would be a good solution.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] chris-baynes commented on issue #618: [CALCITE-2157] ClickHouse dialect implementation.
chris-baynes commented on issue #618: [CALCITE-2157] ClickHouse dialect implementation. URL: https://github.com/apache/calcite/pull/618#issuecomment-613856490 @danny0405 check style is fixed This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1915: [CALCITE-3921] Support serialize TableModify to json string and deserialize from it
danny0405 commented on a change in pull request #1915: [CALCITE-3921] Support
serialize TableModify to json string and deserialize from it
URL: https://github.com/apache/calcite/pull/1915#discussion_r408613744
##
File path:
core/src/main/java/org/apache/calcite/rel/externalize/RelJsonReader.java
##
@@ -199,6 +199,9 @@ public boolean getBoolean(String tag, boolean default_) {
public List getExpressionList(String tag) {
@SuppressWarnings("unchecked")
final List jsonNodes = (List) jsonRel.get(tag);
+if (jsonNodes == null) {
+ return null;
Review comment:
When the `jsonNodes` could be null ?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1916: [CALCITE-3924] Fix flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly
danny0405 commented on a change in pull request #1916: [CALCITE-3924] Fix
flakey test to handle TIMESTAMP and TIMESTAMP(0) correctly
URL: https://github.com/apache/calcite/pull/1916#discussion_r408612776
##
File path: core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java
##
@@ -180,8 +180,10 @@ protected void generateTypeString(StringBuilder sb,
boolean withDetail) {
boolean printScale = scale != SCALE_NOT_SPECIFIED;
// for the digest, print the precision when defaulted,
-// since (for instance) TIME is equivalent to TIME(0).
-if (withDetail) {
+// but for TIME or TIMESTAMP, although (for instance)
+// TIME is equivalent to TIME(0), the digest of TIME
+// and TIME(0) should be different literally.
+if (withDetail && !SqlTypeName.DATETIME_TYPES.contains(typeName)) {
// -1 means there is no default value for precision
Review comment:
The whole if block should be removed. Not only the `DATETIME_TYPES ` has
this problem, all the types that allows precision could have such a problem,
the root cause is that `#toString` and `digest` has different handling of
printing non specified precision.
One way to solve is to not fire caching of types without precision and with
default precision.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
