various fixes and improvements to vectorization fallback #17098

clintropolis · 2024-09-18T05:11:09Z

changes:

add ApplyFunction support to vectorization fallback, allowing many of the remaining expressions to be vectorized
add CastToObjectVectorProcessor so that vector engine can correctly cast any type
add support for array and complex vector constants
reduce number of cases which can block vectorization in expression planner to be unknown inputs (such as unknown multi-valuedness)
fix array constructor expression, apply map expression to make actual evaluated type match the output type inference
fix bug in array_contains where something like array_contains([null], 'hello') would return true if the array was a numeric array since the non-null string value would cast to a null numeric
fix isNull/isNotNull to correctly handle any type of input argument

changes: * add `ApplyFunction` support to vectorization fallback, allowing many of the remaining expressions to be vectorized * add `CastToObjectVectorProcessor` so that vector engine can correctly cast any type * add support for array and complex vector constants * reduce number of cases which can block vectorization in expression planner to be unknown inputs (such as unknown multi-valuedness) * fix array constructor expression, apply map expression to make actual evaluated type match the output type inference * fix bug in array_contains where something like array_contains([null], 'hello') would return true if the array was a numeric array since the non-null string value would cast to a null numeric * fix isNull/isNotNull to correctly handle any type of input argument

gianm · 2024-09-24T07:07:57Z

processing/src/main/java/org/apache/druid/math/expr/vector/CastToObjectVectorProcessor.java

+    final ExprEvalVector<?> delegateOutput = delegate.evalVector(bindings);
+    final Object[] toCast = delegateOutput.getObjectVector();
+    for (int i = 0; i < bindings.getCurrentVectorSize(); i++) {
+      ExprEval<?> cast = ExprEval.ofType(delegateType, toCast[i]).castTo(outputType);


IIRC from work on #16994, ExprEval.ofType(inType, o).castTo(outType) and ExprEval.ofType(outType, o) have subtly different behavior and I ended up preferring the latter. I think part of the reason was that the former could throw exceptions in cases where the cast was invalid, and the latter was more "forgiving" (like, returning null instead of throwing an exception). It happened that forgiving is what I wanted in that particular patch.

Here, what's the right thing?

this is the vectorized cast operator for objects, so it should totally use castTo to be consistent with non-vectorized cast

gianm · 2024-09-24T07:12:39Z

processing/src/main/java/org/apache/druid/math/expr/vector/CastToStringVectorProcessor.java

  }

  @Override
  public ExprEvalVector<Object[]> evalVector(Expr.VectorInputBinding bindings)
  {
    ExprEvalVector<?> result = delegate.evalVector(bindings);
    final Object[] objects = result.getObjectVector();
-    final Object[] output = new String[objects.length];
-    for (int i = 0; i < objects.length; i++) {
+    for (int i = 0; i < bindings.getCurrentVectorSize(); i++) {


was this a bug?

I don't think so, previously it would just cast the whole vector size instead of only what was needed

gianm · 2024-09-24T07:16:13Z

processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java

-    final ExprEvalObjectVector eval = new ExprEvalObjectVector(strings, ExpressionType.STRING);
+    final Object[] objects = new Object[maxVectorSize];
+    if (type.isNumeric()) {
+      return constant((Long) null, maxVectorSize);


why does this use null instead of looking at what constant is?

oops, idk why this is here, i think this is from a bad merge between some stashes with experiments i had going on. It should just be removed, and wasn't failing any tests because numeric types use their dedicated constant functions

gianm · 2024-09-24T07:17:15Z

processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java

@@ -85,11 +85,14 @@ public static <T> ExprVectorProcessor<T> makeSymmetricalProcessor(
   *
   * @see org.apache.druid.math.expr.ConstantExpr
   */
-  public static <T> ExprVectorProcessor<T> constant(@Nullable String constant, int maxVectorSize)
+  public static <T> ExprVectorProcessor<T> constant(@Nullable Object constant, int maxVectorSize, ExpressionType type)


why did this change to Object? what sorts of non-String objects might be passed in? could you please update the javadoc?

oh, i needed a way to make vector constants for basically anything not a numeric primitive, so this method was updated to make all of the object constants, will update javadoc

abhishekagarwal87 · 2024-09-24T07:17:56Z

processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java

-    final Object[] strings = new Object[maxVectorSize];
-    Arrays.fill(strings, constant);
-    final ExprEvalObjectVector eval = new ExprEvalObjectVector(strings, ExpressionType.STRING);
+    final Object[] objects = new Object[maxVectorSize];


we are creating this unnecessarily if type is numeric.

numeric types shouldn't ever be calling this, added a defensive check

abhishekagarwal87 · 2024-09-24T07:18:41Z

processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java

-    final ExprEvalObjectVector eval = new ExprEvalObjectVector(strings, ExpressionType.STRING);
+    final Object[] objects = new Object[maxVectorSize];
+    if (type.isNumeric()) {
+      return constant((Long) null, maxVectorSize);


why null though?

see #17098 (comment)

changes: * add `ApplyFunction` support to vectorization fallback, allowing many of the remaining expressions to be vectorized * add `CastToObjectVectorProcessor` so that vector engine can correctly cast any type * add support for array and complex vector constants * reduce number of cases which can block vectorization in expression planner to be unknown inputs (such as unknown multi-valuedness) * fix array constructor expression, apply map expression to make actual evaluated type match the output type inference * fix bug in array_contains where something like array_contains([null], 'hello') would return true if the array was a numeric array since the non-null string value would cast to a null numeric * fix isNull/isNotNull to correctly handle any type of input argument

…7142) changes: * add `ApplyFunction` support to vectorization fallback, allowing many of the remaining expressions to be vectorized * add `CastToObjectVectorProcessor` so that vector engine can correctly cast any type * add support for array and complex vector constants * reduce number of cases which can block vectorization in expression planner to be unknown inputs (such as unknown multi-valuedness) * fix array constructor expression, apply map expression to make actual evaluated type match the output type inference * fix bug in array_contains where something like array_contains([null], 'hello') would return true if the array was a numeric array since the non-null string value would cast to a null numeric * fix isNull/isNotNull to correctly handle any type of input argument

clintropolis added Bug Area - Querying labels Sep 18, 2024

github-actions bot added the Area - Segment Format and Ser/De label Sep 18, 2024

clintropolis added 2 commits September 17, 2024 23:47

fixes

3a8cc49

better

925efbe

clintropolis added this to the 31.0.0 milestone Sep 18, 2024

gianm reviewed Sep 24, 2024

View reviewed changes

abhishekagarwal87 reviewed Sep 24, 2024

View reviewed changes

fixes

6037e03

gianm approved these changes Sep 24, 2024

View reviewed changes

clintropolis merged commit 77a362c into apache:master Sep 24, 2024
90 checks passed

clintropolis deleted the various-expression-vector-fixes branch September 24, 2024 11:29

This was referenced Sep 24, 2024

[Backport] various fixes and improvements to vectorization fallback #17142

Merged

use CastToObjectVectorProcessor for cast to string #17148

Merged

clintropolis mentioned this pull request Sep 26, 2024

add multi-value string object vector matcher and expression vector object selectors #17162

Merged

kfaraz mentioned this pull request Oct 11, 2024

[DRAFT] 31.0.0 Release Notes #17332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

various fixes and improvements to vectorization fallback #17098

various fixes and improvements to vectorization fallback #17098

clintropolis commented Sep 18, 2024

gianm Sep 24, 2024

clintropolis Sep 24, 2024

gianm Sep 24, 2024

clintropolis Sep 24, 2024

gianm Sep 24, 2024

clintropolis Sep 24, 2024

gianm Sep 24, 2024

clintropolis Sep 24, 2024

abhishekagarwal87 Sep 24, 2024

clintropolis Sep 24, 2024

abhishekagarwal87 Sep 24, 2024

clintropolis Sep 24, 2024

various fixes and improvements to vectorization fallback #17098

various fixes and improvements to vectorization fallback #17098

Conversation

clintropolis commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment