impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-889: Add support for ISO-SQL trim()
Date Thu, 22 Sep 2016 11:49:01 GMT
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 of
> 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
To unsubscribe, visit

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

View raw message