Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24218 )
Change subject: IMPALA-14912: Calcite Planner: Fix date string parsing ...................................................................... Patch Set 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/24218/7/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java File fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java: http://gerrit.cloudera.org:8080/#/c/24218/7/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java@61 PS7, Line 61: /** > nit: could use a javadoc. Done http://gerrit.cloudera.org:8080/#/c/24218/7/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java@64 PS7, Line 64: */ > nit: add newline after this Done http://gerrit.cloudera.org:8080/#/c/24218/7/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java@69 PS7, Line 69: > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/24218/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java: http://gerrit.cloudera.org:8080/#/c/24218/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java@113 PS7, Line 113: String funcName = rexCall.getOperator().getName().toLowerCase(); > I'm not clear why we have this and the check at line 143, which has equival Neither am I :) I did a refactor in a different branch and ported both versions. This has now been removed. http://gerrit.cloudera.org:8080/#/c/24218/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java@219 PS7, Line 219: return new AnalyzedFunctionCallExpr(fn, params, Type.STRING); > Any chance this could throw IllegalArgumentException? Yeah, just tested, and it will throw an exception if the string is not hex. Original planner returns an empty string in this case, and now Calcite does too. http://gerrit.cloudera.org:8080/#/c/24218/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java: http://gerrit.cloudera.org:8080/#/c/24218/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java@128 PS7, Line 128: StringLiteral stringLiteral = new StringLiteral(timestamp, Type.STRING, false); > Why does this need unescaping? Changed it for consistency, but it doesn't need to. I changed it back. -- To view, visit http://gerrit.cloudera.org:8080/24218 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia08071193f86423a0e548c6b6690993afdad6272 Gerrit-Change-Number: 24218 Gerrit-PatchSet: 9 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Mon, 13 Jul 2026 17:02:14 +0000 Gerrit-HasComments: Yes
