[fix](iceberg) Allow optional position delete fields without nulls#65401
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review result: request changes.
I found one issue: the PR changes the standalone delete-file helper plus the v1 and format_v2 scan-reader position-delete paths, but the new tests only exercise the helper. Please add reader-path coverage for nullable file_path/pos delete files, including no-null success and actual-null rejection through prepare_split / get_block.
Critical checkpoint conclusions: the code is narrowly scoped to Iceberg position-delete nullable handling; no new concurrency, lifecycle, config, persistence, FE/BE protocol, or observability surface was introduced; error propagation is improved by returning Status from _gen_position_delete_file_range; performance impact appears limited to null-map checks on delete-file batches. The only blocking gap I found is test coverage for the parallel reader paths changed by this PR.
Validation: static review only. This checkout is shallow and lacks .worktree_initialized, thirdparty/installed, thirdparty/installed/bin/protoc, and a built doris_be_test, so I could not run the BE unit test. I did run git diff --check and clang-format-16 --dry-run --Werror on the changed C++ files successfully.
User focus: no additional user-provided review focus.
Subagent conclusions: tests-session-config proposed TSC-1, accepted as the inline comment. optimizer-rewrite reported no optimizer/rewrite, join, aggregate, or delete-application semantic issue. In convergence round 1, after M-1 was accepted and the final comment set contained only M-1, both live subagents replied NO_NEW_VALUABLE_FINDINGS.
8c74eff to
8162057
Compare
|
/review |
|
run buildall |
|
Codex automated review failed and did not complete. Error: Review step was skipped (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Static review completed for PR 65401.
I did not find any new non-duplicate correctness issue to submit as an inline comment. The one substantiated remaining concern I found is already covered by the existing inline thread on be/test/format/table/iceberg/iceberg_delete_file_reader_helper_test.cpp: reader-path coverage for nullable Iceberg position-delete schemas, especially the legacy/v1 IcebergParquetReader path. Current head adds format_v2 reader-path tests for nullable no-null, null file_path, and null pos, while the remaining v1 coverage request is already in that thread, so I am not duplicating it.
Critical checkpoint conclusions:
- Goal and tests: the code changes consistently read Parquet position-delete
file_path/poscolumns as nullable, reject actual nulls, strip nullable wrappers, and propagate errors through helper, v1 reader, and format_v2 collector paths. Helper and format_v2 tests were added; v1 reader-path coverage remains the already-open review-thread item. - Scope: the implementation is focused on Iceberg position-delete readers and related tests.
- Concurrency/lifecycle: no new thread, lock, static-initialization, or ownership lifecycle risk found. Existing delete-file cache use remains unchanged except that parse errors are now propagated from
_gen_position_delete_file_range. - Configuration/compatibility/protocol: no new config, persisted format, Thrift, or FE-BE protocol change. This is a reader-side compatibility relaxation for optional Parquet physical schemas without accepting actual null Iceberg values.
- Parallel paths: helper, legacy/v1 Parquet reader, ORC call-site status propagation, and format_v2 Parquet collector were inspected. The remaining test gap for the v1 reader path is already covered by the existing thread.
- Error handling/null handling: actual nulls return
Status::Corruptionwith the offending column name; callers now check the returnedStatus. Nullable columns are unwrapped only afterhas_null(0, read_rows)succeeds. - Performance/observability: no material new hot-path issue found; the added null scan is per delete-file batch and error messages identify
file_pathorpos. - Test result review:
git diff --checkpasses. I did not run BE unit tests because this checkout is missing.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc. - User focus: no additional user-provided review focus was present.
Subagent conclusions:
optimizer-rewrite: no candidates; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.tests-session-config: no candidates; it independently noted the only remaining coverage concern is duplicate of the existing thread; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.- Final proposed inline comment set for this pass: empty.
8162057 to
87ea629
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review summary for PR #65401 at 87ea629.
I did not find any new source-level issue that should be submitted as an additional inline comment. The nullable position-delete reader-path coverage concern is already present in the existing inline thread discussion_r3548953477, so I did not duplicate it.
Critical checkpoint conclusions:
- Goal: the PR relaxes Iceberg Parquet position delete reads so optional
file_path/posfields without actual nulls are accepted, while actual nulls fail. The production changes implement this by reading nullable columns, checking batch null maps, and stripping the nullable wrapper before the existing string/dictionary/int64 processing. - Scope: the patch is focused on Iceberg position-delete helper, v1 reader path, format_v2 reader path, and related BE tests.
- Concurrency/lifecycle: no new shared mutable state, threads, locks, globals, or static initialization concerns were introduced.
- Configuration/session/persistence/protocol: no new config, session variable, persisted format, or FE/BE protocol field is added.
- Compatibility: the behavior change is a compatibility relaxation for Parquet schemas that mark Iceberg-required delete fields optional. Actual null values still return corruption errors; ORC behavior is effectively unchanged except status propagation from the shared v1 helper.
- Parallel paths: helper-level reads, v1 Parquet/ORC position-delete range generation, and format_v2 collection were reviewed. Status propagation from
_gen_position_delete_file_rangeis now checked by both v1 callers. - Tests: helper-level nullable Parquet tests and format_v2 reader-path nullable tests are added. The remaining v1 reader-path coverage ask is already covered by the existing inline review thread, not repeated here.
- Test results: I did not run BE UT locally because this runner lacks
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc. GitHubBE UT (macOS)failed before Doris compilation withERROR: The JAVA version is 25, it must be JDK-17, so I did not treat that as a source-level failure from this patch. Added-line trailing whitespace check over the GitHub patch was clean. - Observability/errors: the new corruption messages identify the offending delete column (
file_pathorpos), which is sufficient for this path. - Data correctness: position deletes remain version/path based as before; the change only affects physical nullability handling while preserving null rejection.
- Performance: the added per-batch null-map scan is bounded by the delete batch size and avoids per-row null branching after validation.
Subagent conclusions:
optimizer-rewrite: no optimizer/rewrite, parallel join, or aggregate-path candidate found.tests-session-config: no distinct evidence-backed test/config/style candidate found beyond existing thread discussion_r3548953477.- Convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same ledger and empty proposed inline comment set.
User focus: no additional user-provided review focus was present.
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
…pache#65401) Relax Iceberg position delete Parquet reads so Doris can consume delete files where `file_path` or `pos` are declared optional in Parquet metadata but contain no actual null values. The reader now materializes position delete columns as nullable, validates that each batch has no nulls, then strips the nullable wrapper before building delete ranges. Actual null values in `file_path` or `pos` still fail with a corruption error. This covers the legacy Iceberg Parquet reader, the shared delete-file helper used by Iceberg writes, and the format_v2 position delete collector. Some writers, including DuckLake/DuckDB-produced data seen in the field, can produce Iceberg position delete files whose required Iceberg fields are encoded as optional Parquet fields. Doris previously rejected these files before checking whether the data actually contained nulls.
…pache#65401) Relax Iceberg position delete Parquet reads so Doris can consume delete files where `file_path` or `pos` are declared optional in Parquet metadata but contain no actual null values. The reader now materializes position delete columns as nullable, validates that each batch has no nulls, then strips the nullable wrapper before building delete ranges. Actual null values in `file_path` or `pos` still fail with a corruption error. This covers the legacy Iceberg Parquet reader, the shared delete-file helper used by Iceberg writes, and the format_v2 position delete collector. Some writers, including DuckLake/DuckDB-produced data seen in the field, can produce Iceberg position delete files whose required Iceberg fields are encoded as optional Parquet fields. Doris previously rejected these files before checking whether the data actually contained nulls.
…pache#65401) Relax Iceberg position delete Parquet reads so Doris can consume delete files where `file_path` or `pos` are declared optional in Parquet metadata but contain no actual null values. The reader now materializes position delete columns as nullable, validates that each batch has no nulls, then strips the nullable wrapper before building delete ranges. Actual null values in `file_path` or `pos` still fail with a corruption error. This covers the legacy Iceberg Parquet reader, the shared delete-file helper used by Iceberg writes, and the format_v2 position delete collector. Some writers, including DuckLake/DuckDB-produced data seen in the field, can produce Iceberg position delete files whose required Iceberg fields are encoded as optional Parquet fields. Doris previously rejected these files before checking whether the data actually contained nulls.
## Summary Backport the requested Apache Doris PRs to `branch-4.1` in their master merge order: 1. #63648 2. #64033 3. #64263 4. #64315 5. #63825 6. #65332 7. #65354 8. #65094 9. #65401 10. #65502 11. #65135 12. #65742 13. #65709 14. #65548 15. #65676 16. #65770 17. #65759 18. #65782 19. #65784 20. #65674 21. #65921 22. #65934 23. #65925 24. #65931 #64263, #65354, #65094, #65502, and #65770 are already effectively present on current `branch-4.1`, so this branch does not duplicate those commits. The remaining requested changes are represented by ordered backport commits followed by branch-4.1 compatibility fixes. ## Compatibility notes - Rebased onto the latest `upstream/branch-4.1` and retained both sides of the Iceberg scan conflict resolution. - Keep `be/src/format_v2` and `be/test/format_v2` exactly aligned with current `upstream/master`, as requested. - Apply branch-4.1 compatibility adaptations outside the format_v2 paths. - Preserve the selected PR behavior while avoiding dependencies on unrelated master-only changes. - #65921 supplies the Parquet benchmark scenario header required by `parquet_benchmark_scenarios_test.cpp`, fixing the BE UT compile failure. - Adapt branch-4.1 `VRuntimeFilterWrapper` at the master TableReader boundary so FileScannerV2 residual predicates keep globally rewritten slot indexes. - Keep non-transactional JDBC V2 connections usable by avoiding the unsupported auto-commit transition before Hikari invalidates the proxy. ## FE UT fix - Widen the auto-profile test timing margins to prevent CI scheduling delays from selecting the wrong threshold branch. - Keep the table-filter correctness assertions and use broader performance guardrails suitable for shared FE UT workers. ## Verification - Full local FE+BE build: `Successfully build Doris`. - ASAN BE UT build completed successfully; `test/doris_be_test` linked with exit code 0. - Related BE tests: 382/382 passed across 32 suites, including FileScannerV2, runtime filters, Parquet, Iceberg readers, and format_v2 coverage. - JDBC scanner UT: 3/3 passed; Maven reactor `BUILD SUCCESS`; Checkstyle reports 0 violations. - ClickHouse JDBC V2 regression: 1 suite, 0 failed suites, 0 fatal scripts. - Iceberg branch/tag regression (`iceberg_branch_complex_queries`): 1 suite, 0 failed suites, 0 fatal scripts. - Repository clang-format 16 check: passed for 4,191 files. - `git diff --check`: passed. - `be/src/format_v2` and `be/test/format_v2` have zero diff from current `upstream/master`. --------- Signed-off-by: Gabriel <liwenqiang@selectdb.com> Co-authored-by: daidai <changyuwei@selectdb.com>
3 participants
Proposed changes
Relax Iceberg position delete Parquet reads so Doris can consume delete files where
file_pathorposare declared optional in Parquet metadata but contain no actual null values.The reader now materializes position delete columns as nullable, validates that each batch has no nulls, then strips the nullable wrapper before building delete ranges. Actual null values in
file_pathorposstill fail with a corruption error.This covers the legacy Iceberg Parquet reader, the shared delete-file helper used by Iceberg writes, and the format_v2 position delete collector.
Problem
Some writers, including DuckLake/DuckDB-produced data seen in the field, can produce Iceberg position delete files whose required Iceberg fields are encoded as optional Parquet fields. Doris previously rejected these files before checking whether the data actually contained nulls.
Testing
git diff --checkJDK_17=/usr/local/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home JAVA_HOME=/usr/local/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home ./run-be-ut.sh --run --filter='IcebergDeleteFileReaderHelperTest.*'contrib/openblas,contrib/faiss, OpenMP).DORIS_THIRDPARTYalso blocked before compiling due missingthirdparty/installed/bin/protocand Snappy.