Mihaly Szjatinya 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 14: (6 comments) http://gerrit.cloudera.org:8080/#/c/21825/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21825/12//COMMIT_MSG@41 PS12, Line 41: sets it > It's unclear to me what "it" means in this sentence, it could be reformulat Please ask Csaba, as he asked me to add this :) http://gerrit.cloudera.org:8080/#/c/21825/12/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/21825/12/be/src/exprs/expr-test.cc@4787 PS12, Line 4787: TestStringValue("trim(NULL FROM ' helloworld ')", " helloworld "); > Can we add a test where the charset is the string "both" (or "leading" or " Added. http://gerrit.cloudera.org:8080/#/c/21825/12/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/12/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@54 PS12, Line 54: fnName > What are the acceptable values here? Can we add a Preconditions check to di Name is checked on analysis stage. This has been discussed already, please see related discussions awa Analysis vs Parsing section from earlier commit message. http://gerrit.cloudera.org:8080/#/c/21825/12/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@85 PS12, Line 85: // For special case: "select trim(table_column from 'string') from table" > trim(<where> table_column FROM 'string') As table_column makes sense only for charset, this is a subset of TRIM(where <charset> FROM <string>) below > trim(table_column <charset> FROM 'string') For the same reason, this is not supported. About the comment message, please sync up with Csaba, he also has some ideas, whereas I would keep it as is. http://gerrit.cloudera.org:8080/#/c/21825/12/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/21825/12/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1511 PS12, Line 1511: trim('aa' from 'abc') > Is it intentional that this is the same as the previous one? It's inside parenthesis, I've seen similar pattern in other tests. http://gerrit.cloudera.org:8080/#/c/21825/12/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1512 PS12, Line 1512: "trim(leading 'aa' from 'abc'), trim(string_col from 'string') " + > Can we include a TRIM case with both <where> and <charset>? Absolutely. -- 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: 14 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Wed, 30 Oct 2024 11:36:44 +0000 Gerrit-HasComments: Yes
