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 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/21825/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21825/2//COMMIT_MSG@55 PS2, Line 55: are specifically removed from the reserved words list. However, de > You could add this example to the text here. Absolutely http://gerrit.cloudera.org:8080/#/c/21825/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21825/4//COMMIT_MSG@56 PS4, Line 56: as a k > Shouldn't it be "would result", as it is not the case in Impala before this Not really. It is perhaps a bit confusing the way I wrote it, rewrote the sentence. http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/main/cup/sql-parser.cup@3873 PS2, Line 3873: > Ok, but I think it's a good idea to put this comment about "string_expr" ab Moved http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/main/cup/sql-parser.cup@3873 PS2, Line 3873: RESULT = new TrimFromExpr( > So it means that "select sum(count(id))" doesn't work if we use 'expr' inst Rewrote 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@98 PS2, Line 98: srcExpr_ = other.srcExpr_; > I didn't mean adding the @Override annotation on this function but also ove My bad, added. http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@113 PS2, Line 113: & !trimFromFnName_.ge > I meant extracting "trimFromFnName_.getFunction()" so we don't have to call It's just a getter function which almost certainly should be inlined, hence no extra code. Besides, there's also getDB() later, which is used only two times, which would lead to inconsistency. I mean it's really minor but unless there's some strong convention concern, I think it's alright to call getters this way, that's their purpose after all. http://gerrit.cloudera.org:8080/#/c/21825/2/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@144 PS2, Line 144: turn errMsg.toString(); : } > I'm not sure I agree that these are conceptually unrelated. Both simply hav In this case yes, but in general case, a person familiar with 'getFunctionNotFoundError' function across similar classes would naturally think of a certain structure for it, checking all sorts of parameters of different types. More generally, arbitrarily adding a utility function (in this case an error handling string utility function) is a complication in itself and relentlessly raises consistency questions with all similar places across the project. http://gerrit.cloudera.org:8080/#/c/21825/4/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/4/fe/src/main/java/org/apache/impala/analysis/TrimFromExpr.java@121 PS4, Line 121: if (where_ != null > Using exception handling for this check doesn't conceptually look good in m Agree, this is better. -- 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: 5 Gerrit-Owner: Mihaly Szjatinya <[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: Sat, 28 Sep 2024 17:21:45 +0000 Gerrit-HasComments: Yes
