xiangfu0 commented on code in PR #20262:
URL: https://github.com/apache/pinot/pull/20262#discussion_r4571413062
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -161,59 +159,43 @@ public static List<InstanceRequest>
constructServerQueryRequests(OpChainExecutio
int numRequests = tableSegmentsMap.size();
if (numRequests == 1) {
Map.Entry<String, List<String>> entry =
tableSegmentsMap.entrySet().iterator().next();
- String tableType = entry.getKey();
- List<String> segments = entry.getValue();
- if (tableType.equals(TableType.OFFLINE.name())) {
- String offlineTableName =
TableNameBuilder.forType(TableType.OFFLINE).tableNameWithType(rawTableName);
- TableDataManager tableDataManager =
instanceDataManager.getTableDataManager(offlineTableName);
- Preconditions.checkState(tableDataManager != null, "Failed to find
data manager for table: %s",
- offlineTableName);
- Pair<TableConfig, Schema> tableConfigAndSchema =
tableDataManager.getCachedTableConfigAndSchema();
- return List.of(compileInstanceRequest(executionContext, pinotQuery,
timeBoundary, TableType.OFFLINE,
- tableDataManager.getTableName(), tableConfigAndSchema.getLeft(),
tableConfigAndSchema.getRight(), segments,
- null));
- } else {
- assert tableType.equals(TableType.REALTIME.name());
- String realtimeTableName =
TableNameBuilder.forType(TableType.REALTIME).tableNameWithType(rawTableName);
- TableDataManager tableDataManager =
instanceDataManager.getTableDataManager(realtimeTableName);
- Preconditions.checkState(tableDataManager != null, "Failed to find
data manager for table: %s",
- realtimeTableName);
- Pair<TableConfig, Schema> tableConfigAndSchema =
tableDataManager.getCachedTableConfigAndSchema();
- return List.of(compileInstanceRequest(executionContext, pinotQuery,
timeBoundary, TableType.REALTIME,
- tableDataManager.getTableName(), tableConfigAndSchema.getLeft(),
tableConfigAndSchema.getRight(), segments,
- null));
- }
+ return List.of(compileInstanceRequest(executionContext, pinotQuery,
timeBoundary,
+ TableType.valueOf(entry.getKey()), rawTableName, entry.getValue(),
instanceDataManager));
} else {
assert numRequests == 2;
List<String> offlineSegments =
tableSegmentsMap.get(TableType.OFFLINE.name());
List<String> realtimeSegments =
tableSegmentsMap.get(TableType.REALTIME.name());
assert offlineSegments != null && realtimeSegments != null;
- String offlineTableName =
TableNameBuilder.forType(TableType.OFFLINE).tableNameWithType(rawTableName);
- TableDataManager offlineTableDataManager =
instanceDataManager.getTableDataManager(offlineTableName);
- Preconditions.checkState(offlineTableDataManager != null, "Failed to
find data manager for table: %s",
- offlineTableName);
- Pair<TableConfig, Schema> offlineTableConfigAndSchema =
offlineTableDataManager.getCachedTableConfigAndSchema();
- String realtimeTableName =
TableNameBuilder.forType(TableType.REALTIME).tableNameWithType(rawTableName);
- TableDataManager realtimeTableDataManager =
instanceDataManager.getTableDataManager(realtimeTableName);
- Preconditions.checkState(realtimeTableDataManager != null, "Failed to
find data manager for table: %s",
- realtimeTableName);
- Pair<TableConfig, Schema> realtimeTableConfigAndSchema =
- realtimeTableDataManager.getCachedTableConfigAndSchema();
// NOTE: Make a deep copy of PinotQuery for OFFLINE request.
return List.of(
compileInstanceRequest(executionContext, new PinotQuery(pinotQuery),
timeBoundary, TableType.OFFLINE,
- offlineTableDataManager.getTableName(),
offlineTableConfigAndSchema.getLeft(),
- offlineTableConfigAndSchema.getRight(), offlineSegments, null),
- compileInstanceRequest(executionContext, pinotQuery, timeBoundary,
TableType.REALTIME,
- realtimeTableDataManager.getTableName(),
realtimeTableConfigAndSchema.getLeft(),
- realtimeTableConfigAndSchema.getRight(), realtimeSegments,
null));
+ rawTableName, offlineSegments, instanceDataManager),
+ compileInstanceRequest(executionContext, pinotQuery, timeBoundary,
TableType.REALTIME, rawTableName,
+ realtimeSegments, instanceDataManager));
}
}
- /// Convert [PinotQuery] into an [InstanceRequest].
+ /// Converts [PinotQuery] into an [InstanceRequest] for one table type of a
physical table.
+ ///
+ /// A server sets up a table when it adds the first segment of it, so a
table that is not set up yet fails the query
+ /// only when the worker has a segment the server must have. Otherwise the
request is built without the schema of
+ /// the table, and the leaf query executor answers it with an empty result.
private static InstanceRequest
compileInstanceRequest(OpChainExecutionContext executionContext, PinotQuery
pinotQuery,
- @Nullable TimeBoundaryInfo timeBoundaryInfo, TableType tableType,
- String tableNameWithType, TableConfig tableConfig, Schema schema,
@Nullable List<String> segmentList,
+ @Nullable TimeBoundaryInfo timeBoundaryInfo, TableType tableType, String
rawTableName, List<String> segments,
+ InstanceDataManager instanceDataManager) {
+ String tableNameWithType =
TableNameBuilder.forType(tableType).tableNameWithType(rawTableName);
+ TableDataManager tableDataManager =
instanceDataManager.getTableDataManager(tableNameWithType);
+ Preconditions.checkState(tableDataManager != null || segments.isEmpty(),
Review Comment:
Nothing in the PR tests this MSE branch. A small
`ServerPlanRequestUtilsTest` with a mocked `OpChainExecutionContext` would
cover it: (a) missing table + empty list builds a request with no schema, (b)
missing table + segments still throws, (c) hybrid where OFFLINE is present and
REALTIME is missing and empty. I ran exactly those locally: (a) and (c) fail at
base and pass at head, and (b) passes on both.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/LogicalTableExecutionInfo.java:
##########
@@ -56,10 +64,11 @@ public static LogicalTableExecutionInfo
create(InstanceDataManager instanceDataM
SingleTableExecutionInfo singleTableExecutionInfo =
SingleTableExecutionInfo.create(instanceDataManager,
tableSegmentsContext.getTableName(),
tableSegmentsContext.getSegments(),
tableSegmentsContext.getOptionalSegments(), queryContext);
- tableExecutionInfos.add(singleTableExecutionInfo);
+ if (singleTableExecutionInfo != null) {
Review Comment:
The new skip/`null` branches have no coverage. Consider adding
`LogicalTableExecutionInfoTest#testCreateSkipsTableNotSetUp` covering: all
physical tables missing with only optional segments returns `null`; one missing
with a required segment throws `TableNotFoundException`; a mix keeps only the
tables that are present. I checked the first two locally and they behave as
intended.
--
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]
