-
Notifications
You must be signed in to change notification settings - Fork 180
Add mvappend function for Calcite PPL #4438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
How does the current logic handle primitive arrays? |
|
How does the current implementation handle type coercion? Is it expected the return 0 for integer value when the array has both integer and double? Or do we need some additional handling logic here |
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMVAppendFunctionIT.java
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMVAppendFunctionIT.java
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMVAppendFunctionIT.java
Show resolved
Hide resolved
Signed-off-by: Tomoyuki Morita <[email protected]>
| if (hasStringType && hasNumericType) { | ||
| return typeFactory.createSqlType(SqlTypeName.VARCHAR); | ||
| } |
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.
3 questions
- Could explain mostGeneralType rule?
- if (hasStringType && hasNumericType) it should return ANY?
- can we leverage typeFactory.leastRestrictive(List.of(...))?
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.
Fixed the logic to simply return ANY when any incompatible type is detected. typeFactory.leastRestrictive does not do much job for incompatible types (It only works between structs)
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.
nit: Does AbstractTypeCoercion.getTightestCommonType() works?
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.
The basic idea of AbstractTypeCoercion.getTightestCommonType() is returning widened type with no precision loss, and works only for nullability difference and number precision difference. Is it mainly used for binary operator (or function) to align the input types to execute operation, in my understanding.
...src/main/java/org/opensearch/sql/expression/function/CollectionUDF/MVAppendFunctionImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
|
Fixed logic to simply return ANY in case of mixed types. This spec might be debatable and welcome your opinion, but the idea is to leave users to cast to their preferred type. (Anyway, |
| +--------------+ | ||
|
|
||
| PPL> source=accounts | eval names_array = array(firstname, lastname) | eval result = mvjoin(names_array, ', ') | fields result | head 1 | ||
| os> source=people | eval result = mvappend(1, 'text', 2.5) | fields result | head 1 |
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.
This is the main reason to add a UDF rather than reusing array_append?
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.
Main motivation is to handle null/value/array seemlessly, so that value merge operation won't become too complicated.
* Add mvappend function for Calcite PPL Signed-off-by: Tomoyuki Morita <[email protected]> * Fix annonymizer test Signed-off-by: Tomoyuki Morita <[email protected]> * Fix IT Signed-off-by: Tomoyuki Morita <[email protected]> * Minor fix Signed-off-by: Tomoyuki Morita <[email protected]> * Fix type coercion issue Signed-off-by: Tomoyuki Morita <[email protected]> * Fix test Signed-off-by: Tomoyuki Morita <[email protected]> --------- Signed-off-by: Tomoyuki Morita <[email protected]> (cherry picked from commit 5c784fe) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Add mvappend function for Calcite PPL Signed-off-by: Tomoyuki Morita <[email protected]> * Fix annonymizer test Signed-off-by: Tomoyuki Morita <[email protected]> * Fix IT Signed-off-by: Tomoyuki Morita <[email protected]> * Minor fix Signed-off-by: Tomoyuki Morita <[email protected]> * Fix type coercion issue Signed-off-by: Tomoyuki Morita <[email protected]> * Fix test Signed-off-by: Tomoyuki Morita <[email protected]> --------- Signed-off-by: Tomoyuki Morita <[email protected]> (cherry picked from commit 5c784fe) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Add mvappend function for Calcite PPL * Fix annonymizer test * Fix IT * Minor fix * Fix type coercion issue * Fix test --------- (cherry picked from commit 5c784fe) Signed-off-by: Tomoyuki Morita <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Add mvappend function for Calcite PPL Signed-off-by: Tomoyuki Morita <[email protected]> * Fix annonymizer test Signed-off-by: Tomoyuki Morita <[email protected]> * Fix IT Signed-off-by: Tomoyuki Morita <[email protected]> * Minor fix Signed-off-by: Tomoyuki Morita <[email protected]> * Fix type coercion issue Signed-off-by: Tomoyuki Morita <[email protected]> * Fix test Signed-off-by: Tomoyuki Morita <[email protected]> --------- Signed-off-by: Tomoyuki Morita <[email protected]> Signed-off-by: Asif Bashar <[email protected]>
Description
mvappend(value1, value2, ...)that combines all arguments into a single arrayspathcommand withoutpathparameter so it can merge extracted fields with existing fields.Related Issues
spathcommand dynamic columns support #4307Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.