Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8900 )
Change subject: IMPALA-3282: Adds regexp_escape built-in function ...................................................................... Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/8900/5/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8900/5/be/src/exprs/string-functions-ir.cc@623 PS5, Line 623: const strings::CharSet to_escape(".\\+*?[^]$(){}=!<>|:-"); > This is a constant (perhaps "REGEX_ESCAPE_CHARACTERS") and could be declare Thanks for the suggestion. The name is changed and static keyword is also added. By the way, I'd like to leave it in the function because it is just used in this function. http://gerrit.cloudera.org:8080/#/c/8900/5/be/src/exprs/string-functions-ir.cc@637 PS5, Line 637: DCHECK_GE(result.len, 0); > DCHECK_GE(result.len, str.len) should be true too, no? Correct. the length of a new generated string should be greater or equal than the length of the original string. Your idea is more close to what I intended. -- To view, visit http://gerrit.cloudera.org:8080/8900 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84c3e0ded26f6eb20794c38b75be9b25cd111e4b Gerrit-Change-Number: 8900 Gerrit-PatchSet: 5 Gerrit-Owner: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 23 Jan 2018 00:57:24 +0000 Gerrit-HasComments: Yes
