Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21335 )
Change subject: IMPALA-13022: Added infrastructure for implicit casting of functions ...................................................................... Patch Set 19: (5 comments) http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java: http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java@42 PS18, Line 42: private static Set<Pair<SqlTypeName, SqlTypeName>> SUPPORTED_IMPLICIT_CASTS = > I think this should be the same as strict compatibility. allow_unsafe_casts is lower priority. Since it is mainly about inserts, we can think about it later. On second look, this is more like default compatibility mode, and I think it makes sense for this to match default compatibility for now. The problem with this code structure where it stays at the Calcite level is that it is hard for me to confirm that this is exactly the same as the default compatibility mode. I assume we're aiming for default compatibility. We could drop down to Impala types and reuse our rules or we can write a unit test that confirms this is identical to Impala's default compatibility. I tried to review some of these entries myself, but it is too subtle. To get this right, we need a unit test. http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java@44 PS18, Line 44: .add(Pair.of(SqlTypeName.NULL, SqlTypeName.BOOLEAN)) : .add(Pair.of(SqlTypeName.NULL, SqlTypeName.TINYINT)) : .add(Pair.of(SqlTypeName.NULL, SqlTypeName.SMALLINT)) : .add(Pair.of(SqlTypeName.NULL, SqlTypeName.INTEGER)) : .add(Pair.of(SqlTypeName.NULL, SqlTypeName.BIGINT)) : .add(Pair.of(SqlTypeName.NULL, SqlTypeName.FLOAT)) : .add(Pair.of(SqlTypeName.NULL, SqlTypeName.DOUBLE)) : .add(Pair.of(SqlTypeName.NULL, SqlTypeName.VARCHAR)) : .add(Pair.of(SqlTypeName.NULL, SqlTypeName.CHAR)) : .add(Pair.of(SqlTypeName.NULL, SqlTypeName.TIMESTAMP)) Are these NULL entries needed? It looks like supportsImplicitCasting() always allow casts to/from NULL. (If they are needed, then what about DATE, DECIMAL, BINARY?) http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java@78 PS18, Line 78: .add(Pair.of(SqlTypeName.FLOAT, SqlTypeName.DECIMAL)) : .add(Pair.of(SqlTypeName.DOUBLE, SqlTypeName.DECIMAL)) Float and double can't currently be implicitly converted to Decimal. https://impala.apache.org/docs/build/plain-html/topics/impala_decimal.html http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java@83 PS18, Line 83: .add(Pair.of(SqlTypeName.CHAR, SqlTypeName.CHAR)) Do we need this CHAR to CHAR entry? http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java@90 PS18, Line 90: .add(Pair.of(SqlTypeName.DATE, SqlTypeName.TIMESTAMP)) I have suspicions about this one. -- To view, visit http://gerrit.cloudera.org:8080/21335 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13a349673f185463276ad7ddb83f0cfc2d73218c Gerrit-Change-Number: 21335 Gerrit-PatchSet: 19 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 18 Sep 2024 18:24:03 +0000 Gerrit-HasComments: Yes
