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 an ISO-SQL compliant trim() function.
Date Thu, 13 Oct 2016 12:05:32 GMT
Jim Apple has posted comments on this change.

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

Patch Set 8:

Commit Message:

Line 14: All syntaxes confirm the standard SQL syntax (Core SQL feature ID
Please describe the existing syntax, too, and say that it already works.

PS8, Line 18: trailing
Please capitalize these.

PS8, Line 18: leading
Please try to match our existing formatting choices. This includes breaking lines at the 70-character
mark, not arbitrarily early as in line 17.

PS8, Line 19:   
Do not indent lines after the first one in a paragraph. Please use git log to study what previous
commit messages looked like.

PS8, Line 23: option
this word is not needed.

PS8, Line 22: empty
            :   argument("" or '')
just say "the empty string"

PS8, Line 24: Syntax
Why is this word capitalized, here and below

PS8, Line 24: target
"target" was not used earlier. Do you mean string_to_be_trimmed?

PS8, Line 31: (standard space)
You don't have to say "standard space" here or below. I was asking about what the standard
says. Are you sure it just says " "? What do postgres and mysql do?

PS8, Line 43: abcdefg
Make the parameter strings even shorter

PS8, Line 46: Syntax #2:
Please separate these sections by blank lines.

PS8, Line 51: "
What is this doing here? Same above. Please try to be more consciencious about these things.
File be/src/exprs/

PS8, Line 1977:  
Make the test cases even smaller.
File be/src/exprs/

PS8, Line 799: static
static in an unnamed namespace is redundant.

PS8, Line 817: static
Put this in the unnamed namespace

PS8, Line 817: *

PS8, Line 818: begin
Is this always 0? Here and below

PS8, Line 838: int begin = 0;
             :   begin = 
Make this one line

PS8, Line 849: StringVal
Does this do the right thing is is_null is true but ptr and len are set?

PS8, Line 862: option.ptr[i]
incorrect argument type

PS8, Line 865: trim_option
This is still not done at parsing/analysis time. That work happens in the front-end.

Line 893:   if (trim_option == TrimOption::LEADING || trim_option == TrimOption::BOTH) {
trim_option | TrimOption::LEADING
File common/function-registry/

Line 429:   [['trim'], 'STRING', ['STRING'], 'impala::StringFunctions::Trim'],
Is this line still needed, or is it covered by the new grammar rules?

Line 430:   [['trim'], 'STRING', ['STRING', 'STRING', 'STRING'], 'impala::StringFunctions::TrimStringWithOption',
Did you try changing this name and making it invisible? What happens?

To view, visit
To unsubscribe, visit

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

View raw message