expression virtual column selector fix for expressions which produce array types#7958
Conversation
clintropolis
changed the title
| @@ -108,7 +108,11 @@ public Object getObject() | |||
| { | |||
| // No need for null check on getObject() since baseSelector impls will never return null. | |||
| //noinspection ConstantConditions | |||
There was a problem hiding this comment.
Is the //noinspection still on the right line?
There was a problem hiding this comment.
I don't believe it is needed anymore, will remove.
| return baseSelector.getObject().value(); | ||
| ExprEval eval = baseSelector.getObject(); | ||
| if (eval.isArray()) { | ||
| return Arrays.stream(eval.asArray()).map(String::valueOf).collect(Collectors.toList()); |
There was a problem hiding this comment.
Do we allow null numbers in lists? If so, this'll turn them into "null" rather than keeping them as null. What's the right behavior?
There was a problem hiding this comment.
Oops, probably not "null". There are a few other places are doing this as well, will fix and add some tests around this behavior.
| private static Object coerceListDimToStringArray(List val) | ||
| { | ||
| Object[] arrayVal = val.stream().map(Object::toString).toArray(String[]::new); | ||
| Object[] arrayVal = val.stream().map(x -> x != null ? x.toString() : x).toArray(String[]::new); |
There was a problem hiding this comment.
nit: x != null ? x.toString() : null looks more clear to me.
There was a problem hiding this comment.
I have some more follow-up work to do, will try to revisit and clean stuff up a bit more in the future.
…array types (apache#7958) * fix bug in multi-value string expression column selector * more test * imports!! * fixes
3 participants
This PR fixes an omission in #7588 of coercing
ExprEvalthat output array types back into a string list, which caused issues with using filters against expression virtual columns that produced an array output, as well as problems coercing output for scan queries in druid-sql.The added test to
MultiValueDimensionTest.javawould fail with no matching results prior to the changes of this PR.Additionally, added a few druid-sql tests using multi-value string dimensions as
varcharto ensure things work as expected, 3 of which would have failed prior to this PR.