github-actions[bot] commented on code in PR #65659:
URL: https://github.com/apache/doris/pull/65659#discussion_r3588166244
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalTable.java:
##########
@@ -444,7 +444,10 @@ public SelectedPartitions
initSelectedPartitions(Optional<MvccSnapshot> snapshot
return SelectedPartitions.NOT_PRUNED;
}
Map<String, PartitionItem> nameToPartitionItems =
getNameToPartitionItems(snapshot);
- return new SelectedPartitions(nameToPartitionItems.size(),
nameToPartitionItems, false);
+ Optional<SortedPartitionRanges<String>> sortedPartitionRanges =
Optional.ofNullable(
Review Comment:
This moves the expensive sorted-range build ahead of the
`enable_binary_search_filtering_partitions` check. `PruneFileScanPartition`
only decides whether binary search is enabled later, and
`PreloadExternalMetadata` also calls `initSelectedPartitions(...)` as a warmup
while discarding the returned `SelectedPartitions`, so large external partition
maps now pay the range-conversion/sort cost even when binary search is disabled
or no pruning result will use it. The OLAP path explicitly returns empty before
loading sorted ranges when this session variable is off. Please defer this
build until pruning knows binary search is enabled, or gate the stored snapshot
construction the same way, including the Hudi-specific initializer.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PruneFileScanPartition.java:
##########
@@ -95,15 +95,18 @@ private SelectedPartitions
pruneExternalPartitions(ExternalTable externalTable,
boolean enableBinarySearch = ctx.getConnectContext() == null
||
ctx.getConnectContext().getSessionVariable().enableBinarySearchFilteringPartitions;
if (enableBinarySearch) {
- sortedPartitionRanges = (Optional)
externalTable.getSortedPartitionRanges(scan);
+ sortedPartitionRanges =
scan.getSelectedPartitions().sortedPartitionRanges;
}
PartitionPruneResult<String> result = PartitionPruner.pruneWithResult(
partitionSlots, filter.getPredicate(), nameToPartitionItem,
ctx,
PartitionTableType.EXTERNAL, sortedPartitionRanges);
List<String> prunedPartitions = new ArrayList<>(result.partitions);
for (String name : prunedPartitions) {
- selectedPartitionItems.put(name, nameToPartitionItem.get(name));
+ PartitionItem item = nameToPartitionItem.get(name);
Review Comment:
At this point a missing item is an invariant failure, not a partition to
skip. The sequential path returns keys from `nameToPartitionItem`, and the
binary-search path should return keys from
`scan.getSelectedPartitions().sortedPartitionRanges`, which this PR now builds
from the same selected-partition snapshot. If `get(name)` is ever null again,
silently omitting it produces a partial or even empty selected partition map
for a positive predicate; the new test's p4 scenario demonstrates exactly that
behavior. Please fail/assert here (or fall back to sequential pruning before
constructing `SelectedPartitions`) instead of returning a scan over fewer
partitions.
--
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]
