github-actions[bot] commented on code in PR #66778:
URL: https://github.com/apache/doris/pull/66778#discussion_r3783138606
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -448,6 +443,7 @@ public long streamingSplitEstimate(ConnectorSession
session, ConnectorTableHandl
Optional<ConnectorExpression> filter, boolean countPushdown) {
IcebergTableHandle iceHandle = (IcebergTableHandle) handle;
if (iceHandle.isResolvedEmptySnapshot() || iceHandle.isSystemTable()
+ || (countPushdown && filter.isEmpty())
Review Comment:
[P1] Preserve streaming when delete files force count fallback
This returns `-1` for every unfiltered `COUNT(*)` before checking whether
metadata count collapse is actually possible. If the snapshot has any live
equality/position delete manifest, `planCountPushdown` later declines and the
synchronous path materializes every `FileScanTask` and range; on a million-file
table that defeats the lazy, backpressured path that exists to prevent FE OOM.
The base logic continued to the file-count threshold when the summary count was
unservable, and `test_iceberg_optimize_count.groovy` already expects the
dangling-delete batch case to remain `approximate`. Please allow streaming once
delete state proves collapse impossible, and add a live-delete
`countPushdown=true` estimate test.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -1241,45 +1235,138 @@ private static Schema pinnedSchema(Table table,
IcebergTableHandle handle) {
}
/**
- * Emit the single collapsed COUNT(*)-pushdown range: the first whole-file
{@link FileScanTask} from
- * {@code scan.planFiles()} carrying the full {@code realCount} via {@code
table_level_row_count} → BE's
- * count reader serves it without opening the data file. Mirrors paimon's
{@code buildCountRange} (one
- * range bearing the summed total). Result-identical to legacy's count
short-circuit even though legacy
- * takes a different shape: legacy byte-splits the count file ({@code
planFileScanTask} →
- * {@code splitFiles} → {@code TableScanUtil.splitFiles}), keeps the first
split task's byte-range for
- * {@code count < 10000}, and {@code assignCountToSplits} distributes the
same total — but under count
- * pushdown BE's count reader never reads the file (the range's
start/length are irrelevant) and sums
- * {@code table_level_row_count} across ranges, so one whole-file range
yields the identical total (and
- * legacy's {@code >10000} parallel multi-split trim is the perf-only
divergence we drop). An empty table
- * (no files) yields no range, so BE gets 0 ranges and COUNT returns 0
(legacy returns empty splits too).
+ * Build a collapsed COUNT(*) range from current manifest-list aggregates.
Summing each data manifest's
+ * added and existing row counts is O(manifests), while only the first
live {@link FileScanTask} is needed as
+ * the representative range. Old manifest lists that omit these aggregates
use the bounded O(files) fallback.
+ * Any live delete file makes the optimization unsafe and tells the caller
to perform a normal scan.
*/
- private List<ConnectorScanRange> planCountPushdown(Table table, TableScan
scan, long realCount,
+ private Optional<List<ConnectorScanRange>> planCountPushdown(Table table,
TableScan scan,
+ int formatVersion, boolean partitioned, List<String>
orderedPartitionKeys, ZoneId zone,
+ UnaryOperator<String> uriNormalizer, ConnectorSession session,
Optional<ConnectorExpression> filter) {
+ Snapshot snapshot = scan.snapshot();
+ if (snapshot == null) {
+ return Optional.of(Collections.emptyList());
+ }
+
+ ManifestDeleteState deleteState =
manifestDeleteState(snapshot.deleteManifests(table.io()));
+ if (deleteState == ManifestDeleteState.PRESENT) {
+ return Optional.empty();
+ }
+ if (deleteState == ManifestDeleteState.NONE) {
+ OptionalLong manifestCount =
liveRowCountFromManifests(snapshot.dataManifests(table.io()));
+ if (manifestCount.isPresent()) {
+ return planManifestCountRange(table, scan,
manifestCount.getAsLong(), formatVersion,
+ partitioned, orderedPartitionKeys, zone,
uriNormalizer, session, filter);
+ }
+ }
+
+ // Older manifest lists may omit aggregate counters. Preserve
correctness by falling back to the
+ // bounded per-file enumeration instead of trusting snapshot summary
metadata.
+ return planCountPushdownFromFileTasks(table, scan, formatVersion,
partitioned,
Review Comment:
[P1] Retry lazy cache failures in the new per-file fallback
This new old-manifest fallback enumerates every task, but it consumes the
cache-backed iterable after `countPushdownFileScanTasks` has left its `catch
(Exception)` boundary. Phase 2 loads data manifests lazily here, and
`IcebergManifestCache` wraps a read failure in `RuntimeException`, so a failure
on a later manifest aborts `COUNT(*)` without `recordFailure` or the normal
`scan.planFiles()` retry. Previously a usable summary count stopped after the
first representative, while missing summary counters fell through to
`planFileScanTask`, whose catch encloses full enumeration. Please restart this
fallback with a fresh accumulator/representative after a cache exception and
add a late Phase-2 failure test.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -1241,45 +1235,138 @@ private static Schema pinnedSchema(Table table,
IcebergTableHandle handle) {
}
/**
- * Emit the single collapsed COUNT(*)-pushdown range: the first whole-file
{@link FileScanTask} from
- * {@code scan.planFiles()} carrying the full {@code realCount} via {@code
table_level_row_count} → BE's
- * count reader serves it without opening the data file. Mirrors paimon's
{@code buildCountRange} (one
- * range bearing the summed total). Result-identical to legacy's count
short-circuit even though legacy
- * takes a different shape: legacy byte-splits the count file ({@code
planFileScanTask} →
- * {@code splitFiles} → {@code TableScanUtil.splitFiles}), keeps the first
split task's byte-range for
- * {@code count < 10000}, and {@code assignCountToSplits} distributes the
same total — but under count
- * pushdown BE's count reader never reads the file (the range's
start/length are irrelevant) and sums
- * {@code table_level_row_count} across ranges, so one whole-file range
yields the identical total (and
- * legacy's {@code >10000} parallel multi-split trim is the perf-only
divergence we drop). An empty table
- * (no files) yields no range, so BE gets 0 ranges and COUNT returns 0
(legacy returns empty splits too).
+ * Build a collapsed COUNT(*) range from current manifest-list aggregates.
Summing each data manifest's
+ * added and existing row counts is O(manifests), while only the first
live {@link FileScanTask} is needed as
+ * the representative range. Old manifest lists that omit these aggregates
use the bounded O(files) fallback.
+ * Any live delete file makes the optimization unsafe and tells the caller
to perform a normal scan.
*/
- private List<ConnectorScanRange> planCountPushdown(Table table, TableScan
scan, long realCount,
+ private Optional<List<ConnectorScanRange>> planCountPushdown(Table table,
TableScan scan,
+ int formatVersion, boolean partitioned, List<String>
orderedPartitionKeys, ZoneId zone,
+ UnaryOperator<String> uriNormalizer, ConnectorSession session,
Optional<ConnectorExpression> filter) {
+ Snapshot snapshot = scan.snapshot();
+ if (snapshot == null) {
+ return Optional.of(Collections.emptyList());
+ }
+
+ ManifestDeleteState deleteState =
manifestDeleteState(snapshot.deleteManifests(table.io()));
+ if (deleteState == ManifestDeleteState.PRESENT) {
Review Comment:
[P1] Retire the dangling-delete flag and its P0 contract consistently
This unconditional fallback, together with removal of
`ignoreIcebergDanglingDelete`, makes the public
`ignore_iceberg_dangling_delete` session variable inert on the plugin-driven
Iceberg path. `SessionVariable` still promises metadata COUNT behavior, and the
unchanged `test_iceberg_optimize_count.groovy` sets the flag to `true` and
requires `pushdown agg=COUNT (1)`, so that P0 case now fails deterministically
(the changed unit test already demonstrates both flag values behave the same).
If exactness intentionally retires the unsafe optimization, please
remove/deprecate the variable and update the P0 expectation, stale BE comment,
and release note in this PR; otherwise preserve a tested flag-aware path.
--
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]
