[fix](iceberg) Derive count pushdown from live manifests - #66778
Open
Gabriel39 wants to merge 4 commits into
Open
[fix](iceberg) Derive count pushdown from live manifests#66778Gabriel39 wants to merge 4 commits into
Gabriel39 wants to merge 4 commits into
Conversation
Iceberg snapshot summary fields are optional writer metadata and may contain a valid but incorrect positive row count. Derive exact unfiltered COUNT(*) results from live data-file record counts, and fall back to a normal scan for filters, deletes, invalid counts, or overflow. Tests: IcebergScanPlanProviderTest (148 tests)
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 17607 ms |
Use current manifest-list live-row aggregates for the common no-delete path and read only one representative file task. Fall back to bounded per-file counting when old manifests omit aggregates, and to a normal scan when live delete manifests are present. Tests: IcebergScanPlanProviderTest (148 tests)
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 17780 ms |
Contributor
TPC-DS: Total hot run time: 85964 ms |
Contributor
ClickBench: Total hot run time: 14.7 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review status: complete/converged after three rounds. Every final-round reviewer returned NO_NEW_VALUABLE_FINDINGS, and every ledger candidate is resolved.
The central change is sound for its intended exactness boundary: an unfiltered, pinned Iceberg snapshot with no live delete files can derive its row total from live manifest-list counters, while missing, invalid, or overflowing metadata falls back conservatively. Three blocking regressions remain, each called out inline:
- Delete-bearing counts lose the existing lazy/backpressured planning path and can synchronously materialize a very large scan.
ignore_iceberg_dangling_deletebecomes inert without retiring its public contract or updating the unchanged P0 behavior check.- The new old-manifest per-file fallback lets a lazy manifest-cache failure escape instead of recording it and retrying through the SDK.
Checkpoint conclusions: the source scope is localized and adds no new shared state, lock, lifecycle, persistence, transaction, write, SPI, thrift, or storage-format surface. Current/snapshot/ref pins, filters and partition pruning, COUNT argument semantics, format v1-v3 metadata, delete types, empty/zero/overflow cases, cache/SDK and sync/stream paths, representative-range handling, and both BE count carriers were reviewed. Compatibility cleanup is incomplete only as described inline; error handling is conservative except for the lazy-cache retry gap; no additional observability issue was found.
Tests were not run because the authoritative review bundle forbids builds and test execution. Static review covered all four changed files, the replacement/deleted unit tests, and the relevant unchanged external P0 assertions.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 17545 ms |
Contributor
TPC-DS: Total hot run time: 85574 ms |
Contributor
There was a problem hiding this comment.
Request changes: one new P1 is attached, and the existing dangling-delete P1 remains unresolved.
Blocking findings:
- Existing discussion #66778 (comment) already owns the
ignore_iceberg_dangling_delete/P0 contract, so I am not duplicating it inline. The reply makes the flag active, but now pushes raw data-manifest rows for every position delete. The removed behavior and unchanged P0 require data rows minus position-delete rows; the changed 10-row/3-delete unit expectation of 10 therefore still violates the contract and the P0's expected pushed count of 1 remains broken. - The attached inline is a distinct cache-availability regression: snapshots with usable manifest aggregates but unusable old summary counters newly enter the representative fast path, where a lazy cache read can escape without failure accounting or a fresh SDK retry.
Critical checkpoint conclusions:
- Goal and data correctness: deriving exact unfiltered COUNT from the pinned snapshot's live manifest/file metadata avoids the stale-summary bug for valid metadata, but the two P1 paths above mean the PR does not yet fully accomplish its goal.
- Scope and clarity: the production change is focused on Iceberg COUNT planning and related comments/tests; no unrelated source changes were found.
- Concurrency: this planning path and its count/cache-stat decisions run on the FE planning thread. The PR adds no thread, lock, lock ordering, or new shared mutable state.
- Lifecycle and error handling: try-with-resources owns the scan iterables. The old-metadata consumer now discards partial state and retries correctly, but the aggregate representative consumer lacks the equivalent lazy-failure/close boundary described inline.
- Configuration: no new configuration item is added. The existing session variable is dynamically read as before, but its flag-true numeric semantics are not preserved, as covered by the existing thread.
- Compatibility: no function symbol, storage format, persistence format, or new FE/BE protocol field is introduced. The established
-1versus nonnegativetable_level_row_countcarrier remains compatible; the user-visible session-variable behavior is the compatibility regression. - Parallel paths and conditions: current and old manifest metadata, cache and SDK enumeration, filtered counts, equality/position deletes, v2/v3, snapshot pins, empty/zero-row cases, and synchronous/streaming planning were checked. The conditions otherwise fail closed or retain the normal delete-applying path; no third issue was substantiated.
- Test coverage and results: units cover wrong summaries, pins, filters, deletes, empty tables, representative selection, and late old-metadata cache failure. The flag-true expected result is incorrect, and there is no aggregate-present/unusable-summary fail-on-first-manifest cache test. No build or test command was run because the review harness explicitly prohibits builds.
- Observability: the repaired old-metadata fallback warns and records cache failure before retrying; the newly reachable aggregate path omits both on a lazy failure.
- Transactions, persistence, and data writes: not applicable; this PR changes read planning only.
- FE/BE handoff: no new variable is passed. Both Iceberg native reader paths retain the same nonnegative count shortcut and normal ranges retain
-1. - Performance and remaining issues: current metadata stays O(manifests) plus one representative, old metadata is bounded-memory O(files), and streaming remains the large-scan memory fence. No additional correctness, lifecycle, compatibility, or performance issue remained after the final sweep.
- User focus and completion: no additional user-provided focus was supplied, so the full PR was reviewed. The review converged after Round 2 with all candidates accepted, duplicate-suppressed, or dismissed; it is complete rather than capped.
Contributor
ClickBench: Total hot run time: 14.53 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 17601 ms |
Contributor
TPC-DS: Total hot run time: 85111 ms |
Contributor
ClickBench: Total hot run time: 14.56 s |
Contributor
There was a problem hiding this comment.
Review status: capped/incomplete (3 of 3 rounds; a valuable candidate surfaced in the final round).
Summary opinion
The PR fixes the core exactness problem in the right direction: exact unfiltered COUNT(*) results now come from live manifest state instead of optional snapshot-summary values, and unprovable states fall back to row scanning. I found no new result-correctness blocker distinct from the existing review threads. I am requesting changes for two avoidable full delete-manifest rescans on the successful position-delete compatibility path; both are reported inline.
Critical checkpoint conclusions
- Goal and proof: The implementation accomplishes the stated goal for the reviewed paths. Manifest-list ADDED/EXISTING row totals or the live-file fallback provide the data-row proof; equality deletes, invalid counts, overflow, predicates, and unsafe non-netted position deletes decline metadata COUNT. Tests cover an intentionally wrong positive snapshot summary and the principal proof/fallback states.
- Scope and clarity: The functional change is concentrated in
IcebergScanPlanProvider; the BE, metadata-statistics, and session-variable edits are explanatory. The exact-count and optimizer-estimate paths remain explicitly separated. - Concurrency: Count planning is synchronous and introduces no new shared mutable state or locks. The asynchronous streaming source retains its existing pinned-handle and backpressure ownership. The pre-pin batch estimate can change only the eager/streaming choice, not the snapshot used for rows.
- Lifecycle: Both cache-backed count consumers place iterable creation, lazy iteration, close, failure recording, and fresh SDK retry inside the retry boundary. The per-file retry owns a fresh accumulator. No new leak, partial-reuse, or close-order issue survived review.
- Configuration: No configuration key or default changes.
ignore_iceberg_dangling_deleteremains dynamically propagated through connector session properties, defaults to false, and its description now matches the deliberately approximate position-delete compatibility behavior. - Compatibility: No storage format, symbol, or thrift shape is added. The existing
table_level_row_countcarrier retains-1as the decline sentinel and>= 0(including zero) as a pushed count; V1 and V2 consumers preserve their established rolling-upgrade fences. - Parallel paths: Direct COUNT(*)/COUNT(1), filtered scans, equality/position deletes, format v3, cache-enabled/disabled planning, empty snapshots, current/ref/time-travel pins, optimizer statistics, and BE V1/V2 readers were traced. The two redundant-delete-read instances are the remaining mismatches and are called out inline.
- Conditional checks: The filter, delete-state, overflow, negative-count, and empty-table gates all have an exact-or-decline interpretation. The negative-net streaming case is real but is the same gate and consequence already covered by discussion r3783138606, so it was not duplicated.
- Test coverage: The FE unit suite covers wrong summaries, absent manifest aggregates, filters, empty snapshots, equality and position deletes, and eager/late manifest-cache failures. The missing format-v3 flagged-estimator and single-open synchronous representative cases are specified in the inline findings.
- Test results: I did not build or run tests because the review-runner contract prohibits builds. At submission time CheckStyle, Clang Formatter, license, dependency, secrets, and performance checks are green; FE UT, BE UT, macOS BE UT, and compile checks are still pending.
- Observability: Existing cache failure statistics and warning logs cover failure diagnosis. The accepted issues are successful-path redundant I/O rather than silent failures; no additional metric is necessary for the fix.
- Transactions and persistence: Not applicable. The PR changes read planning only; it adds no EditLog, replay, persisted state, or transaction transition.
- Data writes: Not applicable. No data mutation, commit, atomicity, or crash-recovery behavior changes.
- FE/BE variables: No new cross-boundary variable is introduced. The existing count field and COUNT argument/runtime-filter fences are populated and consumed on the reviewed paths.
- Performance: Manifest aggregates keep the common proof O(manifests), and the old-metadata fallback is O(files) with constant accumulator memory. However, successful flagged counts reread delete manifests during representative planning, and v3 batch estimation adds another avoidable read; these are the two P2 findings.
- Other issues: All six changed paths and their directly affected production/test paths were reviewed. Four existing inline threads and replies were treated as known context; no duplicate was resubmitted.
User focus
No additional focus was supplied, so the full PR was reviewed without further narrowing.
Completion
Two accepted inline findings are included. Because the second was identified in the third and final allowed round, this review is capped/incomplete rather than converged; a refreshed pass after fixes may still be warranted.
| return -1; | ||
| } | ||
| if (deleteState != ManifestDeleteState.PRESENT || netPositionDeletes) { | ||
| OptionalLong positionDeleteRows = livePositionDeleteRowCount(table, snapshot); |
Contributor
There was a problem hiding this comment.
[P2] Keep the v3 fence ahead of the delete-manifest proof
On a format-v3 snapshot with live position deletes and ignore_iceberg_dangling_delete=true, this call opens and walks every live delete-file entry, then the COUNT block selects synchronous planning. But the unconditional v3 fence below makes that batch result invariant, and synchronous planCountPushdown calls livePositionDeleteRowCount again. The base ordering fenced v3 before this proof. For large position-delete/deletion-vector manifests, the estimator therefore adds a full remote metadata scan that cannot affect its result. Please move the v3 fence ahead of the COUNT block and cover the flagged v3/live-delete estimator path.
| if (!visibleRows.isPresent()) { | ||
| return Optional.empty(); | ||
| } | ||
| return planManifestCountRange(table, scan, visibleRows.getAsLong(), formatVersion, |
Contributor
There was a problem hiding this comment.
[P2] Avoid rebuilding the delete index just to choose a COUNT representative
The global proof above has already opened every live delete manifest and either rejected equality deletes or obtained the position-delete count. This branch then chooses its first FileScanTask through scan.planFiles() or the cache-backed path; both eagerly reread all delete manifests to build a delete index before yielding that task. The range carries the exact table-level count, so BE short-circuits without applying the representative task's delete bindings. Successful flagged counts therefore pay a second full delete-manifest scan even when batch mode is disabled. Please obtain the representative data file without constructing unused delete bindings (or reuse the proof's entries), and test that synchronous flagged count planning opens each delete manifest only once.
Contributor
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review status: complete/converged after Round 2. All three final-round reviewers returned NO_NEW_VALUABLE_FINDINGS, and every ledger candidate has a main-verified final status.
Summary opinion
The core exactness change is sound on the reviewed paths: unfiltered COUNT(*)/COUNT(1) reads a pinned Iceberg snapshot's live manifest state rather than optional snapshot-summary values, and unknown, invalid, overflowing, filtered, or unsafe delete states decline the metadata result and retain a real scan. I found no new actionable issue distinct from the existing review context, so this review adds no inline comments.
Two current-head P2 performance concerns remain open in the estimator ordering thread and the representative/delete-index thread. I did not duplicate them. A flagged negative-net estimator fallback is also substantially the same estimator/planner disagreement already covered by discussion r3783138606, so it was duplicate-suppressed under the review contract.
Critical checkpoint conclusions
- Goal and proof: The implementation accomplishes the stated exact-count goal for the reviewed row domains. Manifest ADDED/EXISTING totals or the live-file fallback prove the data-row total at the selected snapshot; equality deletes, unsafe position deletes, malformed values, overflow, and negative subtraction fall back. Unit coverage includes a deliberately wrong snapshot summary, selected snapshots, deletes, filters, empty/zero results, old manifests, FE-to-BE population, and cache failures.
- Scope and clarity: The functional work is focused in
IcebergScanPlanProvider; the BE, connector-metadata, and session-variable changes align comments/contracts, and the obsolete summary-count test is replaced by provider-level coverage. No unrelated source change was found. - Concurrency: Planning uses request-local state and introduces no new thread, lock, lock ordering, atomic, or shared mutable state. Existing asynchronous streaming ownership and backpressure remain unchanged.
- Lifecycle: Manifest/task iterables are closed through try-with-resources. Cached eager/lazy/close failures are caught at the consumer boundary, record one cache failure, and retry with fresh SDK iterables and fresh accumulator state. No new resource leak, partial-state reuse, or static-initialization concern was found.
- Configuration: No configuration key or default is added.
ignore_iceberg_dangling_deleteretains its name, boolean type, persisted session behavior, dynamic request-scoped lookup, and defaultfalse; only its description changes. - Compatibility: No function symbol, persisted/storage format, or Thrift shape changes. The established optional
table_level_row_countcarrier still uses-1to decline and nonnegative values, including zero, to push a result, preserving both BE scanner paths and mixed-version behavior. - Parallel paths: Direct COUNT semantics, static/runtime filter gates, TABLESAMPLE behavior, system tables, empty snapshots, current/ref/time-travel pins, cache/SDK planning, format v1-v3 deletes, rewrite scope, and both BE reader generations were traced. No additional parallel-path correctness issue was found; the remaining redundant-delete work is already covered by the two open P2 threads.
- Conditional checks: Each new gate has an exact-or-decline interpretation. Missing/negative/overflow metadata and unsafe delete inventories do not emit an unproven nonnegative count. Similar estimator/count gates were checked and duplicate-fenced where already reported.
- Test coverage: The changed FE units cover the central positive, negative, fallback, pinning, carrier, and cache-retry cases; the existing Iceberg COUNT regression and BE count-reader contracts cover the end-to-end carrier behavior. Additional edge tests could strengthen the matrix, but no uncovered edge justified a distinct correctness finding.
- Test results: I did not build or run tests because the authoritative review bundle prohibits builds and test execution. At submission time, FE UT, BE UT, compile, external/P0 regressions, style, license, dependency, secrets, and performance checks are green;
check_coverage_fe (Coverage)is failing, and thiscode-reviewjob is still pending. - Observability: Existing scan profiles, contextual metadata errors, cache-failure statistics, and warning paths are sufficient for the changed behavior. No new metric or log is required for correctness.
- Transactions and persistence: Not applicable; this is read planning and adds no EditLog, replay, transaction transition, or persisted state.
- Data writes: Not applicable; no data mutation, commit, atomicity, or crash-recovery behavior changes.
- FE/BE variables: No new cross-boundary variable is introduced. The existing count carrier and sentinel are populated and consumed consistently by the reviewed FE and BE paths.
- Performance: The no-delete exact proof is O(manifests), while old metadata falls back to an O(files), constant-accumulator scan. The two avoidable delete-manifest rescans remain the open P2 discussions linked above; no distinct third performance issue survived deduplication.
- Other issues: Error propagation, memory ownership, nullability, data-version visibility, and cloud/shared-nothing applicability were checked. Transactional write, persistence, and security review were not applicable to this read-planning change.
User focus and completion
No additional user-provided focus was supplied, so the full six-file change and its directly affected paths were reviewed. The review is complete/converged, not capped: Round 2 ended with no new valuable findings, no accepted new inline comments, and no unresolved ledger candidate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Problem Summary:
Iceberg V2 COUNT(*) pushdown treated the optional
total-recordssnapshot summary field as an exact query result. A writer can provide a syntactically valid positive value that does not match the live data files, causing Doris to return a wrong count without reading data.This change derives an exact unfiltered count by summing
addedRowsCount + existingRowsCountacross the current manifest list. The common path is O(manifests) and reads only the first live data-file task to build the representative range. Older manifest lists that omit aggregate counters fall back to bounded per-filerecordCountenumeration. Queries with row filters or live delete files use the normal scan, while invalid counters and overflow safely fall back instead of exposing unverified metadata.Release note
Fix incorrect Iceberg V2 COUNT(*) results when snapshot summary row counts are inaccurate.
Check List (For Author)
IcebergScanPlanProviderTest: 148 tests passed