Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22086 )
Change subject: IMPALA-13523: Decimal Precision needs to be in return type ...................................................................... Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/22086/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22086/8//COMMIT_MSG@7 PS8, Line 7: Precision nit: precision and scale http://gerrit.cloudera.org:8080/#/c/22086/8//COMMIT_MSG@9 PS8, Line 9: Inferred nit: lowercase 'inferred' http://gerrit.cloudera.org:8080/#/c/22086/8//COMMIT_MSG@16 PS8, Line 16: select appx_median(c1), appx_median(c2), appx_median(c3) nit: it's hard to tell what gets fixed in this query. Suggest putting the pre and post values returned by just one of these functions before applying the patch and after. http://gerrit.cloudera.org:8080/#/c/22086/8//COMMIT_MSG@23 PS8, Line 23: 1) when the function is a case statement, in which all cases need I would think the same logic that applies to a CASE .. WHEN would also apply to COALESCE functions. e.g if we have COALESCE (arg1, arg2, .. arg_N), depending on the decimal precision and scale of individual args, the return type of this function would infer the overall precision and scale. Do we have tests like this ? http://gerrit.cloudera.org:8080/#/c/22086/8/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java: http://gerrit.cloudera.org:8080/#/c/22086/8/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@304 PS8, Line 304: fn.getNumArgs() Can we be sure that this method is only called when getNumArgs() > 0 ? If not, this will create an invalid -1 index. The for loop checks for argTypes.size() but I am not sure if fn.getNumArgs() is always the same as that value. Better to check for the number of arguments upfront. http://gerrit.cloudera.org:8080/#/c/22086/8/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@378 PS8, Line 378: if (fromType.getSqlTypeName().equals(SqlTypeName.NULL)) { : if (toType.getSqlTypeName().equals(SqlTypeName.DECIMAL)) { It would be better to use the public static methods in Calcite's SqlTypeUtil class: isNull(), isDecimal() etc. There's an additional check for non-null SqlTypeName that needs to be done which is done in these methods. Otherwise, in some edge cases you might hit an NPE in the above invocation. There are other locations in this code where this could be used; I didn't comment on each one. -- To view, visit http://gerrit.cloudera.org:8080/22086 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie10521b587a74930a01c08b711364f897bb2dc33 Gerrit-Change-Number: 22086 Gerrit-PatchSet: 8 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: Sat, 01 Feb 2025 21:27:56 +0000 Gerrit-HasComments: Yes
