Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66348, apache#68854, apache#69376 Problem Summary: Partition writers call open before registering a new writer in the partition map. If the output file is created but the Parquet or ORC transformer then fails to open, the table writer cannot reach that writer through its close path. Clean up the created file at the open failure boundary so the error cannot leave an orphan, matching the lifecycle hardening introduced on master by apache#66348. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [ ] Unit Test - [x] Manual test: repository clang-format check and direct compilation of the modified BE writer translation unit passed. - Behavior changed: - [x] No. - Does this need documentation? - [x] No.
|
run buildall |
TPC-H: Total hot run time: 28182 ms |
TPC-DS: Total hot run time: 150921 ms |
ClickBench: Total hot run time: 24.31 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Reviewed all four changed files at cf14958. No new actionable defect was substantiated, and there are no existing inline blockers. Two complete-scope reviewers and a separate failure-lifecycle reviewer each returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty finding set. Static review converged after one round; no unresolved code-path question remains within the assigned scope.
Critical checkpoint conclusions:
- Goal and evidence: all three Parquet writers resolve compression before create_file. Unsupported codecs return the original error without constructing a file writer or transformer. The added test checks rejection and file absence, with the limitation below.
- Scope and reuse: the change is focused and retains the shared codec conversion helper; each local lambda preserves the existing creation arguments.
- Concurrency and locking: existing async writer ownership, sort mutexes, and active-writer snapshots remain unchanged. Failed opens are excluded from newly published snapshots; no new thread, lock, shared state, or lock ordering is introduced.
- Lifecycle and memory safety: traced static/dynamic partition dispatch, sort, MERGE, delete-sink cleanup, and destruction. Async failure reaches close; null transformers are guarded, failed writers cannot build commits, and original failure is preserved. No new static initialization or ownership cycle.
- Configuration: none added or changed.
- Compatibility: no interface, successful codec encoding, or storage-format change; no new rolling-upgrade requirement.
- Thrift/protobuf: no definitions or field IDs changed.
- Parallel paths: Iceberg data/delete Parquet and ORC, plus Hive Parquet/ORC/CSV, each retain exactly one creation call on supported branches. Hive retains its S3 committer option. GZ still maps to GZIP.
- Conditional/error handling: DORIS_TRY and RETURN_IF_ERROR propagate codec and creation failures. Existing transformer-error handling remains; no speculative conditional check was added.
- Test coverage: the new local Iceberg negative test is registered by the existing recursive test glob and uses supported BE-UT private access. No new Hive/delete, successful-codec, or remote-filesystem case was added. Builds and tests were not run under the static-only contract.
- Test expectations/results: the expected unsupported-codec error and eventual absence match the inspected logic; no golden result files changed.
- Observability: codec errors and existing cleanup warnings remain available; this ordering change needs no additional metrics or logging.
- Persistence/failover: no EditLog, metadata replay, or commit-protocol change. Rejected opens cannot reach commit publication along the inspected callers.
- Writes/atomicity/crashes: successful write/report behavior is preserved; rejected compression avoids the unnecessary output-file side effect. No new crash leak or publication inconsistency was identified.
- FE/BE variables: none added or changed.
- Performance: the immediate stack lambda adds no allocation or scan, while rejected codecs avoid file creation. Existing memory accounting is unaffected; no benchmark was run.
- Other concerns: rechecked format defaults, rollover, partial initialization, lambda lifetime, options, and cleanup. No additional issue was substantiated. No extra user focus was supplied.
Validation limit: UnsupportedParquetCodecDoesNotCreateFile asserts eventual file absence. On the local filesystem, the previous create-then-delete implementation also satisfies that assertion, so the test alone does not prove that creation was never attempted. An assertion that _file_writer remains null or instrumentation of create_file would strengthen that invariant. This is a test-strength observation; static control-flow inspection confirms the production ordering fix.
Review converged after 1 round(s).
|
/review |
There was a problem hiding this comment.
Reviewed all four changed paths at cf14958. No new verified defects or existing blocking inline findings. The main review and both assigned reviewers converged after one round; both reviewers returned NO_NEW_VALUABLE_FINDINGS. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and correctness: Iceberg data, Iceberg position-delete, and Hive Parquet writers resolve the shared compression mapping before create_file. Unsupported codec errors propagate before writing or constructing commit metadata. The added negative unit test checks the returned error and absence of a residual file, subject to item 10.
- Scope and reuse: The change is small and focused. Local synchronous lambdas reuse the existing file path/options, and the shared Parquet codec helper remains authoritative.
- Concurrency and locks: No new shared state, threads, locks, or lock ordering. The existing async writer preserves failures through close; sorting and rollover retain their existing synchronization.
- Lifecycle and static initialization: Lambda captures remain alive throughout the synchronous call. Failed codec validation leaves no file writer or transformer; retained failed writers receive close(error), whose null checks and status guards prevent publication. Dynamic failed writers are destroyed locally. No new static initialization or ownership cycle.
- Configuration: No configuration items or dynamic-update behavior changed.
- Compatibility: No public signature, compression mapping, persisted file encoding, or protocol changed. Hive file options and compatibility gate remain intact. No new libc API was introduced; completed ELF artifacts and runtime compatibility were not audited.
- Thrift/protobuf: No definitions, field IDs, enum values, or required fields changed.
- Parallel paths: All three Parquet branches were checked. Iceberg data/delete ORC and Hive ORC/CSV still create exactly one file before constructing their transformer; unsupported formats do not create one. Initial open, static/unpartitioned versus dynamic registration, and sorted rollover were traced.
- Conditions and error handling: Standard DORIS_TRY/RETURN_IF_ERROR propagation preserves non-OK failures. No new conditional predicate was introduced; existing post-creation transformer cleanup remains.
- Test coverage: The new test is valid by static inspection, uses an isolated UUID path, and is included by the BE unit-test glob. Its existence assertion does not prove that create_file was skipped: the previous Iceberg implementation could create and then unlink the file before returning, also satisfying that assertion on Linux. A create-call assertion or checking that _file_writer remains null would test the ordering directly. No new Hive/delete-writer-specific or end-to-end tests are added. This is a confirmed validation limitation, with no unresolved production-code candidate.
- Expected results: No regression .out or other expected-result files changed. The error substring matches the shared helper; the filesystem absence check matches the current final state.
- Observability: Existing returned codec errors, cleanup warnings, and writer profiles are adequate for this ordering change; no additional logging or metrics are needed.
- Transactions, persistence, and failover: No EditLog, replay, transaction protocol, or persisted metadata changed. Existing success guards still control Iceberg/Hive commit updates and delete-file commit data.
- Writes and atomicity: Invalid codecs avoid the file side effect. Actual caller failure propagation prevents a failed open from publishing data, and supported-format create/transformer failure cleanup is preserved. No new crash-leak window was identified.
- FE/BE variable passing: No new transmitted variables or sender paths.
- Performance and memory: Successful writes retain one conversion and one create; the local lambda adds no owning allocation. Invalid codecs avoid filesystem creation and file-writer allocation. No new large allocation, memory-tracking, reservation, or column/null-handling behavior.
- Final recheck: Revisited all changed paths, codec success/failure, failed creation, null-transformer cleanup, sort rollover, unsupported formats, and delete-sink publication. No additional verified defect or unresolved review question remains. Visibility/version and MoW delete-bitmap rules are unaffected.
This was a static review: no builds, tests, binary audits, or source modifications were performed. Convergence covers the reviewed source and explicit candidate dispositions; it does not establish runtime test success.
Review converged after 1 round(s).
cf14958 to
b47b8d1
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all four changed paths at b47b8d1. No new actionable correctness findings, and no existing inline P0/P1 findings to carry forward. Two general reviewers and a separate lifecycle reviewer checked the unchanged empty inline set.
Critical checkpoint conclusions:
- Goal: all three writers resolve the Parquet codec before creating the output file; unsupported codecs return the existing error without allocating a file writer.
- Scope: focused changes to creation ordering, plus one negative unit test; the existing filesystem API and shared codec mapping are reused.
- Concurrency: the new lambdas execute synchronously within open and retain no stack references beyond it; no changed threads, locks, atomics, or dependency signaling.
- Lifecycle: checked static, dynamic, unpartitioned, sorted, rollover, and delete-sink callers. Open failures propagate into failure cleanup; no newly reachable successful commit or null-transformer use was found. No new static initialization.
- Configuration: none added or changed.
- Compatibility: supported codec mappings, filesystem options, transformer arguments, timestamp settings, and file encodings remain unchanged; no signature or new libc API dependency.
- Thrift/protobuf: no definition or field-ID changes.
- Parallel paths: all three generic-to-Parquet codec callers are updated; Iceberg ORC and Hive ORC/text retain their creation flow.
- Conditions and errors: codec and filesystem errors remain checked and propagated; unsupported format branches avoid creation. No newly ignored Status.
- Test coverage: the added test checks ZLIB rejection and final local-file absence. Its assertions also hold for the previous create-then-delete data-writer implementation, so it does not prove zero calls to create_file. Intercepting/counting that operation would strengthen the regression check. The other two writers and remote-storage failure behavior have no added targeted test.
- Test results: no expected-result files changed. No builds, unit tests, regression tests, formatting, or static-analysis commands were run under this static-only review contract.
- Observability: existing codec, format, and filesystem errors remain available; no additional logging or metrics appear necessary for this ordering fix.
- Transactions and persistence: no EditLog, metadata replay, visible-version, or MoW changes; commit reporting and rollback ownership remain unchanged.
- Data writes and atomicity: rejected codecs now fail before output creation; supported write/close/report paths and existing upload cleanup are preserved. No new crash-cleanup regression was identified by code tracing.
- FE/BE variables: no transmitted variables or send sites changed.
- Performance and memory: no new hot-loop work, large allocation, ownership cycle, or retry; invalid codecs avoid file creation and cleanup.
- Further recheck and focus: all changed hunks and special cases were rechecked; no additional user focus was supplied and no unresolved correctness candidates remain.
Validation limits: conclusions are based on static code inspection. Compile/link, test execution, and object-store behavior were not verified at runtime. The test limitations above are nonblocking coverage observations, not separate inline bug findings.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28312 ms |
TPC-DS: Total hot run time: 150389 ms |
ClickBench: Total hot run time: 24.07 s |
b47b8d1 to
ccc5227
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69376 at ccc5227 identified one P2 regression-test defect, reported inline. The production changes put unsupported Parquet codec rejection before file creation in all three writers; no introduced production correctness defect was found. There are no existing inline threads or independently confirmed existing P0/P1 blockers, and no additional user-provided focus points.
Critical checkpoint conclusions:
- Goal: the source achieves reject-before-create. The new test does not establish that property because the old create-and-delete flow satisfies its assertions; see the inline finding.
- Scope and clarity: the changes are small and focused; supported transformer options and writer options remain the same.
- Concurrency and locking: the local lambdas run synchronously and do not escape. No thread, shared variable, lock, lock order, or atomic ordering is added or changed; existing I/O dispatch waits for completion.
- Lifecycle and initialization: normal, spill, sorted, MERGE, Hive async, and delete owners preserve failed-open status before cleanup or commit/report. Null writer/transformer members on rejection are safe in those callers. No new ownership cycle or static initializer is introduced.
- Configuration: no configuration is added or changed; dynamic propagation is not applicable.
- Compatibility: no interface, wire format, storage encoding, compression mapping, or supported timestamp behavior changes. No new external API or toolchain dependency is introduced; no release-artifact ABI audit was run.
- Thrift/protobuf: no definitions, field IDs, or enum values change.
- Parallel paths: Iceberg data and delete Parquet/ORC plus Hive Parquet/ORC/text and unsupported-format branches were checked. Every supported branch still creates exactly one file writer before transformer open.
- Conditions and error handling: no new if/else predicate is added. Existing Result/Status macros preserve codec and creation errors, with no silent continuation into writes or commit construction.
- Test coverage: the added local negative test has the inline-reported blind spot. No new Hive/delete negative-codec tests are included. Related supported writer and cleanup tests were inspected statically; no build or test was executed.
- Expected results: no golden output files change. The unit-test expectations were reviewed from source; no executed result is claimed.
- Observability: unsupported-codec Status text and existing close/cleanup warnings are preserved. This ordering change does not add a stage needing new metrics or logs.
- Transactions and persistence: successful external commit/report gating and rollback protocols are unchanged. No EditLog, persistence schema, or failover protocol changes.
- Data writes and atomicity: rejected codecs create no writer/upload; successful formats retain their creation/transform/write/close sequence. Retained failed writers close with failure, preventing missing-file commits; dynamic failures destruct before insertion. Existing crash/rollback guarantees are unchanged. Internal visible versions and MoW delete bitmaps are not affected.
- FE/BE variables: none are added or changed.
- Performance and memory: rejected codecs avoid file I/O and writer allocation; supported paths retain one creation call. The reference-capturing local lambdas introduce no material buffer or reservation requirement.
- Other BE checks: no hub header, include, PCH, template-instantiation, unity-skip, or hygiene-table change occurs; the test includes no source .cpp. The final recheck found no additional verified issue.
Coverage includes all four changed files and relevant caller, cleanup, filesystem, transformer, and report paths. No unresolved static candidate remains. Validation is limited to static inspection: builds, unit/regression tests, remote-filesystem execution, and runtime experiments were not run, as required by the review contract.
Review converged after 2 round(s).
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69376 Problem Summary: The negative codec test only checked that no file remained after open. The previous create-then-delete implementation could also satisfy that assertion. Instrument LocalFileSystem::create_file_impl through the existing sync point and assert it is never called, while retaining the null writer and final absence checks. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test: strengthened UnsupportedParquetCodecDoesNotCreateFile; CI will run it. - [ ] Manual test - Behavior changed: - [x] No. - Does this need documentation? - [x] No.
|
run buildall |
|
/review |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
ClickBench: Total hot run time: 24.32 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69376
commit: bf2a3b312d64a4df17eceeebfd0c23b136ef2293
base: 7acfd9bfc27f01f0b953e8ac0cdc3482a46e3c59
reviewed_at: 2026-11-04T07:12+00:00
reviewer: hubgeter
model: gpt-6.1-sol
effort: max
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69376 Problem Summary: The Iceberg data, position-delete and Hive partition writers wrap a single filesystem create_file call in forwarding lambdas. Expand the calls directly at each format branch to make the validation and file-creation order explicit, while preserving every argument, writer option and error path. ### Release note None ### Check List (For Author) - Test: clang-format 16 via the repository formatter and git diff --check passed. - No need to rerun runtime tests: this mechanically expands seven existing forwarding calls without changing arguments or branch positions. Verified the remaining source tokens and existing negative test are unchanged. - Behavior changed: No. - Does this need documentation: No.
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69376
commit: 9b8cc64cf043ae1b6d3f53eab10c77cfe0c722ef
base: 7acfd9bfc27f01f0b953e8ac0cdc3482a46e3c59
reviewed_at: 2026-11-04T08:35+00:00
reviewer: hubgeter
model: gpt-6.1-sol
effort: max
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
|
run buildall |
TPC-H: Total hot run time: 28353 ms |
TPC-DS: Total hot run time: 151062 ms |
ClickBench: Total hot run time: 24.65 s |
Issue Number: N/A Related PR: apache#68854, apache#69338 Problem Summary: The Iceberg data, position-delete, and Hive Parquet writers created their output file before validating the requested Parquet compression codec. For an unsupported codec, the Iceberg data writer then attempted to delete an open file, while the other writers returned after creating the file. This made error cleanup depend on filesystem behavior and could leave an orphan file. Resolve the codec before calling create_file so invalid codecs fail without creating any output file, and add a regression test for the failure path. None - Test - [x] Unit Test: added UnsupportedParquetCodecDoesNotCreateFile; not executed locally per request, CI will run it. - [ ] Regression test - [ ] Manual test - [ ] No need to test or manual test. Explain why - Behavior changed: - [x] No. - Does this need documentation? - [x] No.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69376 Problem Summary: The negative codec test only checked that no file remained after open. The previous create-then-delete implementation could also satisfy that assertion. Instrument LocalFileSystem::create_file_impl through the existing sync point and assert it is never called, while retaining the null writer and final absence checks. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test: strengthened UnsupportedParquetCodecDoesNotCreateFile; CI will run it. - [ ] Manual test - Behavior changed: - [x] No. - Does this need documentation? - [x] No.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69376 Problem Summary: The unsupported-Parquet-codec regression test included common/sync_point.h, which does not exist under the configured BE include roots. This prevented the doris_be_test target from compiling. Include cpp/sync_point.h and keep the include list ordered so the existing zero-file-creation assertions can compile against the shared SyncPoint API. ### Release note None ### Check List (For Author) - Test: clang-format 16 for the four PR files, BE build hygiene, git diff --check, and a compiler probe that rejects the old include and accepts the corrected include and SyncPoint API usage all passed. - Unit Test: attempted run-be-ut.sh --run --filter=VIcebergPartitionWriterTest.* -j 2 with ASAN; stopped during the initial faiss submodule clone before compilation. No local BE unit-test runtime pass is claimed. - Behavior changed: No; this commit corrects a test include only. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69376 Problem Summary: The Iceberg data, position-delete and Hive partition writers wrap a single filesystem create_file call in forwarding lambdas. Expand the calls directly at each format branch to make the validation and file-creation order explicit, while preserving every argument, writer option and error path. ### Release note None ### Check List (For Author) - Test: clang-format 16 via the repository formatter and git diff --check passed. - No need to rerun runtime tests: this mechanically expands seven existing forwarding calls without changing arguments or branch positions. Verified the remaining source tokens and existing negative test are unchanged. - Behavior changed: No. - Does this need documentation: No.
### Background Iceberg writers carry separate physical and metadata directory forms in IcebergOutputLocation. Their WriteInfo aggregate requires this location to be initialized before a writer can be constructed. ### What problem does this PR solve? Issue Number: None Related PR: apache#69376 (follow-up), apache#69392 (output-location interface change) PR Type: Test Problem Summary: The unsupported-Parquet-codec test still default-constructed WriteInfo and assigned its removed write_path field. After merging current master this prevents the BE unit-test target from compiling. Initialize the physical and metadata directories explicitly while retaining the existing zero-file-creation regression assertions. ### Details - Failure: BE UT compilation reports a deleted WriteInfo default constructor and a nonexistent write_path member in the new codec-rejection test. - Cause: the output-location refactor changed WriteInfo after the PR branched. - Stability evidence: use the same aggregate pattern as current master tests, with both local directory forms set to '.', FILE_LOCAL, and no brokers. Keep the unique filename, SyncPoint callback and all assertions unchanged. ### Testing - clang-format 16 on the four PR files, build hygiene, and git diff --check passed. - git range-diff confirmed the four original patches are preserved by rebase. - ASAN run-be-ut.sh --run --filter=VIcebergPartitionWriterTest.UnsupportedParquetCodecDoesNotCreateFile -j 2 was attempted locally, but stopped during dependency preparation before compilation. Required submodules were incomplete and the mandatory libgoogle_cloud_cpp_oauth2.a is absent. No local runtime test pass is claimed. ### Release note None ### Check List (For Author) - Test: static checks passed; local unit-test execution remains unverified. - Behavior changed: No. - Does this need documentation: No. - Pre-submission status: Partially verified: BE UT compilation and execution need verification on the prepared CI environment. ### AI Disclosure - Tool: Codex. - Generated: test initialization and this commit message. - Human review: Not performed in this automated session.
9b8cc64 to
8df4f80
Compare
|
run buildall |
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69376
commit: 8df4f80b1f0824a91311db51ffc5a43d72c3b24c
base: 315c2e003317ef44e8a8fbb445a1ec8452d5647e
reviewed_at: 2026-11-05T03:41+00:00
reviewer: hubgeter
model: gpt-6.1-sol
effort: max
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
TPC-H: Total hot run time: 28348 ms |
TPC-DS: Total hot run time: 151615 ms |
ClickBench: Total hot run time: 24.35 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
|
PR approved by at least one committer and no changes requested. |
What problem does this PR solve?
Issue Number: N/A
Related PR: #68854, #69338
Problem Summary:
The Iceberg data, position-delete, and Hive Parquet writers created their output file before validating the requested Parquet compression codec. For an unsupported codec, the Iceberg data writer then attempted to delete an open file, while the other writers returned after creating the file. This made error cleanup depend on filesystem behavior and could leave an orphan file. Resolve the codec before calling
create_fileso invalid codecs fail without creating any output file, and add a regression test for the failure path.Release note
None
Check List (For Author)
Test
UnsupportedParquetCodecDoesNotCreateFile; CI will run it.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)