gianm commented on code in PR #15974:
URL: https://github.com/apache/druid/pull/15974#discussion_r1516964283
##########
processing/src/main/java/org/apache/druid/math/expr/Function.java:
##########
@@ -3243,6 +3252,62 @@ public Set<Expr> getArrayInputs(List<Expr> args)
}
}
+ class MultiValueStringHarmonizeNullsFunction implements Function
Review Comment:
Could use a comment as to the purpose of this function, and why it's
undocumented.
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/MultiValueStringOperatorConversions.java:
##########
@@ -458,6 +578,28 @@ boolean isAllowList()
}
}
+
+ private static List<DruidExpression>
harmonizeNullsMvdArg0OperandList(List<DruidExpression> druidExpressions)
+ {
+ final List<DruidExpression> newArgs;
+ if (druidExpressions.get(0).isDirectColumnAccess()) {
Review Comment:
Why not always harmonize the first arg if it's of type `VARCHAR` or
`STRING`? So something like `MV_OVERLAP(LOOKUP(mvd, 'lookupTable'),
ARRAY['stuff', 'things'])` would still have the first arg harmonized.
##########
processing/src/main/java/org/apache/druid/math/expr/Function.java:
##########
@@ -3803,15 +3977,110 @@ public ExpressionType
getOutputType(Expr.InputBindingInspector inspector, List<E
}
@Override
- ExprEval doApply(ExprEval lhsExpr, ExprEval rhsExpr)
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
{
- final Object[] array1 = lhsExpr.asArray();
- final List<Object> array2 = Arrays.asList(rhsExpr.asArray());
- boolean any = false;
+ final ExprEval arrayExpr1 = args.get(0).eval(bindings);
+ final ExprEval arrayExpr2 = args.get(1).eval(bindings);
+
+ final Object[] array1 = arrayExpr1.asArray();
+ if (array1 == null) {
+ return ExprEval.ofLong(null);
+ }
+ ExpressionType array1Type = arrayExpr1.asArrayType();
+ final Object[] array2 = arrayExpr2.castTo(array1Type).asArray();
+ if (array2 == null) {
+ return ExprEval.ofLongBoolean(false);
+ }
+ List<Object> asList = Arrays.asList(array2);
for (Object check : array1) {
- any |= array2.contains(check);
+ if (asList.contains(check)) {
+ return ExprEval.ofLongBoolean(true);
+ }
+ }
+ return ExprEval.ofLongBoolean(false);
+ }
+
+ @Override
+ public void validateArguments(List<Expr> args)
+ {
+ validationHelperCheckArgumentCount(args, 2);
+ }
+
+ @Override
+ public Set<Expr> getScalarInputs(List<Expr> args)
+ {
+ return Collections.emptySet();
+ }
+
+ @Override
+ public Set<Expr> getArrayInputs(List<Expr> args)
+ {
+ return ImmutableSet.copyOf(args);
+ }
+
+ @Override
+ public boolean hasArrayInputs()
+ {
+ return true;
+ }
+
+ @Override
+ public Function asSingleThreaded(List<Expr> args,
Expr.InputBindingInspector inspector)
+ {
+ if (args.get(1).isLiteral()) {
+ final ExpressionType lhsType = args.get(0).getOutputType(inspector);
+ if (lhsType == null) {
+ return this;
+ }
+ final ExpressionType lhsArrayType =
ExpressionType.asArrayType(lhsType);
+ final ExprEval<?> rhsEval =
args.get(1).eval(InputBindings.nilBindings());
+ final Object[] rhsArray = rhsEval.castTo(lhsArrayType).asArray();
+ if (rhsArray == null) {
+ return new ArrayOverlapFunction()
+ {
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final ExprEval arrayExpr1 = args.get(0).eval(bindings);
+ final Object[] array1 = arrayExpr1.asArray();
+ if (array1 == null) {
+ return ExprEval.ofLong(null);
+ }
+ return ExprEval.ofLongBoolean(false);
+ }
+ };
+ }
+ final Set<Object> set = new
ObjectAVLTreeSet<>(lhsArrayType.getElementType().getNullableStrategy());
+ set.addAll(Arrays.asList(rhsArray));
+ return new OverlapConstantArray(set);
+ }
+ return this;
+ }
+
+ private static final class OverlapConstantArray extends
ArrayContainsFunction
+ {
+ final Set<Object> set;
+
+ public OverlapConstantArray(Set<Object> set)
+ {
+ this.set = set;
+ }
+
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final ExprEval<?> lhsExpr = args.get(0).eval(bindings);
+ final Object[] array1 = lhsExpr.asArray();
Review Comment:
It would be great if this didn't need to wrap the `lhs` in an array if `lhs`
is a scalar. Then it would be a better fit for implementing expr version of the
scalar `IN` filter, like `array_contains(scalar_field, array('a', 'b', 'c'))`
for `scalar_field IN ('a', 'b', 'c')`. The current code would still work, it
just has more overhead than necessary.
Could also do this in a follow-up.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java:
##########
@@ -83,7 +83,11 @@ public void reduce(
final RexNode literal;
if (sqlTypeName == SqlTypeName.BOOLEAN) {
- literal = rexBuilder.makeLiteral(exprResult.asBoolean(),
constExp.getType(), true);
+ if (exprResult.valueOrDefault() == null) {
Review Comment:
I wasn't suggesting we should change behavior, so let's keep it then.
##########
processing/src/main/java/org/apache/druid/math/expr/Function.java:
##########
@@ -3803,16 +3854,56 @@ public ExpressionType
getOutputType(Expr.InputBindingInspector inspector, List<E
}
@Override
- ExprEval doApply(ExprEval lhsExpr, ExprEval rhsExpr)
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
{
- final Object[] array1 = lhsExpr.asArray();
- final List<Object> array2 = Arrays.asList(rhsExpr.asArray());
+ final ExprEval arrayExpr1 = args.get(0).eval(bindings);
+ final ExprEval arrayExpr2 = args.get(1).eval(bindings);
+
+ final Object[] array1 = arrayExpr1.asArray();
+ final Object[] array2 = arrayExpr2.asArray();
+ if (array1 == null) {
+ return ExprEval.ofLong(null);
Review Comment:
The new version of this patch with `mv_harmonize_nulls` helps make this more
clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
