Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/21825 )
Change subject: IMPALA-889: Add trim() function matching ANSI SQL definition ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/21825/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21825/2//COMMIT_MSG@19 PS2, Line 19: where FROM string Since both 'where' and 'string' are keywords, it would be better to put them in <> signs to avoid confusion. Just like you did in the comments of TrimFromExpr class. Same applies to L20 and L21. http://gerrit.cloudera.org:8080/#/c/21825/2//COMMIT_MSG@51 PS2, Line 51: Alghough generally keyword nit: Although generally, a keyword... http://gerrit.cloudera.org:8080/#/c/21825/2//COMMIT_MSG@50 PS2, Line 50: no "TRIM" keyword was added to : the language 'TRIM' is already part of the SQL standard. Using it as a keyword would make detecting errors in parser time easier. http://gerrit.cloudera.org:8080/#/c/21825/2//COMMIT_MSG@55 PS2, Line 55: using the TRIM keyword in remote contexts results in syntax errors What do you mean exactly? http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java File fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java: http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@35 PS2, Line 35: nit: the http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@41 PS2, Line 41: nit: a http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@125 PS2, Line 125: if (where_ != null && !TRIM_OPTIONS.contains(where_.toUpperCase())) This error should be caught during parsing, then TrimOption could be an enum and could be used in 'baseFnName'. http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@1927 PS2, Line 1927: // Special cases for TRIM in function call You can also add parser tests for the syntax in 'ParserTest.java'. -- To view, visit http://gerrit.cloudera.org:8080/21825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c4fa6d0d8d0684c4b6d8dac8fd531d205e4f7b4 Gerrit-Change-Number: 21825 Gerrit-PatchSet: 2 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Fri, 20 Sep 2024 14:41:26 +0000 Gerrit-HasComments: Yes
