Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14885 )
Change subject: IMPALA-8891: concat_ws() null handling is non-standard ...................................................................... Patch Set 8: (8 comments) Thanks for addressing the nits! Sorry that I still have some minor comments... But the code is pretty close to be merge! http://gerrit.cloudera.org:8080/#/c/14885/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14885/8//COMMIT_MSG@7 PS8, Line 7: concat_ws() null handling is non-standard I think the title can be more clear as "Fix non-standard null handling in concat_ws()" http://gerrit.cloudera.org:8080/#/c/14885/8//COMMIT_MSG@42 PS8, Line 42: ConCat nit: Concat http://gerrit.cloudera.org:8080/#/c/14885/8//COMMIT_MSG@43 PS8, Line 43: the nit: The http://gerrit.cloudera.org:8080/#/c/14885/8//COMMIT_MSG@45 PS8, Line 45: ConCat nit: Concat http://gerrit.cloudera.org:8080/#/c/14885/8/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/14885/8/be/src/exprs/expr-test.cc@4654 PS8, Line 4654: TestIsNull("concat('a', 'b', NULL)", TYPE_STRING); Could you add a test case for empty strings to cover total_size == 0 in StringFunctions::Concat()? e.g. concat('', '', '') http://gerrit.cloudera.org:8080/#/c/14885/8/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/14885/8/be/src/exprs/string-functions-ir.cc@860 PS8, Line 860: NULL nit: we try to use nullptr in new codes. http://gerrit.cloudera.org:8080/#/c/14885/8/be/src/exprs/string-functions-ir.cc@869 PS8, Line 869: } if total_size == 0, we can return StringVal() fast here. http://gerrit.cloudera.org:8080/#/c/14885/8/be/src/exprs/string-functions-ir.cc@885 PS8, Line 885: NULL nit: can change to nullptr too. -- To view, visit http://gerrit.cloudera.org:8080/14885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I64cd3bfbb952e431a0cf52a5835ac05d2513d29b Gerrit-Change-Number: 14885 Gerrit-PatchSet: 8 Gerrit-Owner: jichen <ji.c...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: jichen <ji.c...@cloudera.com> Gerrit-Comment-Date: Sun, 15 Dec 2019 08:02:31 +0000 Gerrit-HasComments: Yes
