search

trinodb / trino

Official repository of Trino, the distributed SQL query engine for big data, formerly known as PrestoSQL (https://trino.io)
https://trino.io
Apache License 2.0
10.27k stars 2.95k forks source link

DateTimeException misclassified as INTERNAL_ERROR #5924

Open JamesRTaylor opened 3 years ago

JamesRTaylor commented 3 years ago

Seeing the following exception which ends up being classified as an INTERNAL_ERROR instead of a USER_ERROR:

java.time.DateTimeException: Invalid value for HourOfDay (valid values 0 - 23): 24
    at java.base/java.time.temporal.ValueRange.checkValidValue(ValueRange.java:311)
    at java.base/java.time.temporal.ChronoField.checkValidValue(ChronoField.java:717)
    at java.base/java.time.LocalTime.of(LocalTime.java:339)
    at java.base/java.time.LocalDateTime.of(LocalDateTime.java:362)
    at java.base/java.time.ZonedDateTime.of(ZonedDateTime.java:339)
    at io.prestosql.operator.scalar.timestamp.VarcharToTimestampCast.castToShortTimestamp(VarcharToTimestampCast.java:132)
    at io.prestosql.operator.scalar.timestamp.VarcharToTimestampCast.castToLegacyShortTimestamp(VarcharToTimestampCast.java:96)
    at io.prestosql.operator.scalar.timestamp.VarcharToTimestampCast.castToShort(VarcharToTimestampCast.java:57)
    at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:710)
    at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:735)
    at io.prestosql.sql.InterpretedFunctionInvoker.invoke(InterpretedFunctionInvoker.java:100)
    at io.prestosql.sql.planner.ExpressionInterpreter$Visitor.visitCast(ExpressionInterpreter.java:1133)
    at io.prestosql.sql.tree.Cast.accept(Cast.java:91)
    at io.prestosql.sql.tree.AstVisitor.process(AstVisitor.java:27)
    at io.prestosql.sql.planner.ExpressionInterpreter$Visitor.visitComparisonExpression(ExpressionInterpreter.java:747)
    at io.prestosql.sql.tree.ComparisonExpression.accept(ComparisonExpression.java:71)
    at io.prestosql.sql.tree.AstVisitor.process(AstVisitor.java:27)
    at io.prestosql.sql.planner.ExpressionInterpreter$Visitor.visitLogicalBinaryExpression(ExpressionInterpreter.java:861)
    at io.prestosql.sql.tree.LogicalBinaryExpression.accept(LogicalBinaryExpression.java:88)
    at io.prestosql.sql.tree.AstVisitor.process(AstVisitor.java:27)
    at io.prestosql.sql.planner.ExpressionInterpreter.optimize(ExpressionInterpreter.java:287)
    at io.prestosql.sql.planner.iterative.rule.SimplifyExpressions.rewrite(SimplifyExpressions.java:54)
    at io.prestosql.sql.planner.iterative.rule.SimplifyExpressions.lambda$createRewrite$0(SimplifyExpressions.java:79)
    at io.prestosql.sql.planner.iterative.rule.ExpressionRewriteRuleSet$FilterExpressionRewrite.apply(ExpressionRewriteRuleSet.java:226)
    at io.prestosql.sql.planner.iterative.rule.ExpressionRewriteRuleSet$FilterExpressionRewrite.apply(ExpressionRewriteRuleSet.java:207)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.transform(IterativeOptimizer.java:168)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreNode(IterativeOptimizer.java:143)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreGroup(IterativeOptimizer.java:108)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreChildren(IterativeOptimizer.java:193)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreGroup(IterativeOptimizer.java:110)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreChildren(IterativeOptimizer.java:193)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreGroup(IterativeOptimizer.java:110)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreChildren(IterativeOptimizer.java:193)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreGroup(IterativeOptimizer.java:110)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreChildren(IterativeOptimizer.java:193)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreGroup(IterativeOptimizer.java:110)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreChildren(IterativeOptimizer.java:193)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreGroup(IterativeOptimizer.java:110)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreChildren(IterativeOptimizer.java:193)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreGroup(IterativeOptimizer.java:110)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreChildren(IterativeOptimizer.java:193)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.exploreGroup(IterativeOptimizer.java:110)
    at io.prestosql.sql.planner.iterative.IterativeOptimizer.optimize(IterativeOptimizer.java:99)
    at io.prestosql.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:200)
    at io.prestosql.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:189)
    at io.prestosql.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:184)
    at io.prestosql.execution.SqlQueryExecution.doPlanQuery(SqlQueryExecution.java:433)
    at io.prestosql.execution.SqlQueryExecution.planQuery(SqlQueryExecution.java:421)
    at io.prestosql.execution.SqlQueryExecution.start(SqlQueryExecution.java:373)
    at io.prestosql.execution.SqlQueryManager.createQuery(SqlQueryManager.java:232)
    at io.prestosql.dispatcher.LocalDispatchQuery.lambda$startExecution$7(LocalDispatchQuery.java:132)
    at io.prestosql.$gen.Presto_338_12____20201102_161224_2.run(Unknown Source)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:834)
findepi commented 3 years ago

The VarcharToTimestampCast should catch DateTimeException and rethrow as PrestoException with proper error code.

martint commented 3 years ago

It might be better to just validate the range of values for each field directly. There's an opportunity for producing better error messages.

johnwhumphreys commented 3 years ago

I'll try to do this one; assigned to myself. Found that it can be reproduced with:

select cast('2020-01-01 24:01:01' as timestamp)

java.time.DateTimeException: Invalid value for HourOfDay (valid values 0 - 23): 24 at java.base/java.time.temporal.ValueRange.checkValidValue(ValueRange.java:311) at java.base/java.time.temporal.ChronoField.checkValidValue(ChronoField.java:717)

And it does report INTERNAL_ERROR/GENERIC_INTERNAL_ERROR (65536) on the UI.

johnwhumphreys commented 3 years ago

@martint

It might be better to just validate the range of values for each field directly. There's an opportunity for producing better error messages.

The DateTimeException has very good messages already, so I think we can just leverage them to give good messages out of the original error. Then we can avoid adding much extra logic. E.g.

        catch (DateTimeException e) {
            //Leverage highly specific error message from the source exception.
            throw new PrestoException(INVALID_CAST_ARGUMENT,
                    String.format("Value cannot be cast to timestamp; %s. ", e.getMessage()), e);
        }

Produces a user-error like:

SQL Error [9]: Query failed (#20201115_215031_00010_bqy8y): Value cannot be cast to timestamp; Invalid value for HourOfDay (valid values 0 - 23): 24.

or:

SQL Error [9]: Query failed (#20201115_215055_00011_bqy8y): Value cannot be cast to timestamp; Invalid value for MonthOfYear (valid values 1 - 12): 13.

For these two bad cast values respectively.

select cast('2020-13-01 23:61:01' as timestamp);
select cast('2020-13-01 23:59:01' as timestamp);

Does this look okay to you approach-wise?

johnwhumphreys commented 3 years ago

@martint - Made a PR with the above fix and some text cases for you to review. Happy to change it if you have any issues. Thanks!

rnzucker commented 3 years ago

This is still listed as Open, but it looks like it has been fixed. Is that correct?

hashhar commented 3 years ago

@rnzucker Thanks for bumping this up - I see the PR is still open ~but I cannot reproduce the behaviour anymore with select cast('2020-01-01 24:01:01' as timestamp).~

~Closing. Feel free to re-open if needed.~

EDIT: I apparently misread the issue, it's still classified as an INTERNAL_ERROR. The PR needs to be rebased and should be good to go then.

rnzucker commented 3 years ago

Okay. I was just looking for issues for my son to start on for his first OSS project (in Java). Hence my question.