Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21525 )
Change subject: IMPALA-13136: Refactor AnalyzedFunctionCallExpr (for Calcite) ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/21525/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/21525/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedFunctionCallExpr.java@34 PS1, Line 34: * A FunctionCallExpr that is always in an analyzed state : * : * The analysis for Calcite expressions can be done in the constructor : * rather than issuing a separate call to "analyze" after the object : * is constructed. Nit: Let's update this comment http://gerrit.cloudera.org:8080/#/c/21525/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java: http://gerrit.cloudera.org:8080/#/c/21525/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java@28 PS1, Line 28: * A NulLLiteral that is always in analyzed state Nit: Update this comment http://gerrit.cloudera.org:8080/#/c/21525/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/AnalyzedNullLiteral.java@34 PS1, Line 34: throws ImpalaException Nit: We can drop this "throws ImpalaException" http://gerrit.cloudera.org:8080/#/c/21525/1/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/21525/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java@47 PS1, Line 47: throws ImpalaException Optional extension: To drop this exception in the API, we would change the AnalysisException below to some form of RuntimeException. http://gerrit.cloudera.org:8080/#/c/21525/1/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/21525/1/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java@58 PS1, Line 58: throws ImpalaException Optional extension: To drop this exception in the API, we would need for the existing throwers to use unchecked exceptions. That would be: - Use NumericLiteral.create() rather than NumericLiteral constructor. It takes the same arguments, but it throws a IllegalStateException if the cast doesn't work rather than a SqlCastException. - The exception we throw for "Unsupported RexLiteral" would need to be a RuntimeException of some form (we sometimes use guava's Preconditions.checkState() for assertions). My Java is a bit rusty, but I think this makes sense. Generally, we want to use unchecked exceptions for things that are not recoverable. A caller has no hope of handling these. -- To view, visit http://gerrit.cloudera.org:8080/21525 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideb662d9c7536659cb558bf62baec29c82217aa2 Gerrit-Change-Number: 21525 Gerrit-PatchSet: 1 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Tue, 18 Jun 2024 17:18:26 +0000 Gerrit-HasComments: Yes
