Alex Behm has posted comments on this change.

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

Patch Set 3:


Missing parser tests as well as
Commit Message:

Line 9: Syntax #1: BTRIM(string where, string trim_char_set, string target);
We should definitely not invent a new function name and stick to trim(). The 
existing trim() is a special case of the more general ISO-SQL trim(), so we 
should fold the old functionality into the new more general expr.

Line 32:   btrim('left', 'a%', 'abc%%defg%%%%%'); returns 'bc%%defg%%%%%';
The 'where' and 'trim_char_set' clauses are optional, so the following are also 
legal uses of trim():

trim('  abcde  ');
trim(right from 'abcde  ');
trim('z' from 'zzzabczzz');
trim(left 'xz' from 'abcxz');
File fe/src/main/cup/sql-parser.cup:

Line 2418: string_expr ::=
Not clear what this rule adds. The copy+paste nature of it means an increased 
maintenance burden.

Line 2449:   // Below is a special case for BTRIM. Idents are used to avoid 
adding new keywords.
My understanding is that several of the trim() clauses are optional, e.g., the 
location and the characters to trim.

The expr seems complicated enough that we should make TRIM a keyword and add a 
separate production "trim_expr ::=" to recognize it (keep the optional clauses 
in mind).

Most other DBs seem to have TRIM as a keyword also, so we'd not be alone here.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 3
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

Reply via email to