Refactor query error handling to use QueryErrorCode and QueryErrorMessage for improved clarity and consistency #15037

gortiz · 2025-02-12T11:37:19Z

This PR derives from #14994. Here, I'm applying the bigger refactors required to clean up the error hierarchy. Although the number of lines changed is pretty extensive, the actual changes are:

Create some new classes in org.apache.pinot.spi.exception:
1. PinotQueryException, which should be used as our main runtime class.
2. QueryException, which extends PinotQueryException. This class substitutes most usages of ProcessingException, which was checked and includes tons of unnecessary thrift code. It is important to notice that this class differs from the older org.apache.pinot.common.exception.QueryException).
3. QueryErrorCode, an enum that substitutes the integer error codes in the older QueryException.
4. QueryErrorMessage, which contains a QueryErrorCode and two strings: one that can be shown to the user and one that could be used internally (mainly to be logged if needed).
The older QueryException class (which did not actually extend the exception) has been deleted. Its responsibilities have been moved to the new classes QueryException and QueryErrorCode.
Most uses of ProcessingException have been replaced with QueryException (when we actually need to throw it) or QueryErrorMessage when we need to keep the error code and messages in memory (usually to send them to other nodes).
In this version, errors received by customers never include stack traces. Instead, some messages have been improved, and the idea is to continue improving them in subsequent PRs, trying to achieve something closer to what Improve exceptions broker #14994 was doing.
Some error codes have changed. For example the query testQueryException("SELECT COUNT(*) FROM mytable where ArrTime = 'potato'" fails with QUERY_EXECUTION in MSE (as you can see in test BaseClusterIntegrationTestSet

…sage for improved clarity and consistency

…ge constructor for improved JSON serialization

… error codes instead of messages

…lingTest to focus on error codes

…stead of messages

codecov-commenter · 2025-02-12T15:28:19Z

Codecov Report

Attention: Patch coverage is 50.55432% with 223 lines in your changes missing coverage. Please review.

Project coverage is 63.63%. Comparing base (59551e4) to head (f3afeca).
Report is 2193 commits behind head on master.

Files with missing lines	Patch %	Lines
...sthandler/BaseSingleStageBrokerRequestHandler.java	7.89%	35 Missing ⚠️
...requesthandler/MultiStageBrokerRequestHandler.java	0.00%	29 Missing ⚠️
...t/controller/api/resources/PinotQueryResource.java	25.00%	21 Missing ⚠️
.../pinot/query/service/dispatch/QueryDispatcher.java	17.64%	13 Missing and 1 partial ⚠️
...erator/streaming/BaseStreamingCombineOperator.java	23.07%	9 Missing and 1 partial ⚠️
...apache/pinot/query/service/server/QueryServer.java	35.71%	9 Missing ⚠️
.../apache/pinot/spi/exception/QueryErrorMessage.java	40.00%	7 Missing and 2 partials ⚠️
.../core/operator/combine/GroupByCombineOperator.java	41.66%	6 Missing and 1 partial ⚠️
...roker/requesthandler/BaseBrokerRequestHandler.java	0.00%	6 Missing ⚠️
...nMaxValueBasedSelectionOrderByCombineOperator.java	14.28%	6 Missing ⚠️
... and 30 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #15037      +/-   ##
============================================
+ Coverage     61.75%   63.63%   +1.88%     
- Complexity      207     1461    +1254     
============================================
  Files          2436     2764     +328     
  Lines        133233   155674   +22441     
  Branches      20636    23935    +3299     
============================================
+ Hits          82274    99069   +16795     
- Misses        44911    49129    +4218     
- Partials       6048     7476    +1428

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.59% <50.33%> (+1.88%)`	⬆️
java-21	`63.46% <50.55%> (+1.83%)`	⬆️
skip-bytebuffers-false	`63.61% <50.33%> (+1.86%)`	⬆️
skip-bytebuffers-true	`63.44% <50.55%> (+35.71%)`	⬆️
temurin	`63.63% <50.55%> (+1.88%)`	⬆️
unittests	`63.63% <50.55%> (+1.88%)`	⬆️
unittests1	`56.18% <60.88%> (+9.29%)`	⬆️
unittests2	`34.04% <22.39%> (+6.30%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…tions during result streaming

…n various classes

… cleanUpQueryExceptions

gortiz · 2025-02-28T15:59:33Z

I've pushed several changes to fix most of the comments from @yashmayya. I think I also commented on the open discussions

vrajat · 2025-03-03T05:01:00Z

I'll take a look at this PR today.

yashmayya · 2025-03-03T05:01:24Z

@gortiz I didn't follow why we reintroduced the truncated stack traces in the error data blocks here? Are you planning to remove it in a follow-up PR instead? Any reason we couldn't have removed it here itself like you'd done previously?

vrajat · 2025-03-03T10:32:55Z

pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java

   */
-  List<QueryProcessingException> getExceptions();
+  // TODO: Rename this method
+  List<BrokerResponseErrorMessage> getExceptions();


This change will cause backward incompatibility for clients. If you search for QueryProcessingException in:

org:startreedata - there are multiple places where it is used in startree-pinot. It will help if you have a PR ready to remove references as it is not trivial for others to fix the compilation issue

All of github - It is used trinodb. https://github.com/trinodb/trino/pull/14399/files

I suggest you deprecate and keep this function as is. Anyway you plan to rename it.

This PR will break backward compatibility, that is expected

Oh, I see, that is part of the driver. We should not break backward compatibility there.

I've renamed the name back to the original one

vrajat · 2025-03-03T10:33:18Z

pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java

  @Deprecated
  @JsonIgnore
-  default List<QueryProcessingException> getProcessingExceptions() {
+  default List<BrokerResponseErrorMessage> getProcessingExceptions() {


Same here. Keep as is.

gortiz · 2025-03-05T11:06:51Z

I didn't follow why we reintroduced the truncated stack traces in the error data blocks here? Are you planning to remove it in a follow-up PR instead? Any reason we couldn't have removed it here itself like you'd done previously?

I still plan to clean up most stack traces and try to not send them to the final user. But that change should be done at the same time we improve the error codes, messages and logs. That is why I think it is better to keep the stack traces in this PR and then create one where we apply the other changes.

gortiz · 2025-03-05T15:22:13Z

checks are green again

yashmayya · 2025-03-07T06:04:07Z

pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java

+      try {
+        arguments[i] = parameterType.convert(argument, argumentType);
+      } catch (Exception e) {
+        String errorMsg = "Invalid conversion when calling " + _method + ": " + e.getMessage();
+        throw QueryErrorCode.QUERY_EXECUTION.asException(errorMsg, e);
+      }


This is called in various ingestion paths as well, I don't think we should be using query execution error here.

AFAIK, when ingestion calls this code, what they are using is to evaluate some built-in functions to apply transformations. I think it makes sense to receive query exceptions in that case. Don't you?

I'm not sure I agree that query processing related exceptions should ever be used in ingestion related error paths even if function related (since those are common components). But I'd prefer to not block this PR any longer, so we can discuss this further separately.

pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java

pinot-spi/src/main/java/org/apache/pinot/spi/exception/EarlyTerminationException.java

yashmayya · 2025-03-10T09:56:07Z

pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java

+      try {
+        arguments[i] = parameterType.convert(argument, argumentType);
+      } catch (Exception e) {
+        String errorMsg = "Invalid conversion when calling " + _method + ": " + e.getMessage();
+        throw QueryErrorCode.QUERY_EXECUTION.asException(errorMsg, e);
+      }


I'm not sure I agree that query processing related exceptions should ever be used in ingestion related error paths even if function related (since those are common components). But I'd prefer to not block this PR any longer, so we can discuss this further separately.

…e compatibility

…ng and directly throwing QueryException

gortiz · 2025-03-10T10:21:51Z

I've pushed a change where I'm creating a new QueryFunctionInvoker. The old FunctionInvoker is now only called by ingestion code and the new QueryFunctionInvoker. The latter delegates to on FunctionInvoker but wraps exceptions into QueryExceptions. It also improves the error message in case the method throws an exception, removing the unnecessary InvocationTargetException in the middle and returning INTERNAL error in the unlikely case that the call fails with an IllegalAccessException

Refactor query error handling to use QueryErrorCode and QueryErrorMes…

9fd0886

…sage for improved clarity and consistency

gortiz force-pushed the cleanUpQueryExceptions branch from 2308828 to 9fd0886 Compare February 12, 2025 11:44

gortiz added 10 commits February 12, 2025 13:00

Add JsonCreator and JsonProperty annotations to BrokerQueryErrorMessa…

d19c41d

…ge constructor for improved JSON serialization

Make SqlCompilationException a QueryException

744226a

Return the correct error code when catching QueryExceptions

36f89f1

Fix two test that was expecting an older error message

77231d5

Improve error message in BaseCombineOperator

5ac9c00

Fix a test that was expecting an older error message

8d8d85b

Remove exception name from some errors

e609c93

Update error code assertions in OfflineClusterIntegrationTest to test…

57b6f27

… error codes instead of messages

Refactor exception assertions in OfflineClusterMemBasedServerQueryKil…

144f8e1

…lingTest to focus on error codes

Refactor QueryQuotaClusterIntegrationTest to assert on error codes in…

b69f7a4

…stead of messages

gortiz added 7 commits February 13, 2025 09:09

Keep QueryException error codes in BaseCombineOperator

cb22097

Adapt test to the new more precise errors in MSE

4ba96e0

Fix error handling in QueryDispatcher and its tests

d9074d8

Merge remote-tracking branch 'origin/master' into cleanUpQueryExceptions

50e1860

Remove unused import for TimeoutException in QueryRunnerTestBase

6868df7

fix imports

a51c854

Enhance error message in QueryServer to include exception details

e7b67ea

gortiz marked this pull request as ready for review February 13, 2025 13:14

gortiz requested review from Jackie-Jiang, vrajat and yashmayya and removed request for Jackie-Jiang, vrajat and yashmayya February 13, 2025 13:14

gortiz added 4 commits February 28, 2025 16:45

Enhance logging in StreamingInstanceResponseOperator to capture excep…

a41a6d5

…tions during result streaming

Refactor error handling to use default messages from QueryErrorCode i…

b134915

…n various classes

Merge branch 'cleanUpQueryExceptions' of github.com:gortiz/pinot into…

b53335b

… cleanUpQueryExceptions

Merge remote-tracking branch 'origin/master' into cleanUpQueryExceptions

baf1b39

vrajat reviewed Mar 3, 2025

View reviewed changes

gortiz added 4 commits March 5, 2025 12:19

Fix compilation issues

0e8d652

Renamed BrokerResponseErrorMessage back to QueryProcessingException

a762f9a

Merge remote-tracking branch 'origin/master' into cleanUpQueryExceptions

9674f98

Reorganize imports

98a8e4f

gortiz mentioned this pull request Mar 6, 2025

Improve error handling and reporting in query side #14950

Open

9 tasks

yashmayya reviewed Mar 7, 2025

View reviewed changes

vrajat reviewed Mar 7, 2025

View reviewed changes

pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java Outdated Show resolved Hide resolved

pinot-spi/src/main/java/org/apache/pinot/spi/exception/EarlyTerminationException.java Show resolved Hide resolved

gortiz added 2 commits March 10, 2025 10:20

Merge remote-tracking branch 'origin/master' into cleanUpQueryExceptions

902fedb

Remove TODO comment for renaming getExceptions method in BrokerResponse

d5e9253

yashmayya approved these changes Mar 10, 2025

View reviewed changes

vrajat approved these changes Mar 10, 2025

View reviewed changes

gortiz added 2 commits March 10, 2025 11:16

Refactor function invoker to use QueryFunctionInvoker for query engin…

f33ff03

…e compatibility

Simplify exception handling in QueryFunctionInvoker by removing loggi…

149cc89

…ng and directly throwing QueryException

gortiz added 2 commits March 10, 2025 11:26

Add license

3c03250

Fix incorrect error code introducing on merge.

f3afeca

gortiz merged commit bc567a0 into apache:master Mar 10, 2025
21 of 22 checks passed

gortiz deleted the cleanUpQueryExceptions branch March 10, 2025 13:14

gortiz mentioned this pull request Mar 13, 2025

[multistage] Replace LogicalTableScan with PinotLogicalTableScan #15225

Merged

Refactor query error handling to use QueryErrorCode and QueryErrorMessage for improved clarity and consistency #15037

Refactor query error handling to use QueryErrorCode and QueryErrorMessage for improved clarity and consistency #15037

Uh oh!

Conversation

gortiz commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gortiz commented Feb 28, 2025

Uh oh!

vrajat commented Mar 3, 2025

Uh oh!

yashmayya commented Mar 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gortiz commented Mar 5, 2025

Uh oh!

gortiz commented Mar 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gortiz commented Mar 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gortiz commented Feb 12, 2025 •

edited

Loading

codecov-commenter commented Feb 12, 2025 •

edited

Loading