[bug](iceberg) use split file format for iceberg scan#65760
Open
zhangstar333 wants to merge 4 commits into
Open
[bug](iceberg) use split file format for iceberg scan#65760zhangstar333 wants to merge 4 commits into
zhangstar333 wants to merge 4 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Request changes: the Iceberg per-split format path is largely sound, but the shared range-construction reorder breaks Hudi JNI scans, and the current head also fails the required FE style gate.
Critical checkpoint conclusions:
- Goal and end-to-end behavior: Ordinary Iceberg eager, lazy/batch, rewrite-context, and table-level-count splits all carry each
DataFile.format()into the range, while system-table and delete-file paths retain their dedicated formats. V1 and V2 BE readers honor the per-range value, so the intended mixed Parquet/ORC dispatch is present. Defaulting a property-less Iceberg table to Parquet avoids the extra manifest planning pass and matches Iceberg's default. - Scope and parallel paths: The Iceberg-specific pieces are focused, but changing the shared
FileQueryScanNodehook ordering is not isolated. Hudi consumes the inherited range format while buildingTHudiFileDesc; the resulting JNI/native mismatch is the blocking issue described inline. - Concurrency and lifecycle: No new lock ordering or concurrent mutation was introduced. The split format is assigned before eager use or publication to the existing lazy split queue, and every production data-split path preserves it.
- Compatibility and error handling: No Thrift schema or persisted format changed; current BE code already supports the optional per-range override with scan-level fallback. Unsupported Iceberg formats fail explicitly. The Hudi descriptor mismatch nevertheless violates the existing FE/BE contract and makes MOR/forced-JNI scans fail.
- Configuration, persistence, transactions, and data writes: No configuration, EditLog, transaction, or storage-format change is introduced by this PR.
- Tests and validation: The new unit tests verify property lookup and a manually initialized ORC split, but do not exercise a real mixed-format eager/lazy range pipeline or the shared Hudi hook. No builds were run in this review-only environment. The live CheckStyle job fails on the now-unused
DdlExceptionimport inIcebergScanNode.javaand the lexicographic position of the newTFileFormatTypeimport inIcebergScanNodeTest.java; both must be fixed and the gate rerun. - Performance and observability: Removing the property-less
planFiles()fallback avoids redundant manifest planning. Existing scan/profile/error paths are sufficient for this change; no additional metrics are required.
No additional user-provided focus was supplied. The full changed-file set, shared-base connector paths, tests, and current CI result were reviewed.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Found two actionable test issues: the mixed-format bootstrap fixture is not repeatable against retained Iceberg state, and the property-absent unit test does not exercise the populated-table branch that previously planned files. Please address the inline comments.
Critical checkpoint conclusions:
- Goal and data correctness: The production change carries each Iceberg
DataFile.format()into its split/range, so mixed Parquet/ORC snapshots dispatch the correct reader. Property-absent tables use Iceberg's Parquet write default without planning data files. - Scope and clarity: The production change is focused on split-level format propagation and removal of the repeated planning side effect. The two scope-local defects are confined to fixture repeatability and the strength of the new regression guard.
- Parallel paths: Eager, lazy/batch, manifest-cache, table-level count, rewrite-context, ordinary system-table, position-delete, and independent delete-file-format paths were checked. All non-system data paths use
createIcebergSplit; system paths bypass the nullable field safely. - Concurrency and lifecycle: The new split field is populated before the split is published through
SplitAssignment; no new shared mutable lifecycle, lock ordering, static initialization, or cleanup hazard was introduced. - Compatibility and FE/BE transport: The existing optional range-level thrift field is reused. Both legacy and V2 BE scanners prefer the range format with scan-level fallback. The existing Hudi P1 is fixed at the current head by seeding the scan default before connector-specific setup and is not duplicated here.
- Configuration, persistence, and transactions: No configuration, EditLog, Doris transaction, or storage-serialization contract is added. The Iceberg sink, commit, metrics, and compression callers were traced and consistently use the property-derived write default, while physical reads use each split's actual format.
- Tests and expected results: The split-format unit test and ordered regression output are internally consistent, but
run31.sqlviolates the drop-before-use fixture rule and breaks repeated Iceberg stack initialization. The property-absent lookup test also leavescurrentSnapshot()null, so it passes the removed empty-table shortcut and does not guard elimination ofplanFiles()for populated tables. Per the review contract, this was a static review; no builds or tests were run. - Performance and observability: Removing
planFiles()from format lookup avoids repeated full planning, and no new hot-path copy, logging, metric, or observability issue was found. - User focus and completeness: No additional user focus was provided. The whole changed-file list, existing review thread, accepted/dismissed candidates, and upstream/downstream paths were swept; after three normal/risk convergence rounds, no unresolved point remains beyond the two inline test issues.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Approval opinion: no new blocking issues found on 2eda0b5de745e1bc32e3ec4383abca99bb5c5b4b.
Critical checkpoint conclusions:
- Goal and proof: The PR now carries
DataFile.format()into each ordinary Iceberg range, so one active snapshot can read Parquet and ORC files with their respective readers. It also removes data-file planning from table-default format resolution. The propertyless-table unit test, split-format unit test, and ordered external mixed-format regression are each sensitive to the corresponding base behavior. - Scope and clarity: The production change is limited to scan-level default seeding, split-level Iceberg format propagation, and property-default resolution; the fixture and tests directly cover those behaviors.
- Concurrency: The only concurrency-adjacent path is lazy split production.
params.format_typeis initialized beforeSplitAssignmentstarts, and eachIcebergSplitreceives its immutable enum before queue publication. No new lock, lock ordering, or shared mutable format state is introduced. - Lifecycle: Ordinary eager, lazy, count-pushdown, rewrite, and manifest-cache paths all construct splits through
createIcebergSplit. System-table factories take early branches before the nullable data-file format is read. Both BE scanner generations reset reader/split state between ranges. - Configuration: No configuration item is added or changed.
- Compatibility: No thrift field, enum, symbol, or persisted storage format changes. The existing optional range-level format is preferred by both legacy and V2 scanners, with the scan-level value retained as the compatibility fallback when the range field is absent.
- Parallel paths: Hudi again sees the initialized scan default before its hook; Paimon and Iceberg can still override the range per split; Hive, MaxCompute, LakeSoul, Trino, TVF, plugin, and Remote Doris paths retain their previous behavior.
- Special conditions and error handling: Ordinary split formats are always populated in production, system-table formats remain specialized, and unsupported data formats fail loudly instead of silently selecting a reader.
- Test coverage: The bootstrap is rerun-safe, proves three records in each physical format, and the ordered data query exercises normal mixed-format planning. The unit tests cover the populated propertyless-table optimization and direct range override. Negative unsupported-format behavior remains fail-fast.
- Test results: Per the automated-review contract, I did not build or run tests. This checkout lacks
.worktree_initializedandthirdparty/installed/bin/protoc; conclusions are static. GitHub CheckStyle is passing at submission time, while compile and FE UT checks are still pending. - Observability: Existing reader-format diagnostics and unsupported-format exceptions are sufficient for this focused change; no new metric or log is required.
- Transactions, persistence, and writes: No transaction protocol, EditLog, failover, or persistence boundary changes. Propertyless future writes now follow Iceberg's Parquet default, while existing files are read using each file's own format.
- FE-BE propagation: The existing
TFileRangeDesc.format_typecarries the per-split value on every ordinary Iceberg path; delete-file formats remain independently propagated. - Performance: Format resolution no longer calls
planFiles(), and per-split overhead is only an enum mapping. No new repeated scan, copy, or allocation was found. - Other issues: The three existing inline concerns (Hudi hook ordering, repeatable bootstrap, and the propertyless-table mock) are fixed on the current head and were not duplicated. No additional user focus was provided.
Review completion: two normal full-review passes and one separate risk-focused pass all returned NO_NEW_VALUABLE_FINDINGS in Round 1. Main-agent verification resolved every risk item and found no unresolved candidate or inline comment to submit.
Contributor
TPC-H: Total hot run time: 29590 ms |
Contributor
TPC-DS: Total hot run time: 177880 ms |
Contributor
ClickBench: Total hot run time: 25.11 s |
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
3 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?
Problem Summary:
A table maybe contain mixed parquet and orc files.
before used the table-level format for every split files, it's should use from data file.
and when table properties were absent, maybe trigger full planFiles() call for every splits, this cause mush times.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)