Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14963 )
Change subject: IMPALA-9010: Add builtin mask functions ...................................................................... Patch Set 2: (9 comments) Thank Zoltan! Addressed the comments. http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/expr-test.cc@10147 PS1, Line 10147: d mas > invalid? or 'invalid digit' maybe? Sorry, I mean can't convert the string 'a' into a valid integer value... Changed the error message. http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc File be/src/exprs/mask-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@46 PS1, Line 46: static inline uint8_t MaskTransform(uint8_t val, int masked_upper_char, > It's only called from the loop of MaskSubStr, maybe it'd be beneficial to m Done http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@141 PS1, Line 141: } > What about the value 0? Shouldn't it have one digit? Oops! I think it's a bug for Hive too. Created HIVE-22699. http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@145 PS1, Line 145: ng t > nit: 'be kept'? Done http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@171 PS1, Line 171: > nit: 'be kept' Done http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@242 PS1, Line 242: r, month, day).To > maybe 'GetFirstChar()' would be more precise and shorter? Good name! Done. http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@260 PS1, Line 260: r > nit: too many slashes Done http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@304 PS1, Line 304: e > nit: too many slashes Done http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions.h File be/src/exprs/mask-functions.h: http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions.h@18 PS1, Line 18: #pragma once > nit: extra blank line Done -- To view, visit http://gerrit.cloudera.org:8080/14963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ica779a1bf63a085d51f3b533f654cbaac102a664 Gerrit-Change-Number: 14963 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Norbert Luksa <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 07 Jan 2020 12:26:53 +0000 Gerrit-HasComments: Yes
