Repository navigation
[fix](iceberg) Fail on missing snapshot files - #68142
Merged
Gabriel39 merged 2 commits intoSep 18, 2026
Merged
Conversation
### What problem does this PR solve? An Iceberg snapshot can reference a data file that has been removed from storage. With the default `ignore_not_found_file_in_external_table=true`, both file scanners can skip the missing file and return a successful but incomplete result. Exclude Iceberg ranges from the missing-file skip policy, regardless of that setting, while preserving the existing behavior for other table formats. Apply the shared policy at reader initialization and read boundaries. This also prevents a missing delete file reported as `NOT_FOUND` from being swallowed by the scanner. Add unit coverage for Iceberg Parquet/ORC/JNI and ordinary external ranges with both setting values. Add an external regression that creates multiple data files, deletes one actual object, and expects row scans to fail with V1/V2 and both setting values. The regression disables file/result caches and restores the BE configuration after testing. ### Release note Iceberg scans now fail when a required snapshot file is missing instead of silently skipping it, including when `ignore_not_found_file_in_external_table` is enabled. ### Check List (For Author) - Test: The shared-policy GoogleTests passed in a standalone build with actual generated Thrift types; the Iceberg assertion failed with the extracted original policy. Groovy compilation, clang-format 16, and whitespace checks passed. The full BE UT runner was blocked during local clucene/Zstd dependency configuration. The external regression has not been executed locally. - Behavior changed: Yes; missing Iceberg files are no longer ignored. Other table formats retain their existing behavior. - Does this need documentation: No new configuration or API; the corrected behavior is described above.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review at exact head 9aba6d7b9fda43520404458f75b5665bebb9fc80.
The implementation goal is sound: missing files belonging to a committed Iceberg snapshot must fail the query instead of being silently skipped. The shared predicate is small and focused, and all three V1 plus both V2 NOT_FOUND decisions use it. However, the new end-to-end regression deterministically fails during setup, so it never proves the intended behavior; I left one blocking inline comment.
Critical checkpoints:
- Goal and proof: the scanner change accomplishes the intended error policy, but the regression proof is currently invalid because of the URI-scheme assertion.
- Scope and clarity: the production change is narrowly scoped and reuses one helper; no unrelated behavior was added.
- Concurrency and lifecycle: no new shared state, locks, threads, static initialization, ownership, or allocation is introduced. Existing V1/V2 abort, close, block-clearing, and finished-range behavior remains intact.
- Configuration: no configuration item is added. The existing mutable ignore setting is read at each failure boundary; the regression restores per-BE values.
- Compatibility and parallel paths: no protocol or storage-format change is introduced. Current Iceberg native-data, JNI/system-table, position-delete, and count-related ranges carry the same exact per-range
icebergmarker used by reader selection; local and remote split paths copy it. Data-file, delete-resource, and row-id error paths were traced across both scanners. - Error handling and observability: Iceberg NOT_FOUND statuses remain fatal with existing path/status context; non-Iceberg skip behavior and counters are preserved. No additional metric or log is warranted for this policy-only change.
- Transactions, persistence, and data writes: not modified by this PR. No new FE-BE variable or persisted field is introduced.
- Performance: the added work is a constant-time predicate only on NOT_FOUND handling; no hot-path allocation or repeated scan is added.
- Tests: the helper unit covers Iceberg and non-Iceberg truth tables and is discoverable by the BE test glob. The external regression intends to cover both scanners and both config values, but cannot reach them until the inline issue is fixed. Per the review contract, this was a static-only review: no builds, format checks, or tests were run.
User focus: no additional focus points were provided; the complete PR was reviewed.
| def files = sql "select file_path from missing_data_file\$files where content = 0" | ||
| assertTrue(files.size() >= 2, "The snapshot must contain multiple data files") | ||
| URI missingFile = new URI(files[0][0].toString()) | ||
| assertEquals("s3", missingFile.scheme) |
Contributor
There was a problem hiding this comment.
[P1] Accept the fixture's s3a data-file URI
This fixture configures the REST warehouse as s3a://warehouse/wh/, and the write path deliberately commits the preserved raw output path. Consequently $files returns an s3a://... URI here, so this assertion fails before deleteObject or any V1/V2/configuration case runs. Please accept s3a (or remove the scheme assertion) while retaining the host/path checks used for MinIO deletion.
Contributor
TPC-H: Total hot run time: 27634 ms |
Contributor
TPC-DS: Total hot run time: 153511 ms |
Contributor
ClickBench: Total hot run time: 23.96 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? The Iceberg missing-data-file regression assumes metadata paths use s3://, but Hadoop-backed catalogs can return s3a://. Accept the supported s3, s3a and s3n schemes while preserving the bucket/key used for object deletion and rejecting unrelated schemes. ### Release note None ### Check List (For Author) - Test: Reproduced the original s3a assertion failure using the actual URI-handling snippet. Verified s3/s3a/s3n bucket/key parsing and rejection of an unrelated scheme against both branch versions; Groovy compilation and git diff --check passed. Full external regression awaits CI. - Behavior changed: Test fixture compatibility only. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review at exact head 4dbb5dba86a7f94a73c50cf964cbe0b209f21237.
No new actionable findings.
Critical checkpoints:
- Goal and correctness: the change makes missing files from a committed Iceberg snapshot fatal instead of silently returning partial results. All three V1 and both V2
NOT_FOUNDskip decisions now use the shared range predicate, while non-Iceberg behavior remains unchanged. - Scope and clarity: all five changed files and the relevant FE range construction, V1/V2 reader, delete-file/deletion-vector, split scheduling, cleanup, and test-harness paths were reviewed. The helper is narrowly scoped and uses the same exact
icebergmarker already required for Iceberg reader routing. - Concurrency and lifecycle: no new shared state, locking, ownership model, or asynchronous behavior is introduced. Fatal V2 prepare/read errors correctly avoid skipped/finished-range accounting and unwind through scanner/table-reader close; V1 cleanup remains delegate-driven. No partial block can be published before the task error is observed.
- Configuration, compatibility, and parallel paths: no configuration, protocol, or storage-format field is added. The existing mutable ignore setting still governs non-Iceberg ranges, but cannot override Iceberg snapshot correctness. Current and legacy FE range construction carries the marker for data, JNI/system-table, and native delete/deletion-vector variants; nested delete-file errors return through the marked outer range. V1/V2 row-id reads already propagate errors directly.
- Error handling, observability, and performance: missing Iceberg objects retain path/status context and fail the query. Existing skipped-file counters remain limited to files actually skipped. The added work is a bounded field/string check with no allocation; no material performance or observability concern was found.
- Persistence, transactions, and data writes: not changed. There is no new persisted state, FE-BE variable, or transaction boundary.
- Tests: the helper unit test is discoverable by the BE test source glob and covers Iceberg Parquet/ORC/JNI plus non-Iceberg truth tables. The regression removes a committed MinIO data object, verifies absence, disables relevant caches, and checks both scanner versions under both values of the ignore setting with an exact
NOT_FOUNDsubstring oracle. Existing tests cover missing position-delete/deletion-vector sources and both scanner error-propagation paths. Broader ORC/delete-file end-to-end combinations would be additional coverage, but no concrete uncovered incorrect path was found.
The prior inline report about accepting the fixture's s3a URI is fixed at this head by accepting s3, s3a, and s3n, so it was not duplicated. No additional user focus was provided.
Validation disclosure: this review was static-only; I did not build or run tests. At submission time, GitHub reports formatter, Checkstyle, license, secrets, and several aggregate checks passing, while BE UT, compile, performance, and automated-review checks remain pending.
Contributor
TPC-H: Total hot run time: 27379 ms |
Contributor
TPC-DS: Total hot run time: 152819 ms |
Contributor
ClickBench: Total hot run time: 23.76 s |
zhangstar333
approved these changes
Sep 18, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
4 of 12 tasks
morningman
added a commit
that referenced
this pull request
Oct 26, 2026
### What problem does this PR solve? Issue Number: None Related PR: #69571, #68142 Problem Summary: **In short.** When a Paimon split's deletion vector file is missing, the native readers report `NOT_FOUND`, and both file scanners take that for a missing data file and skip the whole range. The query succeeds and silently drops every surviving row of the data file. This PR reports a missing deletion vector as an error, so the query fails instead. A missing data file is still skipped as before. **Background.** A Paimon split for a table with deletion vectors names a data file and the deletion vector that marks its deleted rows. The native readers open the deletion vector before they read a row: V1 in `PaimonParquetReader` / `PaimonOrcReader::_init_deletion_vector`, V2 in `TableReader::_parse_delete_predicates`, both through `DeletionVectorReader`. `FileScanner` and `FileScannerV2` skip a range whose reader reports `NOT_FOUND` while `ignore_not_found_file_in_external_table` is on (the default). That setting exists for file listings that are cached by the FE and may name a file that has since been removed. #68142 already exempted Iceberg ranges, whose files all belong to a committed snapshot. **The problem, and what it cost.** A deletion vector is never a stale listing: it belongs to the snapshot that planned its data file. It can still be missing, for example when Paimon expires the snapshot that wrote it while the data file stays live under a newer one, or when it is looked up under a partition name that is never found (#69571). The data file is still there, but the scanners skip the whole range, so its surviving rows are dropped together with the deleted ones. The BE logs `Skip missing file` with the path of the data file, which exists. Paimon's own reader, the JNI reader and the Rust reader fail on the same split instead. **How this PR fixes it.** `DeletionVectorReader`, which opens every deletion vector for V1 Paimon, V1 Iceberg and the V2 table reader, now reports a missing file as `INTERNAL_ERROR` with the path of the deletion vector, both when it opens the file and when it reads it. The scanners only skip `NOT_FOUND`, so the query fails. A missing data file still reports `NOT_FOUND` and is skipped as before, with or without a deletion vector. For Iceberg, which already failed on any missing file (#68142), only the error code of a missing deletion vector changes. One boundary: if the data file is gone as well, the result depends on which file the reader reaches first. FileScannerV2 parses the deletion vector while it prepares the split, before it opens the data file, so such a split now fails; in the local run below V1 Parquet failed too, while V1 ORC, which opens the data file first, still skipped it. **Results.** A local cluster with a `file://` Paimon catalog, two append tables (Parquet and ORC, `deletion-vectors.enabled = true`), five rows each, the first row deleted through a deletion vector the way a Spark `DELETE` persists it: | Files on storage | Reader | Before | After | |---|---|---|---| | data file and deletion vector | V1 / V2, Parquet / ORC | `2,3,4,5` | `2,3,4,5` | | data file only (deletion vector removed) | V1 / V2, Parquet / ORC | empty result, query succeeds; `Skip missing file: path=<data file>` in be.INFO | `[INTERNAL_ERROR]Deletion vector file is missing: path .../index/index-...-0, failed to get file size ...: No such file or directory` | | deletion vector only (data file removed) | V1 / V2, Parquet / ORC | empty result | empty result (unchanged) | | neither | V2, V1 Parquet | empty result | the deletion vector error above | | neither | V1 ORC | empty result | empty result (unchanged) |
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
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?
An Iceberg snapshot can reference a data file that has been removed from storage. With the default
ignore_not_found_file_in_external_table=true, both file scanners can skip the missing file and return a successful but incomplete result.Exclude Iceberg ranges from the missing-file skip policy, regardless of that setting, while preserving the existing behavior for other table formats. Apply the shared policy at reader initialization and read boundaries. This also prevents a missing delete file reported as
NOT_FOUNDfrom being swallowed by the scanner.Add unit coverage for Iceberg Parquet/ORC/JNI and ordinary external ranges with both setting values. Add an external regression that creates multiple data files, deletes one actual object, and expects row scans to fail with V1/V2 and both setting values. The regression disables file/result caches and restores the BE configuration after testing.
Release note
Iceberg scans now fail when a required snapshot file is missing instead of silently skipping it, including when
ignore_not_found_file_in_external_tableis enabled.Check List (For Author)