clintropolis commented on code in PR #15543:
URL: https://github.com/apache/druid/pull/15543#discussion_r1425883476
##########
processing/src/main/java/org/apache/druid/math/expr/FunctionalExpr.java:
##########
@@ -190,11 +191,22 @@ public ExprEval eval(ObjectBinding bindings)
try {
return function.apply(args, bindings);
}
- catch (DruidException | ExpressionValidationException e) {
+ catch (ExpressionValidationException e) {
+ // ExpressionValidationException already contain function name
+ throw DruidException.forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .build(e.getMessage());
+ }
+ catch (Types.InvalidCastException | Types.InvalidCastBooleanException e) {
+ throw DruidException.forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .build("Function[%s] encountered exception: %s",
name, e.getMessage());
Review Comment:
>this will throw away the stacktrace - the old version was retaining that
I don't think the stack trace is particularly useful for either of these
since the exception message clearly says the problem and they are not caused by
bugs, but rather bad user input. `defensive` is expected to be a bug, so the
stack trace is a lot more important.
> ExpressionValidationException should not be handled
I handled this here because it should be a user exception and have a 400
response, it results from the user writing the function incorrectly. If not
handled here, it needs to be handled somewhere else to make it a user exception.
>InvalidCastException should be a DruidException and done...
The problem with making it a `DruidException` when it is thrown is that
where it is thrown from we don't have the context of function name, so catching
it to decorate with function name isn't particularly straightforward at the
moment afaik. This is also a user exception and should have a 400 response.
I'm still not completely convinced that expressions should be throwing
`DruidException`... While `prependAndBuild` would partially solve these needs,
it also reduces the exception cause to basically a string message, and all we
can really do with it is decorate it with more strings. I'm unsure if this is
too limiting, still thinking a bit about it, but didn't really want to change
too much unrelated stuff in this PR. Currently afaik this is the only place in
native expressions throw `DruidException`, and it only is handled for
non-vectorized expressions. Vectorized expression processing for functions just
make a `ExprVectorProcessor` like all other expressions, so we can't actually
handle this generically in a centralized place, and `ApplyFunctionExpr` doesn't
have this same handling that `FunctionExpr` has, so I find things are in a bit
of a strange inconsistent state.
Ideally, we should catch all exception from outside of expressions from
where they are called, like selectors and vector selectors of
`ExpressionVirtualColumn`, `ExpressionFilter`,
`ExpressionLambdaAggregatorFactory` aggs, and `ExpressionPostAggregator`, so
that they could add the additional context of like what the expression belongs
to. The question I have is if `DruidException` is enough, or if we should have
native expression exceptions that the things that catch them know how to
appropriately translate them into `DruidException`, but I don't really want to
address that in this PR.
##########
processing/src/main/java/org/apache/druid/segment/column/Types.java:
##########
@@ -141,4 +141,20 @@ public IncompatibleTypeException(TypeSignature<?> type,
TypeSignature<?> other)
super("Cannot implicitly cast [%s] to [%s]", type, other);
}
}
+
+ public static class InvalidCastException extends IAE
Review Comment:
see other comment
--
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]
