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 13: (13 comments) 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: > nit: lines longer than 72 chars Done 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 the input is negative and within int32 range while the width is bigger than > Could you add a comment here? Well, giving this a second (third) look this seems incorrect as width can't be negative. I rewrote this part of the code and added a comment. http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc@109 PS12, Line 109: IcebergFunctions::Trun > This DCHECK seems unnecessary in the context of the above line. Done http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc@111 PS12, Line 111: !CheckInputsAn > Maybe you could wrap it to UNLIKELY Done http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc@159 PS12, Line 159: urn IntVal::null(); > nit: maybe rename to BucketByteArray? I remaned this but not exactly to the one you suggested. What do you think? http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-ir.cc@166 PS12, Line 166: al& width) { > seems unnecessary Done 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: ionTransform seems : // problematic as it queries ARG_TYPE_ > nit: that gets the size as parameter? Done http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/exprs/iceberg-functions-test.cc@294 PS12, Line 294: ctx->impl()->Close(); > Could you add tests for int/long min/max values? Done 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 t Sure, Done http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h@357 PS12, Line 357: T value = input; > Is it necessary? How about adding the bytes as they are, and in the end jus This is needed because negative numbers are stored in two's complement format and for this algorithm to work we have to apply some special treatment for negative numbers. L365 and L366 are meant to handle early exits from the loop. With you other suggestion below there is no longer a need for L365. http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h@359 PS12, Line 359: f(value); ++i > 0xFF is one byte Done http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h@365 PS12, Line 365: // is nothing left to process. > nit: We could start the loop with Thanks, your suggestion is cleaner then what I have now. Done http://gerrit.cloudera.org:8080/#/c/16741/12/be/src/util/bit-util.h@368 PS12, Line 368: > 'trailing' is a bit confusing because at the end it'll be in the front. 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: 13 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: Tue, 15 Dec 2020 12:53:38 +0000 Gerrit-HasComments: Yes
