Jim Apple has posted comments on this change.

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

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/expr-test.cc:

PS8, Line 1977:  
Make the test cases even smaller.

File be/src/exprs/string-functions-ir.cc:

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 

Line 893:   if (trim_option == TrimOption::LEADING || trim_option == 
TrimOption::BOTH) {
trim_option | TrimOption::LEADING

File common/function-registry/impala_functions.py:

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'], 
Did you try changing this name and making it invisible? What happens?

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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Youwei Wang <youwei.a.w...@intel.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com>
Gerrit-Reviewer: Youwei Wang <youwei.a.w...@intel.com>
Gerrit-HasComments: Yes

Reply via email to