clintropolis commented on code in PR #18549:
URL: https://github.com/apache/druid/pull/18549#discussion_r2362000495
##########
processing/src/test/java/org/apache/druid/math/expr/VectorExprResultConsistencyTest.java:
##########
@@ -840,4 +961,62 @@ static NonnullPair<Expr.ObjectBinding[],
Expr.VectorInputBinding> populateBindin
return new NonnullPair<>(objectBindings, vectorBinding);
}
+
+ private static Either<String, Object[]> evalVector(
+ Expr expr,
+ Expr.VectorInputBinding bindings,
+ @Nullable ExpressionType outputType
+ )
+ {
+ final ExprVectorProcessor<Object> processor =
expr.asVectorProcessor(bindings);
+ final ExprEvalVector<?> vectorEval;
+ try {
+ vectorEval = processor.evalVector(bindings);
+ }
+ catch (ArithmeticException e) {
+ // After a few occasions, Java starts throwing ArithmeticException
without a message for division by zero.
+ return Either.error(e.getClass().getName());
+ }
+ catch (Exception e) {
+ return Either.error(e.toString());
+ }
+
+ final Object[] vectorVals = vectorEval.getObjectVector();
+ // 'null' expressions can have an output type of null, but still evaluate
in default mode, so skip type checks
Review Comment:
nit: i know these aren't new comments (there is another one a bit later),
but consider removing reference to default value mode, i think i missed this,
its just talking about how outputType isn't reliable for null values so it was
skipping comparison
##########
processing/src/main/java/org/apache/druid/math/expr/vector/FunctionErrorReportingExprVectorProcessor.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.math.expr.vector;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.math.expr.Expr;
+import org.apache.druid.math.expr.ExpressionProcessingException;
+import org.apache.druid.math.expr.ExpressionType;
+import org.apache.druid.math.expr.ExpressionValidationException;
+import org.apache.druid.segment.column.Types;
+
+/**
+ * Wrapper around {@link ExprVectorProcessor} that reports errors in the same
style as {@code FunctionExpr}.
+ */
+public class FunctionErrorReportingExprVectorProcessor<T> implements
ExprVectorProcessor<T>
+{
+ private final String functionName;
+ private final ExprVectorProcessor<T> delegate;
+
+ public FunctionErrorReportingExprVectorProcessor(String functionName,
ExprVectorProcessor<T> delegate)
+ {
+ this.functionName = functionName;
+ this.delegate = delegate;
+ }
+
+ @Override
+ public ExprEvalVector<T> evalVector(Expr.VectorInputBinding bindings)
+ {
+ try {
+ return delegate.evalVector(bindings);
+ }
+ catch (ExpressionValidationException | ExpressionProcessingException e) {
+ // Already contain function name
+ throw DruidException.forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .build(e, e.getMessage());
+ }
+ catch (Types.InvalidCastException | Types.InvalidCastBooleanException e) {
+ throw DruidException.forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .build(e, "Function[%s] encountered exception: %s",
functionName, e.getMessage());
+ }
+ catch (DruidException e) {
+ throw e;
Review Comment:
i guess there are probably some cases where this path loses the function
name? I'm not sure if it matters all that much, but this is part of why i've
been a bit hesitant on migrating to using `DruidException` inside expr
processing in favor of still using the expression specific exceptions and
catching them in the callers to turn into `DruidException`. I feel like its
kind of hard to know if a more general exception like DruidException should be
caught and wrapped or rethrown.
All that said, this is fine i think too, since this is mostly just my opinion
--
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]
