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 17: (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 Done 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: RexNode newProject = rexBuilder.makeCast(newOperandTypes.get(i), inputRef); > This conditional is used several places. It'd be nice to pull it out into a I moved this into a separate function as you suggested, but I also simplified temporarily as well. I think there might be some test that breaks without the VARCHAR part, but I'll make that fix at a later time. 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: * A CastExpr that is always in analyzed state > TODO? Done 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? There are some tests in calcite.test that use "%". The difference for mod is Calcite knows that "%" is the same as "mod", whereas "+" does not map to "add" 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: // For arithmetic types, we use separate logic. Calcite has already > You could avoid a bunch of changes to other files by adding shorter signatu Made a change, not sure if it was exactly what you meant. 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 This is for Calcite types though. In this case, I think I would rather have an explicit Calcite type match rather than do conversions to Impala types and then compare. 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.SMALLINT, SqlTypeName.INTEGER)) > TINYINT to larger numeric types is repeated? Done 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: .build(); > Can CHAR be implicitly cast to VARCHAR? I was pretty sure it can. IMPALA-10 I'll put it in there now. It'll blow up later if there's a problem with it. 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 '=' Done 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) || t2.equals(Type.TIMESTAMP)) { > nit: I'd collapse t1.equals || t2.equals into a single conditional. Done -- 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: 17 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, 11 Sep 2024 22:58:25 +0000 Gerrit-HasComments: Yes
