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 Tue, 20 Sep 2016 03:59:16 GMT
Jim Apple has posted comments on this change.

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

Patch Set 1:

Commit Message:

Line 10: Form #1: Impala UDF calling
What are the pros and cons of having two different forms?

Line 11:   Syntax: SELECT TRIM(where, characters, string_to_be_trimmed);
"select" isn't part of the syntax

PS1, Line 12:   where: enumerate values, available choices are: left/leading/right/trailing/both:
Please wrap lines at the red vertical line. I believe it's 70 characters.

PS1, Line 12: enumerate values
What is the purpose of this phrase in the sentence?

PS1, Line 16: option
What is "option"? Do you mean "where"?

PS1, Line 17: is
are, not is

Line 19:   Examples:
Please don't give examples until you explain, in prose, what it means to trim characters -
does the order of the characters in "characters" matter? Does their multiplicity?

And don't just answer those particular questions - try to describe the function precisely.

PS1, Line 30:   Note: left and right are Impala keywords, they are not available in this syntax.
Does this match the standard?

To view, visit
To unsubscribe, visit

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

View raw message