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 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/21825/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21825/10//COMMIT_MSG@40 PS10, Line 40: > utf8 handling could be mentioned: Added. http://gerrit.cloudera.org:8080/#/c/21825/10//COMMIT_MSG@51 PS10, Line 51: 2. Syntax wrapper. TrimFromExpr class was introduced as a syntax wrapper > This became not 100% accurate as leading/trailing/both are added as keyword I think this is still pretty important design choice, so I left some part of it and added about leading/trailing/both. Pls check the updated version. http://gerrit.cloudera.org:8080/#/c/21825/10//COMMIT_MSG@60 PS10, Line 60: reserved wor > Maybe this part could be removed? These look more like questions than infor Removed, even though there is indeed inconsistency with EXTRACT-FROM on 2 key points: be and argument keywords. http://gerrit.cloudera.org:8080/#/c/21825/8/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/8/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@45 PS8, Line 45: slot > >It won't. getValueWithOriginalEscapes must be doing the trick. Ok, makes sense. http://gerrit.cloudera.org:8080/#/c/21825/10/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/10/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@146 PS10, Line 146: > nit: could be simply "column name", as it may not come from a table (e.g. v Good point. http://gerrit.cloudera.org:8080/#/c/21825/10/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@159 PS10, Line 159: rMsg.append("Expression '" + charset_.toSql() + "' ha > This won't handle the case when charset_ comes from slot_, and I also don't Done -- 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: 11 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: Tue, 29 Oct 2024 09:50:19 +0000 Gerrit-HasComments: Yes
