Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/16741 )
Change subject: IMPALA-10237: Support Bucket and Truncate partition transforms as built-in functions ...................................................................... Patch Set 15: Code-Review+2 (3 comments) carry +2 from Zoltan http://gerrit.cloudera.org:8080/#/c/16741/14/be/src/exprs/iceberg-functions-ir.cc File be/src/exprs/iceberg-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/16741/14/be/src/exprs/iceberg-functions-ir.cc@75 PS14, Line 75: return TruncatePartitionTransformDecimalImpl<int32_t>(input.val4, width.val); > TruncatePartitionTransformNumericImpl handles negative overflow, i.e. when Here you won't overflow as the 9-digit decimals are actually stored as int64 even though they would fit in an int32. E.g. min value of int32 (and the values close to it but still within int32 range) are stored in int64 for decimals and would hit the below case and not this one. http://gerrit.cloudera.org:8080/#/c/16741/14/be/src/exprs/iceberg-functions-ir.cc@163 PS14, Line 163: data( > nit: it doesn't really make a difference, but buffer.data() could be used i Done http://gerrit.cloudera.org:8080/#/c/16741/14/be/src/util/bit-util.h File be/src/util/bit-util.h: http://gerrit.cloudera.org:8080/#/c/16741/14/be/src/util/bit-util.h@363 PS14, Line 363: buffer.push_back(value_to_save); > nit: string also has push_back member function, so this could be just Done -- To view, visit http://gerrit.cloudera.org:8080/16741 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I485680cf79d96d578dd8cfbfd554bec468fe84bd Gerrit-Change-Number: 16741 Gerrit-PatchSet: 15 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 17 Dec 2020 13:10:37 +0000 Gerrit-HasComments: Yes
