[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has abandoned this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 10: Greetings, Jim. I have updated this patch using the new gerrit project, "Impala-ASF". Corresponding new link is: https://gerrit.cloudera.org/#/c/4474/ Thank you. -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 10: Please update to using the new gerrit project, "Impala-ASF". Instructions are here: https://cwiki.apache.org/confluence/display/IMPALA/How+to+switch+to+Apache-hosted+git Pushes to this project will be disabled on October 1. -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 10: Hi Jim. Sorry for this late response. Since Impala 2809: https://gerrit.cloudera.org/#/c/3081/ is my current task of highest priority, I have to resolve it first. I will try my best to re-pick this one as soon as possible. Sorry for this. -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 10: > Youwei, any plans for this? Any news? -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 10: Youwei, any plans for this? -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 10: Today, Impala's official source is at https://github.com/cloudera/Impala/. As of Monday, July 25, it will be at https://git-wip-us.apache.org/repos/asf?p=incubator-impala.git When that happens, you will have to re-send your changes to be reviewed on gerrit, with some different git configuration. When the new configuration is ready, a notice will go to http://mail-archives.apache.org/mod_mbox/incubator-impala-dev/ . If you aren’t subscribed to that list already, you should subscribe now. -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 10: Have you followed the thread you started on the mailing list? What were your thoughts? -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 10: > (1 comment) Youwei, I am proposing that you, not I, open that discussion on the dev@ mailing list. -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/3213/7//COMMIT_MSG Commit Message: PS7, Line 9: ISO-SQL compliant > When you say "Previous syntax follows the request of one customer", what is Hi Jim. When talking about "Previous syntax", I actually refer to this: select trim(source_string, chars_to_trim, option); comparing to the latest one: select trim(option, chars_to_trim, source_string); I have seen your proposal at that JIRA. I will wait for your decision of how to move on. Thank you so much for your great effort! :) -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/3213/7//COMMIT_MSG Commit Message: PS7, Line 9: ISO-SQL compliant > Hi Jim. When you say "Previous syntax follows the request of one customer", what is the "previous" syntax? Which customer are you talking about? On the JIRA, I've asked some people with more user-contact to weight in. -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has uploaded a new patch set (#10).
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
IMPALA-889: Add support for ISO-SQL trim()
Add support for an ISO-SQL compliant trim() function.
Syntax: select trim(option, chars_to_trim, source_string);
option: enumerate values, available choices are:
left/leading/right/trailing/both:
left/leading means trimming characters from the start of the source string;
right/trailing means trimming characters from the end of the source string;
both means trimming characters from both ends of the source string;
Note: option is case-insensitive, which means 'left' equals 'LeFt'.
chars_to_trim: the characters to trim, which is represented as a string;
source_string: the source string to trim;
Example:
select btrim('left', 'a%', 'abc%%defg%'); returns 'bc%%defg%';
select btrim('right', 'fg%', 'abc%%defg%'); returns 'abc%%de';
select btrim('leading', 'ab%', 'abc%%defg%'); returns 'c%%defg%';
select btrim('trailing', 'bfg%', 'abc%%defg%'); returns 'abc%%de';
select btrim('both', 'abfg%', 'abc%%defg%'); returns 'c%%de';
Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 145 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/13/3213/10
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Youwei Wang
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/3213/7//COMMIT_MSG Commit Message: PS7, Line 9: ISO-SQL compliant > This doesn't look ISO-SQL compliant. Looking at the standard links you prov Hi Jim. It is easy to adjust the order of parameters to follow these: "$where", "$characters" and "$string_to_be_trimmed" like: select trim(option, chars_to_trim, source_string); (Previous syntax follows the request of one customer who proposes this JIRA.) As you can see there exists a SQL keyword "FROM" for the standard SQL. So if you want to follow such ISO-SQL standard very strictly, I am afraid we should modify the Impala SQL parser and not just simply add a UDF here. Would you please share your idea about this? Thank you :) http://gerrit.cloudera.org:8080/#/c/3213/7/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 2009: /// Normal cases with different forms of the "option" argument > Can you break this into secions with a short comment describing what each s Done -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 9 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has uploaded a new patch set (#9).
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
IMPALA-889: Add support for ISO-SQL trim()
Add support for an ISO-SQL compliant trim() function.
Syntax: select trim(option, chars_to_trim, source_string);
option: enumerate values, available choices are:
left/leading/right/trailing/both:
left/leading means trimming characters from the start of the source string;
right/trailing means trimming characters from the end of the source string;
both means trimming characters from both ends of the source string;
Note: option is case-insensitive, which means 'left' equals 'LeFt'.
chars_to_trim: the characters to trim, which is represented as a string;
source_string: the source string to trim;
Example:
select btrim('left', 'a%', 'abc%%defg%'); returns 'bc%%defg%';
select btrim('right', 'fg%', 'abc%%defg%'); returns 'abc%%de';
select btrim('leading', 'ab%', 'abc%%defg%'); returns 'c%%defg%';
select btrim('trailing', 'bfg%', 'abc%%defg%'); returns 'abc%%de';
select btrim('both', 'abfg%', 'abc%%defg%'); returns 'c%%de';
Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 145 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/13/3213/9
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Youwei Wang
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change.
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3213/7//COMMIT_MSG
Commit Message:
PS7, Line 9: ISO-SQL compliant
This doesn't look ISO-SQL compliant. Looking at the standard links you
provided, the standard syntax is
TRIM($where $characters FROM $string_to_be_trimmed)
http://gerrit.cloudera.org:8080/#/c/3213/7/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 2009: TestStringValue("btrim('%abcdefg%', '%', 'lEfT')",
"abcdefg%");
Can you break this into secions with a short comment describing what each
section is testing, like "Invalid third arguments cause no trimming to occur".
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Youwei Wang
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has uploaded a new patch set (#7).
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
IMPALA-889: Add support for ISO-SQL trim()
Add support for an ISO-SQL compliant trim() function.
Syntax: select trim(source_string, chars_to_trim, option);
source_string: the source string to trim;
chars_to_trim: the characters to trim, which is represented as a string;
option: enumerate values, available choices are: left/right/both:
left means trimming characters from the start of the source string;
right means trimming characters from the end of the source string;
both means trimming characters from both ends of the source string;
Note: option is case-insensitive, which means 'left' equals 'LeFt'.
Example:
select btrim('abc%%defg%', 'a%', 'left'); returns 'bc%%defg%';
select btrim('abc%%defg%', 'fg%', 'right'); returns 'abc%%de';
select btrim('abc%%defg%', 'ab%', 'leading'); returns 'c%%defg%';
select btrim('abc%%defg%', 'bfg%', 'trailing'); returns 'abc%%de';
select btrim('abc%%defg%', 'abfg%', 'both'); returns 'c%%de';
Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 138 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/13/3213/7
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Youwei Wang
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has uploaded a new patch set (#6).
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
IMPALA-889: Add support for ISO-SQL trim()
Add support for an ISO-SQL compliant trim() function.
Syntax: select trim(source_string, chars_to_trim, option);
source_string: the source string to trim;
chars_to_trim: the characters to trim, which is represented as a string;
option: enumerate values, available choices are: left/right/both:
left means trimming characters from the start of the source string;
right means trimming characters from the end of the source string;
both means trimming characters from both ends of the source string;
Note: option is case-insensitive, which means 'left' equals 'LeFt'.
Example:
select btrim('abc%%defg%', 'a%', 'left'); returns 'bc%%defg%';
select btrim('abc%%defg%', 'fg%', 'right'); returns 'abc%%de';
select btrim('abc%%defg%', 'ab%', 'leading'); returns 'c%%defg%';
select btrim('abc%%defg%', 'bfg%', 'trailing'); returns 'abc%%de';
select btrim('abc%%defg%', 'abfg%', 'both'); returns 'c%%de';
Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 140 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/13/3213/6
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Youwei Wang
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has posted comments on this change.
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3213/3/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:
Line 732: ctx->GetFunctionState(FunctionContext::THREAD_LOCAL));
> Please use functions, not macros
Done
Line 790: enum class TRIM_OPTION
> Does the rest of our code use ALL_CAPS for enum types?
Hi Jim.
According the definition of "enum ExprConstant" in the expr.h file:
enum ExprConstant {
RETURN_TYPE_SIZE, // int
RETURN_TYPE_PRECISION, // int
RETURN_TYPE_SCALE, // int
ARG_TYPE_SIZE, // int[]
ARG_TYPE_PRECISION, // int[]
ARG_TYPE_SCALE, // int[]
};
I think "enum class TrimOption" may be a proper choice.
Thanks.
Line 798: if (memcmp(reinterpret_cast(option.ptr), "left", 4) ==
0) {
> This does the wrong thing if option.ptr is "leftZZZ"
Done
http://gerrit.cloudera.org:8080/#/c/3213/3/be/src/exprs/string-functions.h
File be/src/exprs/string-functions.h:
Line 103: /// Similar to BTrimString with extra direction 'option', which has
three choices:
> You must document what happens if it is NULL or otherwise incorrect.
Done
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Youwei Wang
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has uploaded a new patch set (#4).
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
IMPALA-889: Add support for ISO-SQL trim()
Add support for an ISO-SQL compliant trim() function.
Syntax: select trim(source_string, chars_to_trim, option);
source_string: the source string to trim;
chars_to_trim: the characters to trim, which is represented as a string;
option: enumerate values, available choices are: left/right/both:
left means trimming characters from the start of the source string;
right means trimming characters from the end of the source string;
both means trimming characters from both ends of the source string;
Note: option is case-insensitive, which means 'left' equals 'LeFt'.
Example:
select btrim('abc%%defg%', 'a%', 'left'); returns 'bc%%defg%';
select btrim('abc%%defg%', 'fg%', 'right'); returns 'abc%%de';
select btrim('abc%%defg%', 'ab%', 'leading'); returns 'c%%defg%';
select btrim('abc%%defg%', 'bfg%', 'trailing'); returns 'abc%%de';
select btrim('abc%%defg%', 'abfg%', 'both'); returns 'c%%de';
Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 141 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/13/3213/4
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Youwei Wang
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change.
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:
Line 785: MACRO_TRIM_CHECK
> Greetings, Jim.
That's about what happens if it is no tspecified, not if it is "some garbage
string".
http://gerrit.cloudera.org:8080/#/c/3213/3/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:
Line 732: ctx->GetFunctionState(FunctionContext::THREAD_LOCAL));
Please use functions, not macros
Line 790: enum class TRIM_OPTION
Does the rest of our code use ALL_CAPS for enum types?
Line 798: if (memcmp(reinterpret_cast(option.ptr), "left", 4) ==
0) {
This does the wrong thing if option.ptr is "leftZZZ"
http://gerrit.cloudera.org:8080/#/c/3213/3/be/src/exprs/string-functions.h
File be/src/exprs/string-functions.h:
Line 103: /// Similar to BTrimString with extra direction 'option', which has
three choices:
You must document what happens if it is NULL or otherwise incorrect.
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Youwei Wang
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: Line 785: MACRO_TRIM_CHECK > What does the ISO doc say? Greetings, Jim. Please refer this link: http://troels.arvin.dk/db/rdbms/#functions-TRIM As for trim section, there exists such description “Core SQL feature ID E021-09: TRIM(where characters FROM string_to_be_trimmed) where may be one of LEADING, TRAILING or BOTH—or omitted which implies BOTH.” Here is another link which may be useful: http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt Please goto the context of following description: “b) If is not specified, then BOTH is implicit.” Thanks. -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Jim Apple has posted comments on this change. Change subject: IMPALA-889: Add support for ISO-SQL trim() .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: Line 785: // both > Done What does the ISO doc say? -- To view, visit http://gerrit.cloudera.org:8080/3213 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Youwei Wang Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Youwei Wang Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has posted comments on this change.
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
Patch Set 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 1919:
> long line
Done
Line 1921: for (int i = 0; i < 3; i++) {
> Can you add a test with the second argument being ''? Can you add one where
Done
http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:
PS2, Line 758:
> long line
Done
PS2, Line 764: MACRO_GET_UNIQUE_CHARS
: // When 'chars_to_trim' is unique for each element (e.g. when
'chars_to_trim'
: // is each element of a table column), we need to prepare a
bitset of unique
: // characters here instead of using the bitset from function
context.
: MACRO_TRIM_CHECK
: // Find new starting position.
: int32_t begin = 0;
: MACRO_TRIM_LEFT_PART
: // Find new ending position.
: int32_t end = str.len - 1;
: MACRO_TRIM_RIGHT_PART
: r
> Can you please refactor this copied code into a function?
Done
Line 777:
> Please use C++11 (aka 'scoped') enum
Done
Line 778: StringVal StringFunctions::BTrimStringWithOption(FunctionContext* ctx,
> Maybe memcmp? I'm not sure.
Done
Line 779: const StringVal& str, const StringVal& chars_to_trim, const
StringVal& option) {
> Do you need this cast?
Done
Line 785: MACRO_TRIM_CHECK
> Please check that it actually does equal "both", unless ISO SQL says that n
Done
Line 791: {
> Please refactor these two blocks, too, to avoid the shared code with the ot
Done
http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions.h
File be/src/exprs/string-functions.h:
Line 103: /// Similar to BTrimString with extra direction 'option', which has
three choices:
> This deserves a function comment
Done
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Youwei Wang
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889: Add support for ISO-SQL trim()
Youwei Wang has uploaded a new patch set (#3).
Change subject: IMPALA-889: Add support for ISO-SQL trim()
..
IMPALA-889: Add support for ISO-SQL trim()
Add support for an ISO-SQL compliant trim() function.
Syntax: select trim(source_string, chars_to_trim, option);
source_string: the source string to trim;
chars_to_trim: the characters to trim, which is represented as a string;
option: enumerate values, available choices are: left/right/both:
left means trimming characters from the start of the source string;
right means trimming characters from the end of the source string;
both means trimming characters from both ends of the source string;
Note: option is case-insensitive, which means 'left' equals 'LeFt'.
Example:
select btrim('abc%%defg%', 'a%', 'left'); returns 'bc%%defg%';
select btrim('abc%%defg%', 'fg%', 'right'); returns 'abc%%de';
select btrim('abc%%defg%', 'abfg%', 'both'); returns 'c%%de';
Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 115 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/13/3213/3
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
[Impala-CR](cdh5-trunk) IMPALA-889 Add support for ISO-SQL trim()
Jim Apple has posted comments on this change.
Change subject: IMPALA-889 Add support for ISO-SQL trim()
..
Patch Set 2:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 1919: TestStringValue("btrim('abcdefg',
'abccffg', 'right')", "abcde");
long line
Line 1921: TestStringValue("btrim('', 'abc', 'left')", "");
Can you add a test with the second argument being ''? Can you add one where
only the fist argument is NULL?
http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:
PS2, Line 758: r
long line
PS2, Line 764: bitset<256>* unique_chars = reinterpret_cast*>(
: ctx->GetFunctionState(FunctionContext::THREAD_LOCAL));
: // When 'chars_to_trim' is unique for each element (e.g. when
'chars_to_trim'
: // is each element of a table column), we need to prepare a
bitset of unique
: // characters here instead of using the bitset from function
context.
: if (!ctx->IsArgConstant(1)) {
: unique_chars->reset();
: DCHECK(chars_to_trim.len != 0 || chars_to_trim.is_null);
: for (int32_t i = 0; i < chars_to_trim.len; ++i) {
: unique_chars->set(static_cast(chars_to_trim.ptr[i]),
true);
: }
: }
Can you please refactor this copied code into a function?
Line 777: int option_;
Please use C++11 (aka 'scoped') enum
Line 778: if (strncmp(reinterpret_cast(option.ptr),
Maybe memcmp? I'm not sure.
Line 779: reinterpret_cast("left"), 4) == 0) {
Do you need this cast?
Line 785: // both
Please check that it actually does equal "both", unless ISO SQL says that
non-"both" arguments that are also not "left" or "right" must be interpreted as
"both".
Line 791: if (option_ == 0 || option_ == 2) {
Please refactor these two blocks, too, to avoid the shared code with the other
btrim
http://gerrit.cloudera.org:8080/#/c/3213/2/be/src/exprs/string-functions.h
File be/src/exprs/string-functions.h:
Line 103: static StringVal BTrimStringWithOption(FunctionContext* ctx, const
StringVal& str,
This deserves a function comment
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang
Gerrit-Reviewer: Jim Apple
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-889 Add support for ISO-SQL trim()
Youwei Wang has uploaded a new patch set (#2).
Change subject: IMPALA-889 Add support for ISO-SQL trim()
..
IMPALA-889 Add support for ISO-SQL trim()
Add support for an ISO-SQL compliant trim() function.
Syntax: select trim(source_string, chars_to_trim, option);
source_string: the source string to trim;
chars_to_trim: the characters to trim, which is represented as a string;
option: enumerate values, available choices are: left/right/both;
left means trimming characters from the start of the source string;
right means trimming characters from the end of the source string;
both means trimming characters from both ends of the source string;
Example:
select btrim('abc%%defg%', 'a%', 'left'); returns 'bc%%defg%';
select btrim('abc%%defg%', 'fg%', 'right'); returns 'abc%%de';
select btrim('abc%%defg%', 'abfg%', 'both'); returns 'c%%de';
Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/13/3213/2
--
To view, visit http://gerrit.cloudera.org:8080/3213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang <429222...@qq.com>
