Youwei Wang has posted comments on this change. Change subject: IMPALA-889: Add support for an ISO-SQL compliant trim() function. ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/4474/1//COMMIT_MSG Commit Message: Line 10: Purpose: Removes all instances of one or more characters from the > "Pros" means the reasons for something, while "cons" means the reasons agai Greetings, Jim. Please be kind to allow me to give some explanations from the start: 1. In order to support the syntax like: btrim(heading|trailing|both char_set from string), it is necessary to register an entry in the common/function-registry/impala_functions.py file. Actually, it is also mapped to the entry point of the backend logic. 2. Such entry will be encoded in the frontend built-in UDF category for parsing usage and it will be called when users input such query: “select btrim(heading|trailing|both char_set from string)”; 3. Once such entry is ready and compiled, the syntax of the form #1 will be automatically supported, no matter you like it or not. This is exactly the same case as this function “extract(timestamp, string unit) || extract(unit FROM timestamp)” shown in this link: http://www.cloudera.com/documentation/archive/impala/2-x/2-1-x/topics/impala_datetime_functions.html It is easy for me to conceal the description of the form 1. But I can’t stop users from using it if they have some hacking skills and dig deeply into the impala_functions.py file. So in my opinion, it would be more friendly to point out the existence of such syntax - maybe there is someone whoever wants it. I am not sure whether my explanation can answer your question that “why there should be two different syntaxes for the same function.”If you don’t think my explanation is right. Please feel free to point out, thank you. :) PS1, Line 12: "where": Case-insensitive trim direction. Valid options are: > I don't think you mean "an enumerate value" -- "enumerate" is, as far as I Done PS1, Line 13: > This line is still 3 characters over the red line in patch set 2. Please do Done Line 19: "trim_char_set": Case-sensitive characters to be removed. This > I noted "don't just answer those particular questions", but you did exactly Done -- To view, visit http://gerrit.cloudera.org:8080/4474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Youwei Wang <youwei.a.w...@intel.com> Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Youwei Wang <youwei.a.w...@intel.com> Gerrit-HasComments: Yes
