Skip to content

Commit e4a16b2

Browse files
gortizgortiz
committed
Enhance error handling and update expected exceptions in query tests
1 parent 37be2e2 commit e4a16b2

File tree

7 files changed

+29
-17
lines changed

7 files changed

+29
-17
lines changed

‎pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@
8282
import org.apache.pinot.spi.config.table.UpsertConfig;
8383
import org.apache.pinot.spi.env.PinotConfiguration;
8484
import org.apache.pinot.spi.exception.BadQueryRequestException;
85+
import org.apache.pinot.spi.exception.QException;
8586
import org.apache.pinot.spi.exception.QueryCancelledException;
8687
import org.apache.pinot.spi.plugin.PluginManager;
8788
import org.apache.pinot.spi.trace.LoggerConstants;
@@ -342,7 +343,7 @@ private InstanceResponseBlock executeInternal(ServerQueryRequest queryRequest, E
342343
// Do not log verbose error for BadQueryRequestException and QueryCancelledException.
343344
if (e instanceof BadQueryRequestException) {
344345
LOGGER.info("Caught BadQueryRequestException while processing requestId: {}, {}", requestId, e.getMessage());
345-
instanceResponse.addException(QueryException.getException(QueryException.QUERY_VALIDATION_ERROR, e));
346+
instanceResponse.addException(QException.QUERY_VALIDATION_ERROR_CODE, e.getMessage());
346347
} else if (e instanceof QueryCancelledException) {
347348
if (LOGGER.isDebugEnabled()) {
348349
LOGGER.debug("Cancelled while processing requestId: {}", requestId, e);
@@ -352,12 +353,12 @@ private InstanceResponseBlock executeInternal(ServerQueryRequest queryRequest, E
352353
// NOTE most likely the onFailure() callback registered on query future in InstanceRequestHandler would
353354
// return the error table to broker sooner than here. But in case of race condition, we construct the error
354355
// table here too.
355-
instanceResponse.addException(QueryException.getException(QueryException.QUERY_CANCELLATION_ERROR,
356-
"Query cancelled on: " + _instanceDataManager.getInstanceId() + " " + e));
356+
instanceResponse.addException(QException.QUERY_CANCELLATION_ERROR_CODE,
357+
"Query cancelled on: " + _instanceDataManager.getInstanceId() + " " + e.getMessage());
357358
} else {
358359
LOGGER.error("Exception processing requestId {}", requestId, e);
359-
instanceResponse.addException(QueryException.getException(QueryException.QUERY_EXECUTION_ERROR,
360-
"Query execution error on: " + _instanceDataManager.getInstanceId() + " " + e));
360+
instanceResponse.addException(QException.QUERY_EXECUTION_ERROR_CODE,
361+
"Query execution error on: " + _instanceDataManager.getInstanceId() + " " + e.getMessage());
361362
}
362363
} finally {
363364
for (SegmentDataManager segmentDataManager : segmentDataManagers) {
@@ -389,9 +390,9 @@ private InstanceResponseBlock executeInternal(ServerQueryRequest queryRequest, E
389390
.collect(Collectors.toList());
390391
int numMissingSegments = missingSegments.size();
391392
if (numMissingSegments > 0) {
392-
instanceResponse.addException(QueryException.getException(QueryException.SERVER_SEGMENT_MISSING_ERROR,
393+
instanceResponse.addException(QException.SERVER_SEGMENT_MISSING_ERROR_CODE,
393394
numMissingSegments + " segments " + missingSegments + " missing on server: "
394-
+ _instanceDataManager.getInstanceId()));
395+
+ _instanceDataManager.getInstanceId());
395396
_serverMetrics.addMeteredTableValue(tableNameWithType, ServerMeter.NUM_MISSING_SEGMENTS, numMissingSegments);
396397
}
397398
}

‎pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,9 @@ public QueryResult submitAndReduce(RequestContext context, DispatchableSubPlan d
140140
Thread.currentThread().interrupt();
141141
cancel(requestId, plans);
142142
throw new QException(QException.INTERNAL_ERROR_CODE, e);
143+
} catch (Exception e) {
144+
cancel(requestId, plans);
145+
throw new QException(QException.INTERNAL_ERROR_CODE, e);
143146
}
144147
}
145148

‎pinot-query-runtime/src/main/java/org/apache/pinot/query/service/server/QueryServer.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,9 @@ public void cancel(Worker.CancelRequest request, StreamObserver<Worker.CancelRes
233233
LOGGER.error("Caught exception while cancelling opChain for request: {}", request.getRequestId(), t);
234234
}
235235
// we always return completed even if cancel attempt fails, server will self clean up in this case.
236+
// TODO: This produces fails inside GRPC when the other end did already closed the connection.
237+
// GRPC logs these errors using JUL, which pollutes the logs. Find a way to either suppress these logs or
238+
// handle the cancellation in a way that does not produce these errors.
236239
responseObserver.onCompleted();
237240
}
238241

‎pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryRunnerTest.java‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -203,10 +203,13 @@ public void testSqlWithExceptionMsgChecker(String sql, String expectedError) {
203203
// java.lang.IllegalArgumentException: Illegal Json Path: $['path'] does not match document
204204
// In some cases there is no prefix.
205205
String exceptionMessage = e.getMessage();
206-
Assert.assertTrue(
207-
exceptionMessage.startsWith("Received error query execution result block: ") || exceptionMessage.startsWith(
208-
"Error occurred during stage submission") || exceptionMessage.equals(expectedError),
209-
"Exception message didn't start with proper heading: " + exceptionMessage);
206+
boolean matches = exceptionMessage.startsWith("Received error query execution result block: ")
207+
|| exceptionMessage.startsWith("Error occurred during stage submission")
208+
|| exceptionMessage.equals(expectedError)
209+
|| exceptionMessage.contains(expectedError);
210+
if (!matches) {
211+
Assert.fail("Exception message didn't match. Expected: " + expectedError + ". Actual: " + exceptionMessage);
212+
}
210213
Assert.assertTrue(exceptionMessage.contains(expectedError),
211214
"Exception should contain: " + expectedError + ", but found: " + exceptionMessage);
212215
}
@@ -313,7 +316,8 @@ protected Iterator<Object[]> provideTestSqlWithExecutionException() {
313316
});
314317

315318
// Function with incorrect argument signature should throw runtime exception when casting string to numeric
316-
testCases.add(new Object[]{"SELECT least(a.col2, b.col3) FROM a JOIN b ON a.col1 = b.col1", "For input string:"});
319+
testCases.add(new Object[]{"SELECT least(a.col2, b.col3) FROM a JOIN b ON a.col1 = b.col1",
320+
"Operator TRANSFORM on stage 1 failed: Failed to convert value string value to double"});
317321

318322
// Scalar function that doesn't have a valid use should throw an exception on the leaf stage
319323
// - predicate only functions:

‎pinot-query-runtime/src/test/java/org/apache/pinot/query/service/dispatch/QueryDispatcherTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.apache.pinot.query.runtime.QueryRunner;
3737
import org.apache.pinot.query.service.server.QueryServer;
3838
import org.apache.pinot.query.testutils.QueryTestUtils;
39+
import org.apache.pinot.spi.exception.QException;
3940
import org.apache.pinot.spi.trace.DefaultRequestContext;
4041
import org.apache.pinot.spi.trace.RequestContext;
4142
import org.mockito.Mockito;
@@ -144,7 +145,7 @@ public void testQueryDispatcherCancelWhenQueryReducerThrowsError()
144145
// will throw b/c mailboxService is mocked
145146
_queryDispatcher.submitAndReduce(context, dispatchableSubPlan, 10_000L, Collections.emptyMap());
146147
Assert.fail("Method call above should have failed");
147-
} catch (NullPointerException e) {
148+
} catch (QException e) {
148149
// Expected
149150
}
150151
// wait just a little, until the cancel is being called.

‎pinot-query-runtime/src/test/resources/queries/Aggregates.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -644,13 +644,13 @@
644644
"description": "nested aggregation",
645645
"sql": "SELECT min(max(int_col)) FROM {tbl}",
646646
"comments": ".*Aggregate expressions cannot be nested.",
647-
"expectedException": "Error composing query plan for.*"
647+
"expectedException": ".*Aggregate expressions cannot be nested."
648648
},
649649
{
650650
"psql": "4.2.7",
651651
"description": "nested aggregation",
652652
"sql": "SELECT (SELECT max(min(int_col)) FROM {tbl}) from {tbl};",
653-
"expectedException": "Error composing query plan for.*"
653+
"expectedException": ".*Aggregate expressions cannot be nested."
654654
},
655655
{
656656
"psql": "4.2.7",

‎pinot-query-runtime/src/test/resources/queries/SelectHaving.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,12 @@
4444
{
4545
"comment": "Plan failed. Expression 'a' is not being grouped.",
4646
"sql":"SELECT a FROM {test_having} HAVING min(a) < max(a);",
47-
"expectedException": "Error composing query plan for.*"
47+
"expectedException": "Error composing query plan: At line 1, column 8: Expression 'a' is not being grouped"
4848
},
4949
{
5050
"comment": "Plan failed. Expression 'a' is not being grouped.",
5151
"sql":"SELECT 1 AS one FROM {test_having} HAVING a > 1;",
52-
"expectedException": "Error composing query plan for.*"
52+
"expectedException": "Error composing query plan: At line 1, column 71: Expression 'a' is not being grouped"
5353
},
5454
{
5555
"sql":"SELECT 1 AS one FROM {test_having} HAVING 1 > 2;"

0 commit comments

Comments
 (0)