fix expression post aggregator array handling when grouping wrapper types leak #15543
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -207,7 +207,7 @@ | |||
| break; | |||
| } | |||
| if (!ExpressionProcessing.useStrictBooleans() && !type.is(ExprType.STRING) && !type.isArray()) { | |||
| return ExprEval.ofBoolean(result, type.getType()); | |||
| return ExprEval.ofBoolean(result, type); | |||
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
| @@ -184,7 +184,7 @@ | |||
| if (!ExpressionProcessing.useStrictBooleans()) { | |||
| // conforming to other boolean-returning binary operators | |||
| ExpressionType retType = ret.type().is(ExprType.DOUBLE) ? ExpressionType.DOUBLE : ExpressionType.LONG; | |||
| return ExprEval.ofBoolean(!ret.asBoolean(), retType.getType()); | |||
| return ExprEval.ofBoolean(!ret.asBoolean(), retType); | |||
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
| () -> testHelper.testExpressionString( | ||
| roundFunction, | ||
| ImmutableList.of( | ||
| DruidExpression.ofColumn(ColumnType.FLOAT, "x"), | ||
| DruidExpression.ofStringLiteral("foo") | ||
| ) | ||
| ), | ||
| "IAE Exception" | ||
| testHelper.makeInputRef("x"), | ||
| testHelper.makeLiteral("foo") | ||
| ), | ||
| DruidExpression.ofExpression( | ||
| ColumnType.FLOAT, | ||
| DruidExpression.functionCall("round"), | ||
| ImmutableList.of( | ||
| DruidExpression.ofColumn(ColumnType.FLOAT, "x"), | ||
| DruidExpression.ofStringLiteral("foo") | ||
| ) | ||
| ), | ||
| "IAE Exception" | ||
| ) |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| () -> testHelper.testExpressionString( | ||
| new RightOperatorConversion().calciteOperator(), | ||
| ImmutableList.of( | ||
| testHelper.makeInputRef("s"), | ||
| testHelper.makeLiteral(-1) | ||
| ), | ||
| makeExpression("right(\"s\",-1)"), | ||
| null | ||
| ) |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| testHelper.makeInputRef("s"), | ||
| testHelper.makeLiteral(-1) | ||
| ), | ||
| makeExpression("right(\"s\",-1)"), |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| testHelper.makeInputRef("s"), | ||
| testHelper.makeLiteral(-1) | ||
| ), | ||
| makeExpression("left(\"s\",-1)"), |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| () -> testHelper.testExpressionString( | ||
| new LeftOperatorConversion().calciteOperator(), | ||
| ImmutableList.of( | ||
| testHelper.makeInputRef("s"), | ||
| testHelper.makeInputRef("s") | ||
| ), | ||
| makeExpression("left(\"s\",\"s\")"), | ||
| null | ||
| ) |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| testHelper.makeInputRef("s"), | ||
| testHelper.makeInputRef("s") | ||
| ), | ||
| makeExpression("left(\"s\",\"s\")"), |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| () -> testHelper.testExpressionString( | ||
| new RepeatOperatorConversion().calciteOperator(), | ||
| ImmutableList.of( | ||
| testHelper.makeInputRef("s"), | ||
| testHelper.makeInputRef("s") | ||
| ), | ||
| makeExpression("repeat(\"s\",\"s\")"), | ||
| null | ||
| ) |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| testHelper.makeInputRef("s"), | ||
| testHelper.makeInputRef("s") | ||
| ), | ||
| makeExpression("repeat(\"s\",\"s\")"), |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
|
I did a build of this and I can confirm that this fixes the error that I brought up. |
| 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()); |
There was a problem hiding this 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:
ExpressionValidationExceptionshould not be handledInvalidCastExceptionshould be aDruidExceptionand done...
There was a problem hiding this 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.defensiveis 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 aDruidExceptionwhen 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.
There was a problem hiding this comment.
okay - I don't have any big expectation from these things ; just 1: if there was a parent exception it should be set for the new exception ; please restore that behavior somehow - if the exception will get logged for some reason that part must also be logged; this is also usefull when a test fails for whatever reason: having the full stacktrace will make it possible to navigate to the place where the issue happened.
I think DruidException is designed oddly - but its still possible to pass the parent exception to the build method (I think the other method should be dropped - so that when it gets invoked the developer must fill it to null to get that behaviour - which is less errorprone)
There was a problem hiding this comment.
updated to retain cause exception for the user exceptions
| @@ -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 | |||
| .ofCategory(DruidException.Category.INVALID_INPUT) | ||
| .build(e.getMessage()); | ||
| } | ||
| catch (Types.InvalidCastException | Types.InvalidCastBooleanException e) { |
There was a problem hiding this comment.
why this should be so specific? I think IAE should be allowed with a plain rethrow
There was a problem hiding this comment.
see other comment about translating into invalid input/user exception so it makes the appropriate type of error response and not a system error
| @@ -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 | |||
There was a problem hiding this comment.
if that's true; why catch it at all?
There was a problem hiding this comment.
same comment about making invalid input/user error short of catching it somewhere else
| @@ -357,17 +360,17 @@ public static ExprEval ofArray(ExpressionType outputType, @Nullable Object[] val | |||
| * instead. | |||
| */ | |||
| @Deprecated | |||
There was a problem hiding this comment.
is this method really @Deprecated ? doesn't looks like it..
There was a problem hiding this comment.
it is only used in 2 places, and ideally nothing new should use it, it is for the legacy boolean handling mode (strictBooleans = false)
|
actually I was looking around yesterday for some tool which could help with a tricky refactoring step - I bumped into one which looked to much at first glance and I needed a few minutes to remember the name of the project...but it has a recipe to do something similar :D https://docs.openrewrite.org/recipes/java/testing/junit5/removetrycatchfailblocks it also have junit4to5 stuff... I'll probably try it out someday :) |
Description
Fixes an issue which can occur when using
ExpressionPostAggregatoron on array types output from grouping engine in certain situations such as inline data. The grouping engine uses wrapper types for arrays,ComparableStringArrayandComparableListso that they implementComparable(current requirement for all grouping dimension types). The native expression system did not recognize these types, leading to them being treated asCOMPLEXrather thanARRAYtypes, which made stuff sad.This also made an adjustment to
FunctionalExprwhich after #14987 was causing things like cast exceptions to eventually end up going throughDruidException.defensiveand presented as unknown server-side errors. While in this case the class cast exception really was a bug, in most cases it would not be, and so these are now handled as user errors so that the exception message is now shown to the user.This PR has: