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 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/21825/8/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/21825/8/fe/src/main/cup/sql-parser.cup@3885 PS8, Line 3885: string_expr > I still don't get how string_expr helps, here as it also includedes a simpl string_expr is to narrow down argument types to only those which can result in string, which makes sense for a string function. About ambiguity, yes there is certain ambiguity in this design but I have a feeling we have a bit different understanding of it from your comment. Could you please give an example queries where it would be apparent? 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_ > toSqlImpl() turns it into a string literal, so it will be quoted It won't. getValueWithOriginalEscapes must be doing the trick. > analyzeImpl() analyzes it as SlotRef Usual way would be to store slot as a SlotRef right in the constructor. In that case, later on analysis stage, it would get analyzed prior to TrimFromExpr itself, along with its other children arguments. Therefore analysis error would be about SlotRe being unresolved while imho it's better to give error about identifier not being one of: leading, trailing or both. Hence the trick is to do analysis other way around. > btw can you add a few tests for the new trim syntax in > https://github.com/apache/impala/blob/master/fe/src/test/ > java/org/apache/impala/analysis/ToSqlTest.java ? Added. I noticed some discrepancy between plain FunctionExpr and ExtractFromExpr in a way that the latter uppers the function name. I changed TrimFromExpr to be better consistent with FunctionExpr, but we probably should do the same for ExtractFromExpr if that's ok. And also add some tests for it as well. -- 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: 9 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: Fri, 25 Oct 2024 14:22:28 +0000 Gerrit-HasComments: Yes
