This is an automated email from the ASF dual-hosted git repository.
gian pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new 8a45efbf65f fix some null handling bugs with vector expression
processors (#15587)
8a45efbf65f is described below
commit 8a45efbf65f3abbbc42324b7cb858a3b61aa0818
Author: Clint Wylie <[email protected]>
AuthorDate: Tue Dec 19 08:14:17 2023 -0800
fix some null handling bugs with vector expression processors (#15587)
---
...ubleOutLongsInFunctionVectorValueProcessor.java | 2 +-
.../druid/math/expr/vector/VectorProcessors.java | 14 ++++-
.../druid/math/expr/VectorExprSanityTest.java | 68 ++++++++++++++++++----
.../expression/VectorExpressionsSanityTest.java | 10 +---
4 files changed, 72 insertions(+), 22 deletions(-)
diff --git
a/processing/src/main/java/org/apache/druid/math/expr/vector/DoubleOutLongsInFunctionVectorValueProcessor.java
b/processing/src/main/java/org/apache/druid/math/expr/vector/DoubleOutLongsInFunctionVectorValueProcessor.java
index 699f66f53fe..d9fa3fe2856 100644
---
a/processing/src/main/java/org/apache/druid/math/expr/vector/DoubleOutLongsInFunctionVectorValueProcessor.java
+++
b/processing/src/main/java/org/apache/druid/math/expr/vector/DoubleOutLongsInFunctionVectorValueProcessor.java
@@ -45,7 +45,7 @@ public abstract class
DoubleOutLongsInFunctionVectorValueProcessor
@Override
public ExpressionType getOutputType()
{
- return ExpressionType.LONG;
+ return ExpressionType.DOUBLE;
}
@Override
diff --git
a/processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java
b/processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java
index 1a16d99d183..2e2c5ffee3c 100644
---
a/processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java
+++
b/processing/src/main/java/org/apache/druid/math/expr/vector/VectorProcessors.java
@@ -544,9 +544,11 @@ public class VectorProcessors
outputNulls[i] = rightNulls[i];
} else {
output[i] = rightInput[i];
+ outputNulls[i] = false;
}
} else {
output[i] = leftInput[i];
+ outputNulls[i] = false;
}
}
@@ -580,9 +582,11 @@ public class VectorProcessors
outputNulls[i] = rightNulls[i];
} else {
output[i] = rightInput[i];
+ outputNulls[i] = false;
}
} else {
output[i] = leftInput[i];
+ outputNulls[i] = false;
}
}
@@ -744,6 +748,7 @@ public class VectorProcessors
}
}
output[i] = Evals.asLong(Evals.asBoolean(leftInput[i]) ||
Evals.asBoolean(rightInput[i]));
+ outputNulls[i] = false;
}
@Override
@@ -793,6 +798,7 @@ public class VectorProcessors
}
}
output[i] = Evals.asLong(Evals.asBoolean(leftInput[i]) ||
Evals.asBoolean(rightInput[i]));
+ outputNulls[i] = false;
}
@Override
@@ -839,6 +845,7 @@ public class VectorProcessors
return;
}
output[i] = Evals.asLong(Evals.asBoolean((String) leftInput[i]) ||
Evals.asBoolean((String) rightInput[i]));
+ outputNulls[i] = false;
}
@Override
@@ -907,6 +914,7 @@ public class VectorProcessors
}
}
output[i] = Evals.asLong(Evals.asBoolean(leftInput[i]) &&
Evals.asBoolean(rightInput[i]));
+ outputNulls[i] = false;
}
@Override
@@ -916,7 +924,7 @@ public class VectorProcessors
}
},
() -> new BivariateFunctionVectorProcessor<double[], double[], long[]>(
- ExpressionType.DOUBLE,
+ ExpressionType.LONG,
left.asVectorProcessor(inputTypes),
right.asVectorProcessor(inputTypes)
)
@@ -956,6 +964,7 @@ public class VectorProcessors
}
}
output[i] = Evals.asLong(Evals.asBoolean(leftInput[i]) &&
Evals.asBoolean(rightInput[i]));
+ outputNulls[i] = false;
}
@Override
@@ -965,7 +974,7 @@ public class VectorProcessors
}
},
() -> new BivariateFunctionVectorProcessor<Object[], Object[], long[]>(
- ExpressionType.STRING,
+ ExpressionType.LONG,
left.asVectorProcessor(inputTypes),
right.asVectorProcessor(inputTypes)
)
@@ -1004,6 +1013,7 @@ public class VectorProcessors
output[i] = Evals.asLong(
Evals.asBoolean((String) leftInput[i]) &&
Evals.asBoolean((String) rightInput[i])
);
+ outputNulls[i] = false;
}
@Override
diff --git
a/processing/src/test/java/org/apache/druid/math/expr/VectorExprSanityTest.java
b/processing/src/test/java/org/apache/druid/math/expr/VectorExprSanityTest.java
index a032e993db0..d25220f0929 100644
---
a/processing/src/test/java/org/apache/druid/math/expr/VectorExprSanityTest.java
+++
b/processing/src/test/java/org/apache/druid/math/expr/VectorExprSanityTest.java
@@ -25,6 +25,7 @@ import org.apache.druid.java.util.common.NonnullPair;
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.java.util.common.logger.Logger;
import org.apache.druid.math.expr.vector.ExprEvalVector;
+import org.apache.druid.math.expr.vector.ExprVectorProcessor;
import org.apache.druid.testing.InitializedNullHandlingTest;
import org.junit.Assert;
import org.junit.Test;
@@ -113,7 +114,7 @@ public class VectorExprSanityTest extends
InitializedNullHandlingTest
public void testBinaryLogicOperators()
{
final String[] functions = new String[]{"&&", "||"};
- final String[] templates = new String[]{"d1 %s d2", "l1 %s l2",
"boolString1 %s boolString2"};
+ final String[] templates = new String[]{"d1 %s d2", "l1 %s l2",
"boolString1 %s boolString2", "(d1 == d2) %s (l1 == l2)"};
testFunctions(types, templates, functions);
}
@@ -283,21 +284,17 @@ public class VectorExprSanityTest extends
InitializedNullHandlingTest
log.debug("[%s]", expr);
Expr parsed = Parser.parse(expr, ExprMacroTable.nil());
- NonnullPair<Expr.ObjectBinding[], Expr.VectorInputBinding> bindings;
- for (int iterations = 0; iterations < NUM_ITERATIONS; iterations++) {
- bindings = makeRandomizedBindings(VECTOR_SIZE, types);
- testExpressionWithBindings(expr, parsed, bindings);
- }
- bindings = makeSequentialBinding(VECTOR_SIZE, types);
- testExpressionWithBindings(expr, parsed, bindings);
+ testExpression(expr, parsed, types, NUM_ITERATIONS);
+ testSequentialBinding(expr, parsed, types);
}
- public static void testExpressionWithBindings(
+ public static void testSequentialBinding(
String expr,
Expr parsed,
- NonnullPair<Expr.ObjectBinding[], Expr.VectorInputBinding> bindings
+ Map<String, ExpressionType> types
)
{
+ NonnullPair<Expr.ObjectBinding[], Expr.VectorInputBinding> bindings =
makeSequentialBinding(VECTOR_SIZE, types);
Assert.assertTrue(StringUtils.format("Cannot vectorize %s", expr),
parsed.canVectorize(bindings.rhs));
ExpressionType outputType = parsed.getOutputType(bindings.rhs);
ExprEvalVector<?> vectorEval =
parsed.asVectorProcessor(bindings.rhs).evalVector(bindings.rhs);
@@ -320,6 +317,55 @@ public class VectorExprSanityTest extends
InitializedNullHandlingTest
}
}
+ public static void testExpression(
+ String expr,
+ Expr parsed,
+ Map<String, ExpressionType> types,
+ int numIterations
+ )
+ {
+ Expr.InputBindingInspector inspector =
InputBindings.inspectorFromTypeMap(types);
+ Expr.VectorInputBindingInspector vectorInputBindingInspector = new
Expr.VectorInputBindingInspector()
+ {
+ @Override
+ public int getMaxVectorSize()
+ {
+ return VECTOR_SIZE;
+ }
+
+ @Nullable
+ @Override
+ public ExpressionType getType(String name)
+ {
+ return inspector.getType(name);
+ }
+ };
+ Assert.assertTrue(StringUtils.format("Cannot vectorize %s", expr),
parsed.canVectorize(inspector));
+ ExpressionType outputType = parsed.getOutputType(inspector);
+ final ExprVectorProcessor processor =
parsed.asVectorProcessor(vectorInputBindingInspector);
+ // 'null' expressions can have an output type of null, but still evaluate
in default mode, so skip type checks
+ if (outputType != null) {
+ Assert.assertEquals(expr, outputType, processor.getOutputType());
+ }
+ for (int iterations = 0; iterations < numIterations; iterations++) {
+ NonnullPair<Expr.ObjectBinding[], Expr.VectorInputBinding> bindings =
makeRandomizedBindings(VECTOR_SIZE, types);
+ ExprEvalVector<?> vectorEval = processor.evalVector(bindings.rhs);
+ final Object[] vectorVals = vectorEval.getObjectVector();
+ for (int i = 0; i < VECTOR_SIZE; i++) {
+ ExprEval<?> eval = parsed.eval(bindings.lhs[i]);
+ // 'null' expressions can have an output type of null, but still
evaluate in default mode, so skip type checks
+ if (outputType != null && !eval.isNumericNull()) {
+ Assert.assertEquals(eval.type(), outputType);
+ }
+ Assert.assertEquals(
+ StringUtils.format("Values do not match for row %s for expression
%s", i, expr),
+ eval.valueOrDefault(),
+ vectorVals[i]
+ );
+ }
+ }
+ }
+
public static NonnullPair<Expr.ObjectBinding[], Expr.VectorInputBinding>
makeRandomizedBindings(
int vectorSize,
Map<String, ExpressionType> types
@@ -332,7 +378,7 @@ public class VectorExprSanityTest extends
InitializedNullHandlingTest
types,
() -> r.nextLong(Integer.MAX_VALUE - 1),
r::nextDouble,
- r::nextBoolean,
+ () -> r.nextDouble(0, 1.0) > 0.9,
() -> String.valueOf(r.nextInt())
);
}
diff --git
a/processing/src/test/java/org/apache/druid/query/expression/VectorExpressionsSanityTest.java
b/processing/src/test/java/org/apache/druid/query/expression/VectorExpressionsSanityTest.java
index ba23a54a7ca..c8cfdf10065 100644
---
a/processing/src/test/java/org/apache/druid/query/expression/VectorExpressionsSanityTest.java
+++
b/processing/src/test/java/org/apache/druid/query/expression/VectorExpressionsSanityTest.java
@@ -22,7 +22,6 @@ package org.apache.druid.query.expression;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import org.apache.druid.java.util.common.DateTimes;
-import org.apache.druid.java.util.common.NonnullPair;
import org.apache.druid.java.util.common.logger.Logger;
import org.apache.druid.math.expr.Expr;
import org.apache.druid.math.expr.ExprEval;
@@ -62,13 +61,8 @@ public class VectorExpressionsSanityTest extends
InitializedNullHandlingTest
static void testExpression(String expr, Expr parsed, Map<String,
ExpressionType> types)
{
log.debug("[%s]", expr);
- NonnullPair<Expr.ObjectBinding[], Expr.VectorInputBinding> bindings;
- for (int iterations = 0; iterations < NUM_ITERATIONS; iterations++) {
- bindings = VectorExprSanityTest.makeRandomizedBindings(VECTOR_SIZE,
types);
- VectorExprSanityTest.testExpressionWithBindings(expr, parsed, bindings);
- }
- bindings = VectorExprSanityTest.makeSequentialBinding(VECTOR_SIZE, types);
- VectorExprSanityTest.testExpressionWithBindings(expr, parsed, bindings);
+ VectorExprSanityTest.testExpression(expr, parsed, types, NUM_ITERATIONS);
+ VectorExprSanityTest.testSequentialBinding(expr, parsed, types);
}
@Test
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
