Repository navigation
[fix](iceberg) Publish Parquet split offsets in data file metadata - #69615
Merged
Gabriel39 merged 2 commits intoOct 26, 2026
Merged
Conversation
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.
Reviewed all nine changed files at a2a1cf5. No new actionable findings or existing blocking inline findings were identified. Review converged in one round: the complete BE reviewer, complete FE/protocol/test reviewer, and separate physical-offset risk reviewer all returned NO_NEW_VALUABLE_FINDINGS; every investigated candidate and risk has an evidence-based disposition.
The execution chain reviewed was:
Parquet data writer -> successful Close -> retained Arrow footer
-> per-file TIcebergCommitData.split_offsets (optional field 15)
-> final BE report -> FE binary fragment -> Iceberg DataFile
-> atomic manifest/snapshot commit -> eager/streaming split planning
Critical checkpoint conclusions:
- Goal and evidence: the change publishes Parquet row-group starts, including dictionary pages, independently of column metrics. Added BE tests inspect real footers; FE tests check protocol decoding and manifest readback. Tests were inspected, not executed.
- Scope and reuse: the production change is confined to footer collection, the shared data-file commit builder, optional transport metadata, and the existing SDK DataFiles builder.
- Concurrency and locks: offsets are local to each closed writer/file. Existing sorter locking and synchronized FE transaction/report paths remain in effect; no new thread, lock order, or shared mutable state was introduced.
- Lifecycle and static initialization: Arrow retains its shared footer after Close; collection precedes transformer destruction and report publication. Sorted and unsorted rollover close the old writer first. No ownership cycle or static initialization dependency was added. This matches Arrow 24 footer ownership.
- Configuration: no setting was added. Existing metrics-disabled policy intentionally does not suppress physical split offsets.
- Compatibility: Parquet file layout is unchanged; absent old-BE offsets remain absent, and older Thrift readers can skip the new optional field. Existing Iceberg manifest metadata is populated through the SDK.
- Thrift/protobuf: TIcebergCommitData was compared directly on HEAD and branch-3.1, branch-4.0, branch-4.1, and branch-4.2. Field 15 is unused on all four branches and optional here; existing IDs retain their meanings. No protobuf change.
- Parallel paths: INSERT, overwrite, UPDATE/MERGE data, rewrite, sorted output and per-file rollover reach the modified common path. ORC and delete-file conversion correctly exclude these Parquet data offsets.
- Conditional checks: missing metadata fails; empty files/groups omit offsets; dictionary presence includes the earlier page; FE format/null/empty checks handle ORC and old fragments. Comments explain these cases.
- Test coverage: new tests cover dictionary/plain, multiple/single/empty row groups, repeated collection, metrics disabled, per-file isolation, absent optional fields, ORC exclusion, and manifest roundtrip. Regression checks inspect offsets with metrics on/off. Nested/null-only, sorted-rollover and mixed-version execution have no dedicated new offset tests; their control flow was reviewed statically.
- Test results and conventions: no expected-result files changed. New regression checks use deterministic assertEquals calls rather than the repository-preferred qt snapshots; this is a test-convention deviation, with no demonstrated functional defect.
- Observability: the existing Iceberg $files metadata exposes offsets. Existing close/report/commit error paths surface failures, and the report-size guard measures the complete expanded fragment.
- Persistence and failover: offsets follow existing SDK manifest/snapshot commits; no new FE EditLog or crash-recovery state is introduced. Native partition visible-version and delete-bitmap rules are unaffected.
- Writes and atomicity: collection/report errors prevent successful publication and enter existing file cleanup. MERGE retains cleanup ownership until sibling data/delete close results are known; snapshot publication keeps the existing transaction boundary.
- FE/BE propagation: complete generated-Thrift structs are copied, budgeted, serialized and decoded; there is no hand-picked transport stage dropping field 15. Each file receives a fresh builder and independent offsets.
- Performance and memory: collection reads the retained footer without reopening the file; extra work/storage scale with row groups, plus sorting. FE list copies and existing cache accounting cover the added metadata. No new per-row work or significant unbounded buffer was found.
- Other risks: dictionary/data starts and physical leaf order match Iceberg's split-offset extraction. The resulting half-open split intervals agree with Doris native row-group midpoint selection; nested/Variant/null-only paths and nonzero first offsets yielded no reachable counterexample.
No additional user focus was supplied. No unresolved static candidate or unreviewed changed file remains. Validation limits: this invocation performed static review only; it ran no builds, tests, mixed-version binaries, external regression environment, or Spark/Trino interoperability checks. The PR author's reported execution was not independently reproduced.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28624 ms |
Contributor
TPC-DS: Total hot run time: 151563 ms |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
ClickBench: Total hot run time: 24.39 s |
Contributor
There was a problem hiding this comment.
Reviewed PR #69615 at head 776501a04ef4c78fbadf32c25382ed7ca9b659c0. No new actionable finding was verified, and there are no existing inline findings to carry forward. All 11 changed files and their affected producer, transport, commit and reader paths were reviewed.
Critical checkpoint conclusions:
- Goal and evidence: closed Parquet row-group starts reach per-file Iceberg manifest metadata independently of column metrics. Dictionary handling agrees with the upstream Parquet start-offset definition. Added BE/FE tests exercise the main behavior; this review did not execute them.
- Scope and reuse: the changes are focused and reuse the common partition writer and FE data-file builder.
- Concurrency and locks: extraction uses the existing write/close lifecycle; sorted spill operations retain their existing mutex. Commit receipt and publication retain existing serialization. No new thread, shared cache, lock order or blocking operation was introduced.
- Lifecycle and memory: the retained Arrow writer owns metadata after close. Offset vectors and builders are per file; no new ownership cycle or static initialization dependency was found. Existing report-size accounting includes serialized offsets.
- Configuration: no production setting is added. The existing metrics policy remains independent; test configuration overrides are restored.
- Compatibility: absent or empty offsets preserve older fragments; ORC and delete-file metadata retain their prior behavior. The manifest field is already defined by Iceberg, with no new physical data encoding.
- Thrift/protobuf: the full touched definition was compared on HEAD and current master, 3.1, 4.0, 4.1 and 4.2 refs, verified against the remote. Field 15 is unused there, optional, and does not change existing field meanings.
- Parallel paths: partitioned/unpartitioned, sorted/unsorted and rolling data writers converge on the collector. INSERT, OVERWRITE, UPDATE/MERGE data and REWRITE converge on the FE helper; separate delete paths were checked.
- Conditions: metadata availability, empty groups, dictionary pages, format selection and optional-field checks have concrete explanations. Missing required writer metadata returns failure.
- Test coverage: new tests cover dictionary/plain pages, zero/one/multiple groups, repeat collection, metrics disabled, wire decoding, per-file isolation, old fragments, ORC exclusion, manifest round trips and requested-column order across four metadata-table variants and both partitioning modes. The external regression adds offset checks with metrics enabled and disabled.
- Expected results: no result file changed; existing row/file aggregates remain consistent. New footer-derived offset expectations match the write layout. Runtime results were not independently reproduced.
- Observability and errors: collection/report failures propagate through existing close and cleanup handling; no new transaction stage or logging/metric requirement was identified.
- Persistence and failover: offsets enter the existing atomic Iceberg snapshot commit with their data file. No additional Doris EditLog state or independent persistence step is needed; existing rollback ownership is preserved.
- Writes and crashes: files close before metadata is reported; extraction/report errors retain existing failed-file cleanup. No new publication window or partial metadata operation was found.
- FE/BE passing: whole-object Thrift serialization preserves the optional list through final reports and connector deserialization. Appending file size after requested metadata columns satisfies Iceberg's offset validation while the JNI reader consumes the requested leading positions.
- Performance: collection reads the in-memory footer, with O(row groups) storage and O(groups log groups) sorting; it adds no file reopen or per-row work. Metadata projection adds one scalar dependency only when needed and retains pruning of unrequested readable metrics.
- Other invariants: data visibility, native-table versions and MoW delete bitmaps are unaffected. Physical split ownership was traced through both Parquet reader implementations. No new hub-header/PCH/template or glibc compatibility concern was identified statically.
User focus: no additional focus was supplied. The initial offset, projection/JNI and wire-compatibility concerns were all dismissed with source evidence; no correctness candidate remains unresolved.
Validation limits: static review only; no build, style command, unit/regression test, live JNI, mixed-version run, final ELF audit, or Spark/Trino interoperability check was performed. Dedicated nested/all-null offset, sorted rollover and collector-failure tests are absent; those mechanisms were inspected statically. Delete-metadata and predicate-only projection combinations also lack new execution coverage. Review convergence concerns the completed static review, not runtime certification.
Review converged after 3 round(s).
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28523 ms |
Contributor
TPC-DS: Total hot run time: 150958 ms |
Contributor
ClickBench: Total hot run time: 24.56 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
approved these changes
Oct 26, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
4 tasks done
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Oct 29, 2026
Apply the outstanding changes in master merge order: apache#69545 7d16729 apache#69551 7224a2d apache#69612 61d1aef apache#69615 8d2bd1e apache#69650 3dc7341 apache#69651 c00ecb1 apache#69646 e493c61 apache#69668 7865646 apache#69614 already exists on branch-4.2 through apache#69644. Adapt datasource APIs and tests to the release branch, retaining its existing scanner dispatch and statement-bound metadata contracts.
yiguolei
pushed a commit
that referenced
this pull request
Oct 30, 2026
### What problem does this PR solve? Backport the requested external-data and FileScannerV2 changes to `branch-4.2` in their actual master merge order: | Order | Master PR | Change | Backport | | --- | --- | --- | --- | | 1 | #69545 | Preserve Paimon schema options after field renames | Included | | 2 | #69551 | Expand Iceberg capability, transaction and snapshot tests | Included with 4.2 API adaptations | | 3 | #69614 | Elasticsearch HTTP scans in FileScannerV2 | Already present through #69644 (`2104b7509cda`); not applied twice | | 4 | #69612 | Check catalog visibility before SHOW DATABASES accesses metadata | Included | | 5 | #69615 | Publish Iceberg Parquet split offsets | Included | | 6 | #69650 | Preserve ES compound residuals and expand scanner coverage | Included | | 7 | #69651 | Accept Elasticsearch 9.x during node discovery | Included | | 8 | #69646 | Prioritize Parquet V2 row groups using TopN bounds | Included | | 9 | #69668 | Arrow loads and schema inference in FileScannerV2 | Included | The eight outstanding changes are combined in one commit. This branch retains the 4.2 FE datasource APIs instead of importing master's connector-module refactor. ### Backport adaptations - Port Paimon schema restoration, catalog-option provenance, fallback handling and tests to the existing table-copy/serialization lifecycle. Keep statement-bound fields when reader options are applied after a schema-only rename. - Exercise Iceberg's existing metadata and transaction entry points with real in-memory tables and retained-generation fixtures. Preserve the nine disabled known-bug reproductions; omit two connector-only lifecycle tests. Expiration checks verify live data-file sets and exact delete-file counters; 4.2's separate manifest-name/data-counter issue remains unchanged. The delete/history SQL suite checks visible rows and eager/late-materialization parity because 4.2 does not expose master's `_file`/`_pos` columns. The inventory labels upstream-only evidence explicitly. - Port ES residual tracking to the legacy planner and add compound OR/NOT, docvalue eligibility and Thrift-context tests there. Use Java 8-compatible HTTP APIs for opt-in ES 9 tests and the branch's supported node-address fixtures. - Keep the existing full-schema Iceberg metadata-table scan, which already supplies the file-size dependency for split offsets. Adapt the external write suite to verify offsets without requiring master-only distribution plans or golden output. - Use the existing Parquet statistics interface and physical row-location representation. Keep the TopN scheduling and pruning invariants. - Include the shared Arrow array normalizer required by the new reader, taken from the same master revision. Preserve 4.2 Lance/Avro dispatch; omit the schema-inference test for the unavailable Hive OpenCSV parser. ### Validation - FE production and test sources compile with the branch's Java 8 target. Across the targeted runs and focused reruns, 20 test classes report 181 discovered, 171 passed, zero failures/errors, and 10 skipped: nine intentionally disabled known-bug reproductions plus the opt-in live ES 9 class. - FE Checkstyle: passed with zero violations. - Clang compiler syntax checks: all 23 affected C++ translation units passed against the current sources and freshly generated Thrift, using cached third-party/generated build dependencies. Focused BE runtime validation is listed below. - clang-format 16: all 30 affected C++ source/header files passed. - Groovy 4.0.19 syntax and AST conversion: all seven changed suites passed. All 105 local inventory links resolve. - Complete staged-diff self-review, whitespace/conflict checks, and sensitive-information checks passed. A full BE rebuild and SQL/external-service regressions were not run locally. Live ES 9 tests require their opt-in endpoint. CI is requested for the combined backport. ### Release note Fix Paimon reads after field renames, external-catalog SHOW DATABASES authorization, and ES compound residual filtering. Support ES 9.x node discovery, publish Iceberg Parquet split offsets, improve Parquet V2 TopN row-group scheduling, and use FileScannerV2 for Arrow loads and text/JSON schema inference. ### Check List (For Author) - Test - [x] Unit Test - [x] Regression test additions and syntax checks; SQL execution pending CI - Behavior changed: - [x] Yes. Backport the changes listed above. - Does this need documentation? - [x] No. Existing interfaces and settings are retained; the test coverage inventory is included. ### Follow-up fixes - Set the Thrift `enable_file_scanner_v2` default to `true`, matching the session default. Preserve explicit `false` for legacy query scans and extend the existing load-selection test. - Keep the selected Paimon snapshot and schema together during relation projection. Historical sequence/aggregation options must not be combined with the latest field names. Read-only principals can resolve historical selectors without INSERT permission. - Read legacy append schemas with missing or empty bucket keys without bucket pruning, preserving every historical file. The regression covers both unaware and multiple fixed buckets. Follow-up validation: 169 targeted Paimon FE tests passed with zero failures/errors/skips. FE production compilation and Checkstyle passed. Both changed C++ test translation units passed compiler syntax checks and clang-format 16. Regenerated Java/C++ defaults are enabled; a Java 8-targeted protocol check verifies constructor/clear defaults, an omitted wire field, and an explicit-false round trip. Complete BE CI and live external SQL suites remain pending. The two new reproductions first failed with the same missing-bucket-key and renamed-sequence-field errors as CI. A separate read-only-principal reproduction also failed before the permission correction. Complete external regression execution remains pending CI. ### BE unit-test compatibility - Complete the native Arrow Variant input dependency for `branch-4.2`: accept binary storage and the Parquet Variant extension, validate metadata and payloads before appending, and retain owned data after Arrow buffers are released. Allow that ingestion path through the load reader while preserving query-format restrictions and source-default validation. - Preserve Variant V2 markers in the Arrow test fixtures, including nested types. Verify timestamp input against this branch's SerDe for positive, negative, and second-boundary values. Match the existing DATE range diagnostic in the TopN pruning test. - Add tests for sliced and empty reads, invalid ranges and carriers, malformed metadata/payloads, NULL children, atomic failure, buffer ownership, and the query capability boundary. Validation: all 13 previously failing BE tests passed. The expanded run passed all 309 tests across nine suites, including Arrow/JSON loads, load validation, Iceberg, table mapping, and Variant input/output. The focused executable rebuilds the affected readers and SerDe against cached 4.2 build dependencies; this is not a complete BE rebuild. All seven changed C++ translation units passed compiler checks with current headers/generated Thrift, and all 33 affected C/C++ files passed clang-format 16. The complete staged diff, whitespace, and sensitive-information checks passed. Full CI is requested with `run buildall` and `/review`.
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?
Doris writes Parquet row groups for Iceberg data files but does not publish their split offsets in commit fragments or manifests. Readers cannot use those known boundaries when planning splits.
Collect row-group starts from the closed writer's in-memory Footer, including dictionary pages, and carry them through an optional Thrift field into Iceberg data-file metadata. Keep collection independent of column metrics policy; leave older BE fragments, empty files, ORC and delete files without Parquet offsets.
When a file metadata-table scan requests
split_offsets, also projectfile_size_in_bytes: Iceberg uses the size to validate offsets and otherwise returns null for valid values. Append this dependency after the requested columns to preserve the BE JNI reader's positional contract.Release note
Publish Parquet row-group split offsets in Iceberg manifests and preserve them in projected file metadata-table scans.
Validation
IcebergScanPlanProviderTesttests and 31IcebergWriterHelperTesttests, including Manifest round trips and per-file isolation.Check List (For Author)