Steve Carlin 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 18: (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 = > allow_unsafe_casts is lower priority. Since it is mainly about inserts, we What kind of unit test are you thinking here? If it's not an e2e test, then I'm not sure how we guarantee the code will work unless we specifically code the unit tests to work with this structure, but that really isn't testing whether the entries are correct. 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() alwa Removed. The Null implicit conversion check isn't needed here. 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. So this is interesting... If I remove this? One of my unit tests in calcite.test breaks. Specifically, select float_col + 3.0 When I run this with the current Impala planner There is code in TypesUtil.getArithmeticResultType that looks like this: if (t1.isDecimal() || t2.isDecimal()) { ... t1 = ((ScalarType) t1).getMinResolutionDecimal(); t2 = ((ScalarType) t2).getMinResolutionDecimal(); Preconditions.checkState(t1.isDecimal()); Preconditions.checkState(t2.isDecimal()); return getDecimalArithmeticResultType(t1, t2, op, decimalV2); } ...and indeed, a CTAS using the original Impala planner creates a decimal column. So it at least has implicit conversion in one case? Should I special case this? 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? Removed. None of my tests uses this. If it comes up later, will re-add 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. Removed -- 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: 18 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 21:50:14 +0000 Gerrit-HasComments: Yes
