Youwei Wang has posted comments on this change.

Change subject: IMPALA-889: Add support for an ISO-SQL compliant trim() 

Patch Set 3:

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/ 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:
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 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 

PS1, Line 13: 
> This line is still 3 characters over the red line in patch set 2. Please do

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Youwei Wang <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Youwei Wang <>
Gerrit-HasComments: Yes

Reply via email to