Alex Behm has posted comments on this change. Change subject: IMPALA-889: Add support for an ISO-SQL compliant trim() function. ......................................................................
Patch Set 3: (4 comments) Missing parser tests as well as TrimExpr.java http://gerrit.cloudera.org:8080/#/c/4474/3//COMMIT_MSG Commit Message: Line 9: Syntax #1: BTRIM(string where, string trim_char_set, string target); We should definitely not invent a new function name and stick to trim(). The existing trim() is a special case of the more general ISO-SQL trim(), so we should fold the old functionality into the new more general expr. Line 32: btrim('left', 'a%', 'abc%%defg%%%%%'); returns 'bc%%defg%%%%%'; The 'where' and 'trim_char_set' clauses are optional, so the following are also legal uses of trim(): trim(' abcde '); trim(right from 'abcde '); trim('z' from 'zzzabczzz'); trim(left 'xz' from 'abcxz'); http://gerrit.cloudera.org:8080/#/c/4474/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 2418: string_expr ::= Not clear what this rule adds. The copy+paste nature of it means an increased maintenance burden. Line 2449: // Below is a special case for BTRIM. Idents are used to avoid adding new keywords. My understanding is that several of the trim() clauses are optional, e.g., the location and the characters to trim. The expr seems complicated enough that we should make TRIM a keyword and add a separate production "trim_expr ::=" to recognize it (keep the optional clauses in mind). Most other DBs seem to have TRIM as a keyword also, so we'd not be alone here. -- To view, visit http://gerrit.cloudera.org:8080/4474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Youwei Wang <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Youwei Wang <[email protected]> Gerrit-HasComments: Yes
