-
Notifications
You must be signed in to change notification settings - Fork 25
[POAE7-2544] String LOWER/UPPER op support for Arrow format #147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
For the unsupported functions like |
Yes. @jikunshang also mentioned this yesterday. I'll take a look. |
"stringop_lower_literal_null.json"); | ||
|
||
// SELECT UPPER(column) FROM table | ||
assertQueryArrow("SELECT col_2, UPPER(col_2) FROM test;", "stringop_upper_null.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add some case like SELECT * FROM test WHERE UPPER(col_2) = "AAA"
once #144 got merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
@@ -37,7 +37,7 @@ bool getExprUpdatable(std::unordered_map<std::shared_ptr<Analyzer::Expr>, bool> | |||
} | |||
|
|||
bool isStringFunction(std::string function_name) { | |||
std::unordered_set<std::string> supportedStrFunctionSet{"substring"}; | |||
std::unordered_set<std::string> supportedStrFunctionSet{"substring", "lower", "upper"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please define the input argument as const std::string& name
and cause the supportedStrFunctionSet only has three names, so if we want make this function more fast, codes can be written:
std::string_view names[] = {"substring", "lower", "upper"};
return std::find(std::cbegin(names), std::cend(names), name) != std::cend(names);
or
std::string_view names[] = {"substring", "lower", "upper"};
for (size_t i = 0; i < sizeof(names); ++i) {
if (names[i] == name) {
return true;
}
return false;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the optimization advice!
-
pass-by-const-ref has been added.
-
Currently there are only 3 names, but more names are to be added in the future as more string ops are supported. I think leaving this part as is would be fine?
b050156
to
7441656
Compare
What changes were proposed in this pull request?
Updated support for String
LOWER()
andUPPER()
functions.LOWER
orUPPER
to Substrait plans.So plan JSON files were created manually according to observations and Substrait docs.JSON query plans are generated with a modified local fork of substrait-java that supportsLOWER
andUPPER
.30
to50
to cover lowercase letters in the test.Why are the changes needed?
Supporting string ops for Arrow format.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
UTs.
Which label does this PR belong to?