Map BadQueryRequestException to QueryException.QUERY_VALIDATION_ERROR #14917
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14917 +/- ##
============================================
+ Coverage 61.75% 63.70% +1.95%
- Complexity 207 1471 +1264
============================================
Files 2436 2710 +274
Lines 133233 151911 +18678
Branches 20636 23459 +2823
============================================
+ Hits 82274 96775 +14501
- Misses 44911 47867 +2956
- Partials 6048 7269 +1221
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -111,7 +112,11 @@ protected void processSegments() { | |||
| @Override | |||
| protected void onProcessSegmentsException(Throwable t) { | |||
| _processingException.compareAndSet(null, t); | |||
| _blockingQueue.offer(new ExceptionResultsBlock(t)); | |||
| if (t instanceof BadQueryRequestException) { | |||
There was a problem hiding this comment.
In ServerQueryExecutorV1Impl, we are setting QUERY_VALIDATION_ERROR for BadQueryRequestException. Do we require the same changes here?
Either way I see this change only being made in BaseSingleBlockCombineOperator. What about GroupByCombineOperator and BaseStreamingCombineOperator?
There was a problem hiding this comment.
In ServerQueryExecutorV1Impl, we are setting QUERY_VALIDATION_ERROR for BadQueryRequestException. Do we require the same changes here?
It does. Without this change, generic RuntimeException is thrown to QueryDispatcher.java which will bypass that check.
There was a problem hiding this comment.
Either way I see this change only being made in BaseSingleBlockCombineOperator. What about GroupByCombineOperator and BaseStreamingCombineOperator?
Yeah just noticed these implement the same interface. Will update them as well
| ProcessingException queryException = QueryException.QUERY_EXECUTION_ERROR; | ||
| if (t instanceof BadQueryRequestException) { | ||
| // provide more specific error code if available | ||
| queryException = QueryException.QUERY_VALIDATION_ERROR; |
There was a problem hiding this comment.
We should update broker metrics - BrokerMeter.QUERY_VALIDATION_EXCEPTIONS ?
We should also update this in SingleStageBrokerRequestHandler?
There was a problem hiding this comment.
For SingleStageBrokerRequestHandler, this is not needed because BadQueryRequestException thrown by ServerQueryExecutorV1Impl is stored in BrokerResponseNative object. It's directly parsed in PinotClientRequest::getPinotQueryResponse.
For this MultiStageBrokerRequestHandler, this was needed because query was handled by _queryDispatcher.submitAndReduce which may throw an exception. The Java exception needs to be caught on the broker request handler level.
There was a problem hiding this comment.
Diagrams for these two error reporting scenarios. Many middle steps are omitted and only relevant steps are shown 👇🏼
PinotClientRequest::processSqlQueryPost
- BaseSingleStageBrokerRequestHandler::handleRequest
- SingleConnectionBrokerRequestHandler::processBrokerRequest // broker metric is updated here
- ServerQueryExecutorV1Impl::executeInternal // BadQueryRequestException is caught. InstanceResponseBlock is updated here
- SingleConnectionBrokerRequestHandler::processBrokerRequest // No exception is caught here. Error code is embedded inside brokerResponse
PinotClientRequest::getPinotQueryResponse // 700 error code is parsed
MultiStage query
PinotClientRequest::processSqlQueryPost
- MultiStageBrokerRequestHandler::handleRequest
- QueryDispatcher::submitAndReduce // exception is thrown if ANY block errors. I re-throw BadQueryRequestException here
- MultiStageBrokerRequestHandler::handleRequest // catch BadQueryRequestException. Exceptions in requestContext are added to brokerResponse via augmentStatistics
PinotClientRequest::getPinotQueryResponse // 700 error code is parsed
There was a problem hiding this comment.
Metric incremented now. Resolved
…BaseCombineOperator
| * Exception to contain info about QueryException errors. | ||
| * Throwable version of {@link org.apache.pinot.common.response.broker.QueryProcessingException} | ||
| */ | ||
| public class QueryInfoException extends RuntimeException { |
There was a problem hiding this comment.
Why this class?
QueryException is more of QueryExceptionUtil. It isn't throwable and rather a collection of static ProcessingException extends org.apache.thrift.TException. We need a throwable class that also contains information about ProcessingException.
Can't you use static QueryException.QUERY_VALIDATION_ERROR (or other from that family) directly?
Because they are basically org.apache.thrift.TException, they aren't suitable for general purpose exception. For instance, the message of QueryException.QUERY_VALIDATION_ERROR is statically set when defined/initialized.
Can't you use QueryProcessingException instead?
QueryProcessingException isn't throwable. It's named exception but it's really a QueryExceptionContainer used on broker side.
|
Hi there! I'm working on improving query error management. I've created #14950 to try to keep track of it and also started to create some exception hierarchies we can use. That is probably collision with changes made here. Are you planning to continue working on error management? |
|
@gortiz |
Description
Expand the usage of
BadQueryRequestExceptionwhen error cause is clear. Map it to QUERY_VALIDATION_ERROR (700) instead of QUERY_EXECUTION_ERROR (200)Now
BadQueryRequestExceptionis thrown in the following cases:IllegalArgumentExceptionis thrown inBaseCombineOperator.javaDetailed reasoning
Related to #14916
For some aggregation functions (sum/min/max), throws BadQueryRequestException instead of IllegalStateException because a non-numeric type is passed by client. An example of this is when the query is
This fails with
IllegalStateExceptionand is classified asQueryException.QUERY_EXECUTION_ERRORby default by constructor ofExceptionResultsBlock