You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "liuyongvs (via GitHub)" <gi...@apache.org> on 2023/05/05 12:34:57 UTC
[GitHub] [calcite] liuyongvs opened a new pull request, #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
liuyongvs opened a new pull request, #3189:
URL: https://github.com/apache/calcite/pull/3189
Cast expr to target type should respect nullable when it is complex type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1199589829
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -7033,7 +7036,7 @@ void testGroupExpressionEquivalenceParams() {
sql("select cast(a as row(f0 int, f1 varchar)) from COMPLEXTYPES.CTC_T1")
.withExtendedCatalog()
.columnType("RecordType(INTEGER NOT NULL F0, VARCHAR NOT NULL F1) NOT NULL");
- sql("select cast(b as row(f0 int not null, f1 varchar null))\n"
+ sql("select cast(b as row(f0 int, f1 varchar))\n"
Review Comment:
sounds good to me, one commit should focus on one thing
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1189290615
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
yes @libenchao
for exmpale:
a type is varchar not null
cast(a as timestamp) throw exception
safe_cast/try_cast(a as timestamp) return null, the return type is varchar, which can be nullable.
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1187345055
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
Review Comment:
i does it, becasue other place does it too, i will change 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] liuyongvs commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1539570631
hi @libenchao how do i format code with command to fix checkstyle problem
[Task :core:checkstyleMain] [LineLength] Line is longer than 100 characters (found 118).
i does it but not take effect
$ ./gradlew style # update code formatting (for auto-correctable cases) and verify style
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1188207621
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6984,9 +6984,9 @@ void testGroupExpressionEquivalenceParams() {
@Test void testCastAsCollectionType() {
sql("select cast(array[1,null,2] as int array) from (values (1))")
- .columnType("INTEGER NOT NULL ARRAY NOT NULL");
+ .columnType("INTEGER ARRAY NOT NULL");
Review Comment:
add test for multiset types and row.
because calcite doesn't support map type in parser, so i doesn't add.
```
java.lang.RuntimeException: Error while parsing query: select cast(map['washington', 1, null, 44] as map[CHAR(10), INTEGER NOT NULL>] from (values (1))
at org.apache.calcite.sql.test.AbstractSqlTester.parseAndValidate(AbstractSqlTester.java:157)
at org.apache.calcite.sql.test.AbstractSqlTester.validateAndThen(AbstractSqlTester.java:248)
at org.apache.calcite.sql.test.AbstractSqlTester.checkColumnType(AbstractSqlTester.java:177)
at org.apache.calcite.test.SqlValidatorFixture.columnType(
```
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1199589589
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -7033,7 +7036,7 @@ void testGroupExpressionEquivalenceParams() {
sql("select cast(a as row(f0 int, f1 varchar)) from COMPLEXTYPES.CTC_T1")
.withExtendedCatalog()
.columnType("RecordType(INTEGER NOT NULL F0, VARCHAR NOT NULL F1) NOT NULL");
- sql("select cast(b as row(f0 int not null, f1 varchar null))\n"
+ sql("select cast(b as row(f0 int, f1 varchar))\n"
Review Comment:
just for test row and i fallback it.
then i will submit new pr to fix cast( type not null/ null) latter according sql standard, 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1551270611
@liuyongvs There is one unresolved comment https://github.com/apache/calcite/pull/3189#discussion_r1187312092
--
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] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1552837488
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [73.1% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] chunweilei commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "chunweilei (via GitHub)" <gi...@apache.org>.
chunweilei commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1188262158
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ private static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isNullable = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
+ if (isArray(expressionType)) {
+ return SqlTypeUtil.createArrayType(typeFactory, newElementType, isNullable);
+ } else {
+ return SqlTypeUtil.createMultisetType(typeFactory, newElementType, isNullable);
Review Comment:
Maybe we can use `isArray(expressionType) ? xxx: yyy` to be more concise.
--
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] liuyongvs commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1545009658
hi @libenchao @chunweilei ci passed, do you have time to look 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1539993619
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [81.8% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1539571628
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [85.7% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] libenchao commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1189294026
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
This is not what I meant to say.
Take a example, `ARRAY<VARCHAR NOT NULL> NOT NULL`, safe_cast it to `ARRAY<INT NULLABLE> NULLABLE`, what should the result be? IMO, the inner type should be `NULLABLE` because it may produce `null`s.
--
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] libenchao commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1187308259
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
Review Comment:
There is no need to expose it as `public` for now.
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
Review Comment:
`isn` is not a good name, how about name it `isNullable` or `nullable`?
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6984,9 +6984,9 @@ void testGroupExpressionEquivalenceParams() {
@Test void testCastAsCollectionType() {
sql("select cast(array[1,null,2] as int array) from (values (1))")
- .columnType("INTEGER NOT NULL ARRAY NOT NULL");
+ .columnType("INTEGER ARRAY NOT NULL");
Review Comment:
Could you also try to add tests for map/row/multiset types?
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
The third parameter should be `safe`?
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
+ if (isArray(expressionType)) {
+ return SqlTypeUtil.createArrayType(typeFactory, newElementType, isn);
+ } else {
+ return SqlTypeUtil.createMultisetType(typeFactory, newElementType, isn);
+ }
+ }
+
+ if (isRow(expressionType)) {
+ int fieldCount = expressionType.getFieldCount();
Review Comment:
We usually add `final` to variables when it's possible.
--
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] libenchao commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1188538948
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
I'm not sure about this, if the inner type is `not null`, then safe cast should be `nullable` because the cast may produce `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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao closed pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao closed pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
URL: https://github.com/apache/calcite/pull/3189
--
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] libenchao commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1197472520
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
because in safe mode, we'll produce `null`s, maybe a more concrete example explains this better: "['123', '456', 'abc']", if we cast it to `array<int>`, what should the result be? "[123, 456, null]", or "null" ? I don't know what other databases behaves in this case, but in my opinion, the former one is better. If this is also what other databases which provide safe cast be behaves, then the type for element type should also be nullable.
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1197508002
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
after dig the code of spark, you are right, thanks.
I intuitively thought it was the latter.
```
test("element type nullability") {
val array = Literal.create(Seq("123", "true"),
ArrayType(StringType, containsNull = false))
// array element can be null after try_cast which violates the target type.
val c1 = cast(array, ArrayType(BooleanType, containsNull = false))
assert(!c1.resolved)
val map = Literal.create(Map("a" -> "123", "b" -> "true"),
MapType(StringType, StringType, valueContainsNull = false))
// key can be null after try_cast which violates the map key requirement.
val c2 = cast(map, MapType(IntegerType, StringType, valueContainsNull = true))
assert(!c2.resolved)
// map value can be null after try_cast which violates the target type.
val c3 = cast(map, MapType(StringType, IntegerType, valueContainsNull = false))
assert(!c3.resolved)
val struct = Literal.create(
InternalRow(
UTF8String.fromString("123"),
UTF8String.fromString("true")),
new StructType()
.add("a", StringType, nullable = true)
.add("b", StringType, nullable = true))
// struct field `b` can be null after try_cast which violates the target type.
val c4 = cast(struct, new StructType()
.add("a", BooleanType, nullable = true)
.add("b", BooleanType, nullable = false))
assert(!c4.resolved)
}
```
--
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] liuyongvs commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1539561900
hi @libenchao fix all your reviews, please have a look 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1542959192
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [78.7% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1188499805
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ private static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isNullable = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
+ if (isArray(expressionType)) {
+ return SqlTypeUtil.createArrayType(typeFactory, newElementType, isNullable);
+ } else {
+ return SqlTypeUtil.createMultisetType(typeFactory, newElementType, isNullable);
Review Comment:
fixed @chunweilei
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1188211715
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6984,9 +6984,9 @@ void testGroupExpressionEquivalenceParams() {
@Test void testCastAsCollectionType() {
sql("select cast(array[1,null,2] as int array) from (values (1))")
- .columnType("INTEGER NOT NULL ARRAY NOT NULL");
+ .columnType("INTEGER ARRAY NOT NULL");
Review Comment:
add this grammer should not support select cast(b as row(f0 int not null, f1 varchar null) , i will fix it a new pr latter
sql standard below
```
<cast specification> ::=
CAST <left paren> <cast operand> AS <cast target> <right paren>
<cast operand> ::=
<value expression>
| <implicitly typed value specification>
<cast target> ::=
<domain name>
| <data type>
<data type> ::=
<predefined type>
| <row type>
| <path-resolved user-defined type name>
| <reference type>
| <collection type>
<row type> ::=
ROW <row type body>
<row type body> ::=
<left paren> <field definition> [ { <comma> <field definition> }... ] <right paren>
<field definition> ::=
<field name> <data type>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1536197994
hi @libenchao ,do you have time to review 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] liuyongvs commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1552310755
@libenchao i am sorry for do not see your comments. i response to it just now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liuyongvs commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1552312813
squash the commits
--
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] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1536250107
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [85.7% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] libenchao commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1199578965
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +130,62 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ private static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isNullable = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ requireNonNull(expressionElementType, () -> "componentType of " + expressionType);
+ requireNonNull(targetElementType, () -> "componentType of " + targetType);
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(
+ typeFactory, expressionElementType, targetElementType, safe);
+ return isArray(expressionType)
+ ? SqlTypeUtil.createArrayType(typeFactory, newElementType, isNullable)
+ : SqlTypeUtil.createMultisetType(typeFactory, newElementType, isNullable);
+ }
+
+ if (isRow(expressionType)) {
+ final int fieldCount = expressionType.getFieldCount();
+ List<RelDataType> typeList = new ArrayList<>();
Review Comment:
NIT: add `final`, and initialize `ArrayList` with `fieldCount`
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -7033,7 +7036,7 @@ void testGroupExpressionEquivalenceParams() {
sql("select cast(a as row(f0 int, f1 varchar)) from COMPLEXTYPES.CTC_T1")
.withExtendedCatalog()
.columnType("RecordType(INTEGER NOT NULL F0, VARCHAR NOT NULL F1) NOT NULL");
- sql("select cast(b as row(f0 int not null, f1 varchar null))\n"
+ sql("select cast(b as row(f0 int, f1 varchar))\n"
Review Comment:
why are you changing this test and the below one?
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +130,62 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ private static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isNullable = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ requireNonNull(expressionElementType, () -> "componentType of " + expressionType);
+ requireNonNull(targetElementType, () -> "componentType of " + targetType);
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(
+ typeFactory, expressionElementType, targetElementType, safe);
+ return isArray(expressionType)
+ ? SqlTypeUtil.createArrayType(typeFactory, newElementType, isNullable)
+ : SqlTypeUtil.createMultisetType(typeFactory, newElementType, isNullable);
+ }
+
+ if (isRow(expressionType)) {
+ final int fieldCount = expressionType.getFieldCount();
+ List<RelDataType> typeList = new ArrayList<>();
+ for (int i = 0; i < fieldCount; ++i) {
+ RelDataType expressionElementType = expressionType.getFieldList().get(i).getType();
+ RelDataType targetElementType = targetType.getFieldList().get(i).getType();
+ typeList.add(
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType,
+ targetElementType, safe));
Review Comment:
NIT: add indent.
--
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] liuyongvs commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1537636688
hi @JiajunBernoulli do you also have time to review 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] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1542059036
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [78.7% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1542307080
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [5 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [78.7% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] liuyongvs commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1540024186
@libenchao 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1555873766
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [73.1% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1555876689
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [73.1% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1541205855
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [5 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [81.8% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1545000364
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [73.1% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1197634750
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
fixed and add some test for try_cast and safe_cast
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
fixed and add some test for try_cast and safe_cast @libenchao
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1197269263
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
@libenchao why?
1. cast target type should not contains nullable like your example ARRAY<INT NULLABLE> NULLABLE
2. the array type and element type is different
array type nullable( outer nullable), the value is null
element type nullable (inner nullable), the value is [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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1552325130
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3189)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3189&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3189&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list) [73.1% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3189&metric=new_duplicated_lines_density&view=list)
--
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] liuyongvs commented on a diff in pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #3189:
URL: https://github.com/apache/calcite/pull/3189#discussion_r1187344362
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java:
##########
@@ -122,8 +128,50 @@ static SqlReturnTypeInference returnTypeInference(boolean safe) {
/** Derives the type of "CAST(expression AS targetType)". */
public static RelDataType deriveType(RelDataTypeFactory typeFactory,
RelDataType expressionType, RelDataType targetType, boolean safe) {
- return typeFactory.createTypeWithNullability(targetType,
- expressionType.isNullable() || safe);
+ return createTypeWithNullabilityFromExpr(typeFactory, expressionType, targetType, safe);
+ }
+
+ public static RelDataType createTypeWithNullabilityFromExpr(RelDataTypeFactory typeFactory,
+ RelDataType expressionType, RelDataType targetType, boolean safe) {
+ boolean isn = expressionType.isNullable() || safe;
+
+ if (isCollection(expressionType)) {
+ RelDataType expressionElementType = expressionType.getComponentType();
+ RelDataType targetElementType = targetType.getComponentType();
+ RelDataType newElementType =
+ createTypeWithNullabilityFromExpr(typeFactory, expressionElementType, targetElementType, false);
Review Comment:
it should not. because the try_cast/safe_cast(different vendors, flink/spark former, mssql latter) will return null when it can not cast to targettype instead of throw exception like cast.
it only changes the outer nullable
--
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] libenchao commented on pull request #3189: [CALCITE-5674] Cast expr to target type should respect nullable when …
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on PR #3189:
URL: https://github.com/apache/calcite/pull/3189#issuecomment-1540007524
> hi @libenchao how do i format code with command to fix checkstyle problem automatic like flink using spotless plugin [Task :core:checkstyleMain] [LineLength] Line is longer than 100 characters (found 118).
>
> i does it but not take effect in https://calcite.apache.org/docs/howto.html#setting-up-an-ide-for-contributing $ ./gradlew style # update code formatting (for auto-correctable cases) and verify style
`./gradlew autostyleApply` should work.
--
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