Jim Apple has posted comments on this change.

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

Patch Set 2:


Commit Message:

Line 10: Form 1: Impala UDF calling
> Greetings Jim.
"Pros" means the reasons for something, while "cons" means the reasons against. 
I am asking you to explain why there should be two different syntaxes for the 
same function.

PS1, Line 12:   "where": an enumerate value denoting the trim direction.
> Done
I don't think you mean "an enumerate value" -- "enumerate" is, as far as I 
know, never an adjective. I don't think you need the word at all here.

PS1, Line 13: 
This line is still 3 characters over the red line in patch set 2. Please 
double-check on things ilke this before replying "Done", as it's not a good use 
of your time to have to go back and fix it twice and it's not a good use of 
your reviewer's time to ask you to fix it twice.

Line 19:     The order of such characters doesn't matter. Multiple occurrances 
> Done
I noted "don't just answer those particular questions", but you did exactly 
that. Please don't do that.

The prose should be able to explain what the function is. A person who doesn't 
know what the function is and doesn't understand the name of the function 
should be able to understand what it does without having to decipher the 
examples. You can see how this works with some of the functions here:


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: 2
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: Youwei Wang <youwei.a.w...@intel.com>
Gerrit-HasComments: Yes

Reply via email to