Zoltan Borok-Nagy 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 12: (13 comments) Had a couple of comments, but the change looks good to me overall. http://gerrit.cloudera.org:8080/#/c/16741/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16741/12//COMMIT_MSG@9 PS12, Line 9: BE as nit: lines longer than 72 chars http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc File be/src/exprs/iceberg-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc@68 PS12, Line 68: if (UNLIKELY(width.val < std::numeric_limits<int32_t>::min())) { Could you add a comment here? http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc@109 PS12, Line 109: DCHECK(input.val < 0); This DCHECK seems unnecessary in the context of the above line. http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc@111 PS12, Line 111: result.val > 0 Maybe you could wrap it to UNLIKELY http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc@159 PS12, Line 159: BucketPartitionTransformDecimalImpl nit: maybe rename to BucketByteArray? http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc@166 PS12, Line 166: * sizeof(char) seems unnecessary http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-test.cc File be/src/exprs/iceberg-functions-test.cc: http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-test.cc@199 PS12, Line 199: that skips checking : // function context for ARG_TYPE_SIZE. nit: that gets the size as parameter? http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-test.cc@294 PS12, Line 294: void IcebergBucketPartitionTransformTests::TestIntegerNumbers() { Could you add tests for int/long min/max values? http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h File be/src/util/bit-util.h: http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h@352 PS12, Line 352: /// E.g. 12065530 = [0, -72, 26, -6] nit: could you add an example for a negative number, e.g. -129? From the tests I can see it's {-1, 127} but it'd be a useful example IMO. http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h@357 PS12, Line 357: if (input < 0) value = ~value; Is it necessary? How about adding the bytes as they are, and in the end just remove the redundant zeroes and minus ones from the end of the array before reverse() is called? http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h@359 PS12, Line 359: 2 byte range 0xFF is one byte http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h@365 PS12, Line 365: value &= ~((T)0XFF << (8 * i)); nit: We could start the loop with char value_to_save = value & 0XFF; value >>= BYTE_SIZE; http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h@368 PS12, Line 368: add a trailing zero. 'trailing' is a bit confusing because at the end it'll be in the front. -- 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: 12 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: Mon, 14 Dec 2020 18:10:03 +0000 Gerrit-HasComments: Yes
