You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/11/20 09:06:25 UTC
[GitHub] [calcite] JiajunBernoulli opened a new pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
JiajunBernoulli opened a new pull request #2615:
URL: https://github.com/apache/calcite/pull/2615
# insert union case
Here is the sql:
```
insert into t3
select 'a', 1.0, 1 union
select 'b', 2, 2 union
select 'c', 3.0, CAST(3 AS SMALLINT) union
select 'd', 4.0, 4 union
select 'e', 5.0, 5
```
but the validated sql is :
```
INSERT INTO `CATALOG`.`SALES`.`T3`
SELECT 'a', CAST(1.0 AS INTEGER), CAST(1 AS SMALLINT) UNION
SELECT 'b', 2, CAST(2 AS SMALLINT) UNION -- Encountered an integer and ended early
SELECT 'c', 3.0, CAST(3 AS SMALLINT) UNION -- Encountered an samlint and ended early
SELECT 'd', 4.0, 4 UNION -- should be cast
SELECT 'e', 5.0, 5 -- should be cast
```
This is why CALCITE-4458 changed '&&' to '||', the right validated sql is:
```
INSERT INTO `CATALOG`.`SALES`.`T3`
SELECT 'a', CAST(1.0 AS INTEGER), CAST(1 AS SMALLINT)
UNION
SELECT 'b', 2, CAST(2 AS SMALLINT)
UNION
SELECT 'c', CAST(3.0 AS INTEGER), CAST(3 AS SMALLINT)
UNION
SELECT 'd', CAST(4.0 AS INTEGER), CAST(4 AS SMALLINT)
UNION
SELECT 'e', CAST(5.0 AS INTEGER), CAST(5 AS SMALLINT)
```
# case2
Here is the sql
```
INSERT INTO `CATALOG`.`SALES`.`T3`
SELECT 'a', CAST(1.0 AS INTEGER), CAST(1 AS SMALLINT)
UNION
SELECT 'b', 2, CAST(2 AS SMALLINT)
UNION
SELECT 'c', 3.0, CAST(3 AS SMALLINT)
UNION
SELECT 'd', 4.0, 4
UNION
SELECT 'e', 5.0, 5
```
the validated sql is :
```
INSERT INTO `CATALOG`.`SALES`.`T3`
VALUES ROW('a', CAST(1.0 AS INTEGER), CAST(1 AS SMALLINT)),
ROW('b', 2, CAST(2 AS SMALLINT)),-- Encountered an integer and ended early
ROW('c', 3.0, CAST(3 AS SMALLINT)),Encountered an samllint and ended early ROW('d', 4.0, 4),
ROW('e', 5.0, 5)
```
the right validated sql is:
```
INSERT INTO `CATALOG`.`SALES`.`T3`
SELECT 'a', CAST(1.0 AS INTEGER), CAST(1 AS SMALLINT)
UNION
SELECT 'b', 2, CAST(2 AS SMALLINT)
UNION
SELECT 'c', CAST(3.0 AS INTEGER), CAST(3 AS SMALLINT)
UNION
SELECT 'd', CAST(4.0 AS INTEGER), CAST(4 AS SMALLINT)
UNION
SELECT 'e', CAST(5.0 AS INTEGER), CAST(5 AS SMALLINT)
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757753786
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
##########
@@ -894,6 +894,13 @@ public MockCatalogReader init() {
t2.addColumn("t2_binary", binaryType);
t2.addColumn("t2_boolean", booleanType);
registerTable(t2);
+
+ final MockTable t3 =
+ MockTable.create(this, tSchema, "T3", false, 7.0, null);
+ t3.addColumn("t3_varchar20", varchar20Type);
+ t3.addColumn("t3_int", intType);
Review comment:
I'm worried that too **many columns** will affect everyone's reading, so I only use **a few columns.** If you think it is more appropriate to use the existing table, I can change it later.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757753873
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -121,8 +124,8 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
// Set operations are binary for now.
final SqlCall operand0 = ((SqlCall) query).operand(0);
final SqlCall operand1 = ((SqlCall) query).operand(1);
- final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
- && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+ boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType);
Review comment:
This will not fix the problem. Because when `rowTypeCoercion(scope, operand0, columnIndex, targetType)=true`, `rowTypeCoercion(scope, operand1, columnIndex, targetType) `will not be run.
I will use `|`, do you agree?
```
final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
| rowTypeCoercion(scope, operand1, columnIndex, targetType);
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757858760
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -133,4 +172,24 @@
private void checkPlanEquals(String sql) {
tester.assertConvertsTo(sql, "${plan}");
}
+
+ private void checkValidateSqlEquals(String sql) {
+ Objects.requireNonNull(sql, "sql");
+ final SqlNode sqlQuery;
+ try {
+ sqlQuery = tester.parseQuery(sql);
+ } catch (RuntimeException | Error e) {
+ throw e;
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
Review comment:
Consider using `catch(Throwable e) { TestUtil.rethrow(e, "sql=" + sql) }`
That would be shorter, it would catch all throwables, and it would add the SQL in question
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -133,4 +162,24 @@
private void checkPlanEquals(String sql) {
tester.assertConvertsTo(sql, "${plan}");
}
+
+ private void checkValidateSqlEquals(String sql) {
+ Objects.requireNonNull(sql, "sql");
+ final SqlNode sqlQuery;
+ try {
+ sqlQuery = tester.parseQuery(sql);
+ } catch (RuntimeException | Error e) {
+ throw e;
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
+ }
+ final RelDataTypeFactory typeFactory = tester.getValidator().getTypeFactory();
+ final Prepare.CatalogReader catalogReader =
+ tester.createCatalogReader(typeFactory);
+ final SqlValidator validator =
+ tester.createValidator(catalogReader, typeFactory);
+
+ final String actual = validator.validate(sqlQuery).toString();
Review comment:
@JiajunBernoulli , have you seen https://lists.apache.org/thread/3l9y4fv31kthmpn9pbzqdmfp80yrjphf by @julianhyde ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757863100
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -133,4 +162,24 @@
private void checkPlanEquals(String sql) {
tester.assertConvertsTo(sql, "${plan}");
}
+
+ private void checkValidateSqlEquals(String sql) {
+ Objects.requireNonNull(sql, "sql");
+ final SqlNode sqlQuery;
+ try {
+ sqlQuery = tester.parseQuery(sql);
+ } catch (RuntimeException | Error e) {
+ throw e;
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
+ }
+ final RelDataTypeFactory typeFactory = tester.getValidator().getTypeFactory();
+ final Prepare.CatalogReader catalogReader =
+ tester.createCatalogReader(typeFactory);
+ final SqlValidator validator =
+ tester.createValidator(catalogReader, typeFactory);
+
+ final String actual = validator.validate(sqlQuery).toString();
Review comment:
I didn't know before. I will continue to pay attention to it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757853067
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -121,8 +124,8 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
// Set operations are binary for now.
final SqlCall operand0 = ((SqlCall) query).operand(0);
final SqlCall operand1 = ((SqlCall) query).operand(1);
- final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
- && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+ boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType);
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757852924
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
##########
@@ -894,6 +894,13 @@ public MockCatalogReader init() {
t2.addColumn("t2_binary", binaryType);
t2.addColumn("t2_boolean", booleanType);
registerTable(t2);
+
+ final MockTable t3 =
+ MockTable.create(this, tSchema, "T3", false, 7.0, null);
+ t3.addColumn("t3_varchar20", varchar20Type);
+ t3.addColumn("t3_int", intType);
Review comment:
I deleted `t3` and used `t1`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757854332
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -133,4 +162,24 @@
private void checkPlanEquals(String sql) {
tester.assertConvertsTo(sql, "${plan}");
}
+
+ private void checkValidateSqlEquals(String sql) {
+ Objects.requireNonNull(sql, "sql");
+ final SqlNode sqlQuery;
+ try {
+ sqlQuery = tester.parseQuery(sql);
+ } catch (RuntimeException | Error e) {
+ throw e;
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
+ }
+ final RelDataTypeFactory typeFactory = tester.getValidator().getTypeFactory();
+ final Prepare.CatalogReader catalogReader =
+ tester.createCatalogReader(typeFactory);
+ final SqlValidator validator =
+ tester.createValidator(catalogReader, typeFactory);
+
+ final String actual = validator.validate(sqlQuery).toString();
Review comment:
I'm interested in it. `checkValidateSqlEquals` function can be abstracted.
Do you think it's more appropriate to put it in the parent class `SqlToRelTestBase` or create an interface `SqlToValidateTestBase`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r762520115
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -121,8 +122,9 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
// Set operations are binary for now.
final SqlCall operand0 = ((SqlCall) query).operand(0);
final SqlCall operand1 = ((SqlCall) query).operand(1);
- final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
- && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+ @SuppressWarnings("ShortCircuitBoolean")
+ boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
+ | rowTypeCoercion(scope, operand1, columnIndex, targetType);
// Update the nested SET operator node type.
Review comment:
I didn't find a more suitable way, just modify the original 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757862951
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -133,4 +172,24 @@
private void checkPlanEquals(String sql) {
tester.assertConvertsTo(sql, "${plan}");
}
+
+ private void checkValidateSqlEquals(String sql) {
+ Objects.requireNonNull(sql, "sql");
+ final SqlNode sqlQuery;
+ try {
+ sqlQuery = tester.parseQuery(sql);
+ } catch (RuntimeException | Error e) {
+ throw e;
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
Review comment:
I have modified it, thank you very much for your advice.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r762502902
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -121,8 +122,9 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
// Set operations are binary for now.
final SqlCall operand0 = ((SqlCall) query).operand(0);
final SqlCall operand1 = ((SqlCall) query).operand(1);
- final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
- && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+ @SuppressWarnings("ShortCircuitBoolean")
+ boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
+ | rowTypeCoercion(scope, operand1, columnIndex, targetType);
// Update the nested SET operator node type.
Review comment:
We should not use `|` in the logical operator.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757853246
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -104,14 +104,17 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
updateInferredColumnType(scope1, query, columnIndex, targetType);
return true;
case VALUES:
+ boolean coercedValues = true;
for (SqlNode rowConstructor : ((SqlCall) query).getOperandList()) {
if (!coerceOperandType(scope, (SqlCall) rowConstructor, columnIndex, targetType)) {
- return false;
+ coercedValues = false;
}
}
- updateInferredColumnType(
- requireNonNull(scope, "scope"), query, columnIndex, targetType);
- return true;
+ if (coercedValues) {
+ updateInferredColumnType(
+ requireNonNull(scope, "scope"), query, columnIndex, targetType);
Review comment:
I modified the expected values of failed unit tests. They should all be not null. Are there any other questions?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] beyond1920 edited a comment on pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
beyond1920 edited a comment on pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#issuecomment-979888627
@JiajunBernoulli Thanks for contribution.
LGTM. I only left a minor 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757834655
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -104,14 +104,17 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
updateInferredColumnType(scope1, query, columnIndex, targetType);
return true;
case VALUES:
+ boolean coercedValues = true;
for (SqlNode rowConstructor : ((SqlCall) query).getOperandList()) {
if (!coerceOperandType(scope, (SqlCall) rowConstructor, columnIndex, targetType)) {
- return false;
+ coercedValues = false;
}
}
- updateInferredColumnType(
- requireNonNull(scope, "scope"), query, columnIndex, targetType);
- return true;
+ if (coercedValues) {
+ updateInferredColumnType(
+ requireNonNull(scope, "scope"), query, columnIndex, targetType);
Review comment:
> org.opentest4j.AssertionFailedError: expected: <RecordType(VARCHAR EXPR$0) NOT NULL>
but was: <RecordType(VARCHAR NOT NULL EXPR$0) NOT NULL>
Shouldn't this be the correct behavior ? Two constant literals are type nullable false, and implicit type coercion does not solve nullability in-consistency.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757834749
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -121,8 +124,8 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
// Set operations are binary for now.
final SqlCall operand0 = ((SqlCall) query).operand(0);
final SqlCall operand1 = ((SqlCall) query).operand(1);
- final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
- && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+ boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType);
Review comment:
Yeah, you code is correct, we should not add any short-cut logic here.
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -133,4 +162,24 @@
private void checkPlanEquals(String sql) {
tester.assertConvertsTo(sql, "${plan}");
}
+
+ private void checkValidateSqlEquals(String sql) {
+ Objects.requireNonNull(sql, "sql");
+ final SqlNode sqlQuery;
+ try {
+ sqlQuery = tester.parseQuery(sql);
+ } catch (RuntimeException | Error e) {
+ throw e;
+ } catch (Exception e) {
+ throw TestUtil.rethrow(e);
+ }
+ final RelDataTypeFactory typeFactory = tester.getValidator().getTypeFactory();
+ final Prepare.CatalogReader catalogReader =
+ tester.createCatalogReader(typeFactory);
+ final SqlValidator validator =
+ tester.createValidator(catalogReader, typeFactory);
+
+ final String actual = validator.validate(sqlQuery).toString();
Review comment:
Somehow we need some refactoring for this testing infrustructure, for e.g, a `Sql` util, we can do that in another PR.
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
##########
@@ -894,6 +894,13 @@ public MockCatalogReader init() {
t2.addColumn("t2_binary", binaryType);
t2.addColumn("t2_boolean", booleanType);
registerTable(t2);
+
+ final MockTable t3 =
+ MockTable.create(this, tSchema, "T3", false, 7.0, null);
+ t3.addColumn("t3_varchar20", varchar20Type);
+ t3.addColumn("t3_int", intType);
Review comment:
Yeah, i'm sliding to reuse the original tables.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757749317
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -104,14 +104,17 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
updateInferredColumnType(scope1, query, columnIndex, targetType);
return true;
case VALUES:
+ boolean coercedValues = true;
for (SqlNode rowConstructor : ((SqlCall) query).getOperandList()) {
if (!coerceOperandType(scope, (SqlCall) rowConstructor, columnIndex, targetType)) {
- return false;
+ coercedValues = false;
}
}
- updateInferredColumnType(
- requireNonNull(scope, "scope"), query, columnIndex, targetType);
- return true;
+ if (coercedValues) {
+ updateInferredColumnType(
+ requireNonNull(scope, "scope"), query, columnIndex, targetType);
Review comment:
What's the difference here ? The original code does the same behavior: `#updateInferredColumnType` only if all the operands are coerced.
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
##########
@@ -894,6 +894,13 @@ public MockCatalogReader init() {
t2.addColumn("t2_binary", binaryType);
t2.addColumn("t2_boolean", booleanType);
registerTable(t2);
+
+ final MockTable t3 =
+ MockTable.create(this, tSchema, "T3", false, 7.0, null);
+ t3.addColumn("t3_varchar20", varchar20Type);
+ t3.addColumn("t3_int", intType);
Review comment:
Why add a new table here ? 't1' and 't2' have all the data types you needed.
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -121,8 +124,8 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
// Set operations are binary for now.
final SqlCall operand0 = ((SqlCall) query).operand(0);
final SqlCall operand1 = ((SqlCall) query).operand(1);
- final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
- && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+ boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType);
Review comment:
Change `&&` directly to `||` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a change in pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#discussion_r757753872
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -104,14 +104,17 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
updateInferredColumnType(scope1, query, columnIndex, targetType);
return true;
case VALUES:
+ boolean coercedValues = true;
for (SqlNode rowConstructor : ((SqlCall) query).getOperandList()) {
if (!coerceOperandType(scope, (SqlCall) rowConstructor, columnIndex, targetType)) {
- return false;
+ coercedValues = false;
}
}
- updateInferredColumnType(
- requireNonNull(scope, "scope"), query, columnIndex, targetType);
- return true;
+ if (coercedValues) {
+ updateInferredColumnType(
+ requireNonNull(scope, "scope"), query, columnIndex, targetType);
Review comment:
I thought so at first. But many test will be failed, for example:
```
sql("select 1 from (values(true)) union values (select '1' from (values (true)) as tt)")
.type("RecordType(VARCHAR EXPR$0) NOT NULL");
```
```
org.opentest4j.AssertionFailedError: expected: <RecordType(VARCHAR EXPR$0) NOT NULL>
but was: <RecordType(VARCHAR NOT NULL EXPR$0) NOT NULL>
```
I think it is only executed when coerced values is true, like the following code
```
// Update the nested SET operator node type.
if (coerced) {
updateInferredColumnType(
requireNonNull(scope, "scope"), query, columnIndex, targetType);
}
```
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -121,8 +124,8 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
// Set operations are binary for now.
final SqlCall operand0 = ((SqlCall) query).operand(0);
final SqlCall operand1 = ((SqlCall) query).operand(1);
- final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
- && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+ boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType);
Review comment:
This will not fix the problem. Because when `rowTypeCoercion(scope, operand0, columnIndex, targetType)=false`, `rowTypeCoercion(scope, operand1, columnIndex, targetType) `will not be run.
I will use `|`, do you agree?
```
final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
| rowTypeCoercion(scope, operand1, columnIndex, targetType);
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] beyond1920 commented on pull request #2615: [CALCITE-4897] Fix incomplete implicit type cast
Posted by GitBox <gi...@apache.org>.
beyond1920 commented on pull request #2615:
URL: https://github.com/apache/calcite/pull/2615#issuecomment-979888627
@JiajunBernoulli LGTM. I only left a minor 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org