gianm commented on code in PR #18507:
URL: https://github.com/apache/druid/pull/18507#discussion_r2334305836
##########
processing/src/main/java/org/apache/druid/math/expr/Function.java:
##########
@@ -2140,6 +2140,19 @@ public ExpressionType
getOutputType(Expr.InputBindingInspector inspector, List<E
{
return ExpressionTypeConversion.conditional(inspector, args.subList(1,
3));
}
+
+ @Override
+ public boolean canVectorize(Expr.InputBindingInspector inspector,
List<Expr> args)
+ {
+ final ExpressionType thenType = args.get(1).getOutputType(inspector);
+ return Objects.equals(thenType, args.get(2).getOutputType(inspector));
Review Comment:
Comment here about why the type check exists would be good.
##########
processing/src/test/java/org/apache/druid/math/expr/VectorExprResultConsistencyTest.java:
##########
@@ -348,6 +349,28 @@ public void testSymmetricalBivariateFunctions()
testFunctions(types, templates, functions);
}
+ @Test
+ public void testIfFunction()
+ {
Review Comment:
Would be good to include some tests to validate that lazy evaluation works,
such as `if(l1 % 2 == 0, l2 / (l1 % 2), -1)`. That would throw a
division-by-zero exception if lazy evaluation wasn't working.
##########
processing/src/main/java/org/apache/druid/math/expr/vector/ExprEvalBindingVector.java:
##########
@@ -133,9 +136,42 @@ public Object[] getObjectVector()
objects[i] = values[i];
}
}
- return objects;
+ } else {
+ objects = bindings.getObjectVector(bindingName);
+ }
+ return objects;
+ }
+
+ @Override
+ public boolean elementAsBoolean(int index)
Review Comment:
Would it make sense to vectorize this as well, so it's like `boolean[]
getConditionVector()`?
##########
processing/src/main/java/org/apache/druid/math/expr/vector/VectorConditionalProcessors.java:
##########
@@ -57,4 +57,43 @@ public static <T> ExprVectorProcessor<T>
nvl(Expr.VectorInputBindingInspector in
}
return (ExprVectorProcessor<T>) processor;
}
+
+ public static <T> ExprVectorProcessor<T> ifFunction(
+ Expr.VectorInputBindingInspector inspector,
+ Expr conditionExpr,
+ Expr thenExpr,
+ Expr elseExpr
+ )
+ {
+ // right now this function can only vectorize if then and else clause have
same output type, if this changes then
Review Comment:
is this comment cut off? it seems to end in the middle of a sentence
##########
processing/src/main/java/org/apache/druid/math/expr/vector/ExprEvalDoubleVector.java:
##########
@@ -69,4 +70,13 @@ public Object[] getObjectVector()
}
return objects;
}
+
+ @Override
+ public boolean elementAsBoolean(int index)
+ {
+ if (nulls != null && nulls[index]) {
+ return Evals.asBoolean(0.0);
Review Comment:
This is just `return false` right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
