-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Allow non literal rhs in MV_FILTER_ONLY and MV_FILTER_NONE #16113
Conversation
|
||
Object[] array = arrayEval.asArray(); | ||
if (array == null) { | ||
return ExprEval.of(null); |
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: this should probably return ExprEval.ofArray(arrayEval.asArrayType(), null)
so it doesn't change the type to STRING. This is a bug in FilterFunction
too..
SettableLambdaBinding lambdaBinding = new SettableLambdaBinding(arrayEval.elementType(), lambdaExpr, bindings); | ||
Object[] filtered = filter(arrayEval.asArray(), lambdaExpr, lambdaBinding).toArray(); | ||
if (filtered.length == 0) { | ||
return ExprEval.of(null); |
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.
same comment about ExprEval.ofArray(arrayEval.asArrayType(), null)
@@ -476,6 +476,44 @@ private <T> Stream<T> filter(T[] array, LambdaExpr expr, SettableLambdaBinding b | |||
} | |||
} | |||
|
|||
/** | |||
* Extended version of {@link FilterFunction} to return a null expr if filtered result turns out to be empty |
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.
hmm, reading the docs for filter
, https://github.com/apache/druid/blob/master/docs/querying/math-expr.md#apply-functions, it actually claims that it returns null if no elements match. Given that, I suppose we could just fix filter
to make it match the docs instead of creating a new function. Is it actually useful to have a filter
that returns empty array if no matches?
Alternatively, if we do decide we want both behaviors, we should update those docs and add new docs for this function.
return ExprEval.ofArray(arrayEval.asArrayType(), filtered); | ||
} | ||
|
||
private <T> Stream<T> filter(T[] array, LambdaExpr expr, SettableLambdaBinding binding) |
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: maybe should just make this method not private in FilterFunction
? (assuming we keep this)
|
||
String dummyNull = null; | ||
assertExpr("filter((x) -> array_contains([], x), ['a', 'b'])", dummyNull); | ||
assertExpr("filter((x) -> array_contains([], x), null)", dummyNull); |
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: this variable seems more chars than just using null
directly...
Description
This PR allows to use the MV_FILTER_ONLY & MV_FILTER_NONE functions with a non literal argument.
Currently
select mv_filter_only('mvd_dim', 'array_dim') from 'table'
returns aUnhandled Query Planning Failure
This is being tackled and also considered for the cases where the
array_dim
having null & empty valuesKey changed/added classes in this PR
MultiValueStringOperatorConversions
ApplyFunction
CalciteMultiValueStringQueryTest
This PR has: