Daniel Becker 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: (4 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 > Please ask Csaba, as he asked me to add this :) Ok, he wrote that informally in a comment here, but I don't think the sentence is very clear (grammatically) in this form. What do you think of my suggestion to reformulate the sentence? 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@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 may not be obvious for someone not familiar with this part of the code, so I'd say the comment should definitely say that 'table_column' is <charset>. | this is a subset of TRIM(where <charset> FROM <string>) below There is no <where> parameter in this case. I meant, can we have something like SELECT trim(LEADING table_column from 'some_string') from table; where 'table_column' is a column of 'table'. http://gerrit.cloudera.org:8080/#/c/21825/12/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@148 PS12, Line 148: super.getParams().size() == 1 > It's rather a sanity check, to emphasize that slot is added as exactly seco Wouldn't a Precondition check more appropriate then? 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') > It's inside parenthesis, I've seen similar pattern in other tests. Ok, I missed that. -- 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 14:00:08 +0000 Gerrit-HasComments: Yes
