impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Youwei Wang (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-889: Add support for an ISO-SQL compliant trim() function.
Date Fri, 23 Sep 2016 01:36:02 GMT
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.wang@intel.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Youwei Wang <youwei.a.wang@intel.com>
Gerrit-HasComments: Yes

Mime
View raw message