github-actions[bot] commented on code in PR #66778:
URL: https://github.com/apache/doris/pull/66778#discussion_r3783595760
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -1241,55 +1248,209 @@ 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.
+ * Equality deletes and non-ignored position deletes make the optimization
unsafe and tell 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());
+ }
+
+ boolean ignorePositionDeletes = sessionBool(session,
IGNORE_ICEBERG_DANGLING_DELETE, false);
+ ManifestDeleteState deleteState =
manifestDeleteState(snapshot.deleteManifests(table.io()));
+ if (deleteState == ManifestDeleteState.PRESENT
+ && (!ignorePositionDeletes ||
hasNonIgnorableDeleteFiles(table, snapshot, true))) {
+ return Optional.empty();
+ }
+ if (deleteState != ManifestDeleteState.UNKNOWN) {
+ OptionalLong manifestCount =
liveRowCountFromManifests(snapshot.dataManifests(table.io()));
+ if (manifestCount.isPresent()) {
+ return planManifestCountRange(table, scan,
manifestCount.getAsLong(), formatVersion,
+ partitioned, orderedPartitionKeys, zone,
uriNormalizer, session, filter,
+ ignorePositionDeletes);
+ }
+ }
+
+ // 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,
+ orderedPartitionKeys, zone, uriNormalizer, session, filter,
ignorePositionDeletes);
+ }
+
+ private Optional<List<ConnectorScanRange>> planManifestCountRange(Table
table, TableScan scan, long exactCount,
+ int formatVersion, boolean partitioned, List<String>
orderedPartitionKeys, ZoneId zone,
+ UnaryOperator<String> uriNormalizer, ConnectorSession session,
Optional<ConnectorExpression> filter,
+ boolean ignorePositionDeletes) {
try (CloseableIterable<FileScanTask> tasks =
countPushdownFileScanTasks(scan, session, table, filter)) {
Review Comment:
[P1] Retry lazy cache failures on the newly reachable aggregate path
When manifest-list row aggregates are usable but the old snapshot-summary
counters are missing or unusable, this PR newly routes `COUNT(*)` here;
previously that case fell through to `planFileScanTask`, whose broad cache
fallback covered full iteration. The cache-backed iterable is returned before
its lazy Phase-2 read: `ManifestCacheFileScanTaskIterator.advance()` loads the
first data manifest later and wraps read failures in `RuntimeException`, while
this method catches only `IOException`. A transient optional-cache failure can
therefore abort the query without `recordFailure` or a fresh `scan.planFiles()`
retry. This is distinct from the earlier old-metadata thread because its fix
only surrounds `planCountPushdownFromFileTasks`. Please put representative
iteration and close under the same catch-and-fresh-SDK boundary, and add a test
with usable manifest aggregates, unusable summary counters, and a
fail-on-first-manifest cache read.
--
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]
