kgyrtkirk commented on code in PR #15543:
URL: https://github.com/apache/druid/pull/15543#discussion_r1425227228
##########
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:
Why not use `DruidException` ?
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -357,17 +360,17 @@ public static ExprEval ofArray(ExpressionType outputType,
@Nullable Object[] val
* instead.
*/
@Deprecated
Review Comment:
is this method really `@Deprecated` ? doesn't looks like it..
##########
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
The handler part here is for when really unexpected stuff happens...
I think:
* `ExpressionValidationException` should not be handled
* `InvalidCastException` should be a `DruidException` and done...
##########
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) {
Review Comment:
why this should be so specific? I think `IAE` should be allowed with a plain
rethrow
##########
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
Review Comment:
if that's true; why catch it at all?
--
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]
