Michael Smith 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 16: (10 comments) http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java: http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java@209 PS16, Line 209: * Instead, a Project RelNode is placed as an input the Aggregate RelNode. The Project grammar: as an input to the Aggregate http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java@614 PS16, Line 614: if (!operandTypes.get(i).getSqlTypeName().equals( This conditional is used several places. It'd be nice to pull it out into a more readable helper function. http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedCastExpr.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedCastExpr.java: http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedCastExpr.java@27 PS16, Line 27: * TODO: need comment TODO? http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java: http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java@72 PS16, Line 72: .add(SqlKind.MOD) You added MOD here but no mapping to Impala func. Is it usable? http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java@90 PS16, Line 90: public static Function getFunction(String name, SqlKind kind, You could avoid a bunch of changes to other files by adding shorter signatures that assume exactMatch. Is exactMatch the dominant form, with just a few cases where we want nonstrict-supertype? http://gerrit.cloudera.org:8080/#/c/21335/16/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/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java@36 PS16, Line 36: public class ImplicitTypeChecker { Impala's Type class defines a lot of compatibility rules already. Could we use them? http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java@65 PS16, Line 65: .add(Pair.of(SqlTypeName.TINYINT, SqlTypeName.SMALLINT)) TINYINT to larger numeric types is repeated? http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java@91 PS16, Line 91: .add(Pair.of(SqlTypeName.CHAR, SqlTypeName.DATE)) Can CHAR be implicitly cast to VARCHAR? I was pretty sure it can. IMPALA-10086 has some related history. http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java: http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@372 PS16, Line 372: rdt =getRelDataType(Type.INT); nit: add a space after '=' http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java: http://gerrit.cloudera.org:8080/#/c/21335/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java@216 PS16, Line 216: if (t1.equals(Type.TIMESTAMP)) { nit: I'd collapse t1.equals || t2.equals into a single conditional. -- 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: 16 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: Tue, 10 Sep 2024 22:53:48 +0000 Gerrit-HasComments: Yes
