ankitsultana commented on code in PR #17440:
URL: https://github.com/apache/pinot/pull/17440#discussion_r2652188407
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/timeseries/LeafTimeSeriesOperator.java:
##########
@@ -54,13 +57,12 @@ public LeafTimeSeriesOperator(TimeSeriesExecutionContext
context, ServerQueryReq
public TimeSeriesBlock getNextBlock() {
Preconditions.checkNotNull(_queryExecutor, "Leaf time series operator has
not been initialized");
InstanceResponseBlock instanceResponseBlock =
_queryExecutor.execute(_request, _executorService);
- assert instanceResponseBlock.getResultsBlock() instanceof
GroupByResultsBlock;
- _queryLogger.logQuery(_request, instanceResponseBlock, "TimeSeries");
if (MapUtils.isNotEmpty(instanceResponseBlock.getExceptions())) {
- // TODO: Return error in the TimeSeriesBlock instead?
- String message =
instanceResponseBlock.getExceptions().values().iterator().next();
- throw new RuntimeException("Error running time-series query: " +
message);
+ return buildErrorBlock(instanceResponseBlock, "Error running time-series
query");
}
+ assert instanceResponseBlock.getResultsBlock() instanceof
GroupByResultsBlock;
+ _queryLogger.logQuery(_request, instanceResponseBlock, "TimeSeries");
Review Comment:
why move the logQuery line and the assert?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/timeseries/serde/TimeSeriesBlockSerde.java:
##########
@@ -104,7 +110,21 @@ public static TimeSeriesBlock
deserializeTimeSeriesBlock(ByteBuffer readOnlyByte
long seriesId = Long.parseLong(timeSeries.getId());
seriesMap.computeIfAbsent(seriesId, x -> new
ArrayList<>()).add(timeSeries);
}
- return new TimeSeriesBlock(timeBuckets, seriesMap, metadataMap);
+ TimeSeriesBlock block = new TimeSeriesBlock(timeBuckets, seriesMap,
metadataMap);
+ if (metadataMap.containsKey(EXCEPTIONS_METADATA_KEY)) {
+ String exceptionsJson = metadataMap.get(EXCEPTIONS_METADATA_KEY);
+ try {
+ List<Map<String, Object>> exceptionsList =
JsonUtils.stringToObject(exceptionsJson, List.class);
+ for (Map<String, Object> exceptionData : exceptionsList) {
+ int errorCode = ((Number) exceptionData.get(ERROR_CODE)).intValue();
+ String message = (String) exceptionData.get(MESSAGE);
+ block.addToExceptions(new
QueryException(QueryErrorCode.fromErrorCode(errorCode), message));
+ }
+ } catch (Exception e) {
+ throw new IOException("Failed to deserialize exceptions from
metadata", e);
Review Comment:
Hmm let's change the signature to throw the checked "Exception"? We
shouldn't ideally be wrapping into IOException ourselves.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/timeseries/serde/TimeSeriesBlockSerde.java:
##########
@@ -118,8 +138,26 @@ public static ByteString
serializeTimeSeriesBlock(TimeSeriesBlock timeSeriesBloc
container.add(timeSeriesToRow(timeSeries, dataSchema));
}
}
- RowHeapDataBlock transferableBlock = new RowHeapDataBlock(container,
dataSchema);
- return
DataBlockUtils.toByteString(transferableBlock.asSerialized().getDataBlock());
+ RowHeapDataBlock rowHeapBlock = new RowHeapDataBlock(container,
dataSchema);
+ return
DataBlockUtils.toByteString(rowHeapBlock.asSerialized().getDataBlock());
+ }
+
+ public static void encodeExceptionsToMetadata(TimeSeriesBlock
timeSeriesBlock, Map<String, String> metadataMap) {
+ List<QueryException> exceptions = timeSeriesBlock.getExceptions();
+ if (exceptions != null && !exceptions.isEmpty()) {
+ try {
+ List<Map<String, Object>> exceptionsList = new ArrayList<>();
+ for (QueryException exception : exceptions) {
+ Map<String, Object> exceptionData = new HashMap<>();
+ exceptionData.put(ERROR_CODE, exception.getErrorCode().getId());
+ exceptionData.put(MESSAGE, exception.getMessage());
+ exceptionsList.add(exceptionData);
+ }
+ metadataMap.put(EXCEPTIONS_METADATA_KEY,
JsonUtils.objectToString(exceptionsList));
+ } catch (Exception e) {
+ throw new RuntimeException("Failed to encode exceptions to metadata",
e);
Review Comment:
Let's propagate the checked exception to be explicit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
