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 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 > Done I don't see any changes uploaded here. 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); > I moved this into a separate function as you suggested, but I also simplifi Ack 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 > Done Ack 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) > There are some tests in calcite.test that use "%". The difference for mod Can you add a comment explaining that? 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 > Made a change, not sure if it was exactly what you meant. I meant adding public static Function getFunction(RexCall call) { return getFunction(call, true); } I don't think duplicating the logic and descriptions is worth it in this change. 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 { > This is for Calcite types though. In this case, I think I would rather have Ack 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)) > Done Ack 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(); > I'll put it in there now. It'll blow up later if there's a problem with it Ack 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); > Done Ack 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)) { > Done Ack -- 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: Thu, 12 Sep 2024 21:10:23 +0000 Gerrit-HasComments: Yes
