-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Better query error classification for user errors #18949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better query error classification for user errors #18949
Conversation
…errors (InvalidSqlInput) for invalid queries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change makes sense to me – thank you! Left a few comments. Looks like we're also failing coverage in a few places.
...d/query/aggregation/datasketches/quantiles/sql/DoublesSketchApproxQuantileSqlAggregator.java
Outdated
Show resolved
Hide resolved
.../test/java/org/apache/druid/spectator/histogram/sql/SpectatorHistogramSqlAggregatorTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/apache/druid/spectator/histogram/sql/SpectatorHistogramSqlAggregatorTest.java
Outdated
Show resolved
Hide resolved
…for coverage, add a template for getting numeric literal and throwing DruidException when parameter type is invalid
jtuglu1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left 1 last comment. LGTM pending CI passing.
...java/org/apache/druid/spectator/histogram/sql/SpectatorHistogramPercentileSqlAggregator.java
Outdated
Show resolved
Hide resolved
...java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchBaseSqlAggregator.java
Outdated
Show resolved
Hide resolved
...d/query/aggregation/datasketches/quantiles/sql/DoublesSketchApproxQuantileSqlAggregator.java
Outdated
Show resolved
Hide resolved
.../org/apache/druid/query/aggregation/datasketches/theta/sql/ThetaSketchBaseSqlAggregator.java
Outdated
Show resolved
Hide resolved
...am/src/main/java/org/apache/druid/query/aggregation/histogram/sql/QuantileSqlAggregator.java
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/StringSqlAggregator.java
Outdated
Show resolved
Hide resolved
maytasm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
|
PR is ready for merge, thanks! |
Description
Some invalid queries, like:
select SPECTATOR_PERCENTILE(null, '99.99') from "foo"throw
which causes Druid to incorrectly return 500 to the user instead of 400 (user error).
This change checks instanceof before casting RexLiteral.value() to Number in SQL aggregators. When users pass invalid queries (e.g., a string literal '99.99' where numeric literals are expected),
InvalidSqlInputexception is thrown, which returns 400 (USER/INVALID_INPUT) instead of 500 (ADMIN/UNCATEGORIZED). This improves error diagnostics for invalid queries.Key changed/added classes in this PR
SpectatorHistogramPercentileSqlAggregatorHllSketchBaseSqlAggregatorDoublesSketchApproxQuantileSqlAggregatorThetaSketchBaseSqlAggregatorBloomFilterSqlAggregatorQuantileSqlAggregatorArrayConcatSqlAggregatorArraySqlAggregatorStringSqlAggregatorRelease note
Improved: type validation for numeric literal parameters in SQL aggregator functions to prevent ClassCastException and provide better error message categorized as user error