[fix](build) Keep Arrow 17 and 24 in shared thirdparty - #66546
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
77cf9ff to
8a5c653
Compare
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
1 similar comment
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
c857f27 to
fef0713
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for three canonical build/compatibility defects, detailed inline.
Checkpoint conclusions:
- Goal, scope, and tests: reviewed the complete 15-file dual-stack/build/ADBC change and its upstream/downstream consumers. The focused Arrow/Paimon lifecycle and third-party script checks pass; Linux/macOS/macOS-arm64 third-party builds, BE UT, compile, and automated code-review are still pending.
- Configuration, compatibility, and lifecycle: selected-prefix state can drift from or bypass preflight; the legacy root migration is not failure-atomic; and target normalization/consumer gating is inconsistent. These are blocking compatibility and recovery defects.
- Parallel and conditional paths: checked BE, Cloud, standalone compile-bench, tools, external/install-only prebuilts, pre-upgrade branch-4.1 consumers, clean/retry/interruption paths, macOS recovery, explicit prefixes, and reused CMake caches.
- Tests, results, and observability: the ADBC formatted output remains consistent and the independent raw-value comparison remains intact. Missing lifecycle cases correspond to the inline findings. Failure diagnostics exist, but two paths either silently expose an ABI-mixed prefix or validate artifacts that are not consumed.
- Concurrency, transactions, persistence, data writes, and protocol variables: no applicable runtime changes. No additional performance concern was found beyond the intentional cost of retaining both stacks. No additional user focus was supplied.
Review completion: capped/incomplete. Two valuable candidate instances surfaced in the third and final allowed round. Every currently known candidate is resolved and represented by the three deduplicated inline comments, but the round limit prevents claiming convergence.
fef0713 to
4484046
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Requesting changes for one current-release compatibility blocker, detailed inline.
Checkpoint conclusions:
- Goal and proof: the PR coherently builds Arrow/Paimon 17 at the legacy root and Arrow/Paimon 24 at a versioned prefix, and master selects the versioned pair consistently. The local lifecycle harness and shell syntax checks pass, but the shared-artifact goal is not complete because the current post-backport branch-4.1 consumer still requires Arrow/Paimon 24 at the unversioned root.
- Scope and clarity: the changes are focused on source/build/install selection, validation, cleanup, CMake imports, and the two related regression adjustments.
- Concurrency: no runtime thread or shared runtime state is introduced. Thirdparty mutation remains sequential with the existing single-writer assumption.
- Lifecycle and static initialization: no C++ static-initialization concern applies. For supported consumers, patch and component markers publish only after validation, interrupted cleanup leaves an invalid rather than ABI-mixed pair, and root/versioned cleanup preserves the other stack. The previously reported interrupted-root case is fixed at this head.
- Configuration: Arrow/Paimon roots, versions, fingerprints, and Paimon enablement are build-time inputs, not dynamic runtime settings. Master preflight and CMake resolve one matched versioned pair; unsupported build.sh root overrides fail before recovery.
- Compatibility and parallel paths: master, BE tools, benchmarks, standalone compile-bench, Cloud-only, install-only/source recovery, macOS/Linux, pre-backport branch-4.1, branch-4.0, and sibling publication paths were checked. Current branch-4.1 remains the sole incompatible consumer and is the inline blocker.
- Conditions and errors: incomplete artifacts, stale fingerprints, unsupported roots, and unavailable install-only recovery fail with component/prefix context; no speculative runtime guard was added.
- Tests and results: the lifecycle harness passes locally and in CI, relevant scripts pass
bash -n, and Thirdparty Script Test is green. The harness covers selective recovery, interrupted migration, source isolation, cleanup ownership, Cloud-only, compile-bench, and explicit-root rejection, but omits the live current branch-4.1 validator/import contract. Linux/macOS/macOS-arm64 thirdparty builds, BE UT, compile, and automated code-review are currently pending. The ADBC formatting preserves the expected display output, while the following null-safe raw-value join still checks all five FLOAT/DOUBLE rows. - Observability: build/configuration diagnostics are sufficient for these offline paths; runtime logs and metrics are not applicable.
- Transactions, persistence, data writes, FE/BE variables, memory, and nullable handling: no applicable production-path change. The regression formatting remains null-propagating and raw null/value equality is independently checked.
- Performance: retaining a second source/build/static stack intentionally adds thirdparty build and storage cost; focused recovery rebuilds only invalid pairs, and no runtime hot path changes.
- Other issues and user focus: the three existing inline findings appear fixed at this head and were kept as deduplication fences. No additional user-provided review focus was supplied.
Review completion: complete after two full rounds. Every changed path and identified risk received a final conclusion; all three second-round reviewers returned NO_NEW_VALUABLE_FINDINGS, and the sole accepted blocker survived independent falsification and live duplicate checks.
| # distinguish this stack from unrelated thirdparty changes. | ||
|
|
||
| # arrow | ||
| # Arrow 24 is installed in a versioned prefix. The unversioned install prefix is |
There was a problem hiding this comment.
[P1] Keep the current branch-4.1 consumer compatible with this layout
The live branch-4.1 already contains the Arrow 24 backport and still validates/imports Arrow/Paimon 24 from the unversioned installed root (including libarrow_compute.a); its BE-UT script refreshes the official install-only tree from this same shared automation asset. This change replaces that root with the Arrow/Paimon 17 contract and puts 24 only under installed/arrow-24.0.0. Once the asset uses this layout, the current branch rejects it and cannot recover, while a source checkout rebuilds 24 into the root and removes compatibility for the pre-upgrade revisions this change is meant to preserve. Please coordinate/land a branch-4.1 consumer change that selects the versioned 24 prefix before publishing this artifact, and cover that branch's validator/import contract in the lifecycle test.
There was a problem hiding this comment.
Confirmed against the live apache/branch-4.1 head aa135178aaf1. The current consumer still validates the shared automation archive at the unversioned installed root in run-be-ut.sh / thirdparty/arrow-paimon-vars.sh, and BE imports that same root via THIRDPARTY_DIR.
Therefore this P1 is valid: a master-only change cannot make one fixed unversioned path expose Arrow/Paimon 17 to pre-backport revisions and Arrow/Paimon 24 to the current branch at the same time. The dual-layout artifact must not be published until a companion branch-4.1 consumer change selects installed/arrow-24.0.0. I intentionally did not add an ABI-mixed shim or hide the rollout dependency in this PR, and I am leaving this thread unresolved until that branch change is authorized and landed.
For the code in this PR, the rebase onto current master is now at cf4a25729ff; the focused lifecycle test, shell syntax checks, and formatting checks pass. The prior-head TeamCity Compile, BE UT, and Performance builds also passed.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#66221 Problem Summary: master and pre-upgrade branch-4.1 revisions consume the same prebuilt thirdparty package. Upgrading the unversioned Arrow/Paimon stack to Arrow 24 makes unchanged branch-4.1 code compile against incompatible headers and libraries. Keep Arrow 17/Paimon 17 in the legacy unversioned prefix, install Arrow 24/Paimon 24 in a versioned prefix selected by master, build and validate both source closures independently, and recover only stale stacks. ### Release note None ### Check List (For Author) - Test: - Manual test: downloaded and checksum-verified the latest Linux x86_64 thirdparty prebuilt - Manual test: built Arrow/Paimon 17 and Arrow/Paimon 24 thirdparty stacks - Manual test: ran thirdparty/test/arrow-paimon-lifecycle-test.sh - Manual test: completed an ASAN master BE and FE build - Behavior changed: Yes. The shared thirdparty prefix keeps Arrow/Paimon 17 at the legacy root and installs the Arrow/Paimon 24 stack selected by master under a versioned directory. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66221 Problem Summary: Arrow 24 preserves DBL_MAX as a finite 17-digit value, while the existing ADBC regression baseline is intentionally rendered with 16 significant digits. Passing the 17-digit text through the test client can reparse it above DBL_MAX as infinity. Explicitly format the displayed DOUBLE column to 16 significant digits while retaining the existing bit-for-bit value comparison inside Doris. ### Release note None ### Check List (For Author) - Test: - Manual test: verified format("{:.16g}", value) output for every non-null DOUBLE fixture value - Manual test: git diff --check for this commit - Behavior changed: No. This only stabilizes an existing external regression baseline. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66221 Problem Summary: Review found that reused CMake caches could retain stale automatic Arrow/Paimon prefixes, build.sh validated the shared default even when CMake selected another pair, an interrupted root-prefix Arrow 17 migration could expose Paimon built against Arrow 24, and target normalization incorrectly gated Cloud-only and standalone compile-bench builds. Refresh CMake selections on every configure, reject unsupported build.sh prefix overrides before recovery, remove Paimon before downgrading the legacy Arrow prefix, and normalize dependency consumers before preflight. The PR no longer modifies the macOS BE workflow because upstream intentionally removed PR triggers and the existing thirdparty workflow already validates both macOS builds. ### Release note None ### Check List (For Author) - Test: - Manual test: bash -n for the modified build and lifecycle scripts - Manual test: thirdparty/test/arrow-paimon-lifecycle-test.sh - Manual test: git diff --cached --check - Behavior changed: Yes. Cloud-only builds skip unused Arrow/Paimon validation, standalone compile-bench performs BE preflight, unsupported explicit build.sh prefixes fail clearly, and interrupted Arrow 17 migration removes ABI-incompatible Paimon artifacts first. - Does this need documentation: No
4484046 to
cf4a257
Compare
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Reviewed the full authoritative diff at cf4a25729ff27e70b6b788dc0587ba488bcf54c4. I am requesting changes for two newly introduced issues below; the separate unresolved branch-4.1 artifact-first P1 remains valid.
Checkpoint conclusions:
- Goal and normal paths: the dual source/install layout, Arrow 17 historical patches, Arrow 17/24 archive differences, matched CMake imports, and ADBC semantic assertion are internally consistent outside the findings below.
- Compatibility and rollout: the current official root-only install artifact is incompatible with the new mandatory versioned master consumer (P1). Existing review threads fence the inverse branch-4.1 ordering problem and direct split-prefix path.
- Failure handling and lifecycle: source alias extraction, marker invalidation, legacy-root cleanup, and cross-stack ownership are fail-closed for newly introduced paths. Two additional traces were reproduced but dismissed because the same behavior exists at the base revision.
- Performance: Paimon-only legacy invalidation unnecessarily repeats the newly added Arrow 17 build (P2).
- Tests: the focused lifecycle harness and shell syntax checks pass; both macOS thirdparty CI legs and script checks pass. The harness does not cover the immediately previous official archive or the Paimon-only legacy selector case. Linux thirdparty, BE UT, and compile checks were still pending during the final sweep.
- Concurrency, runtime resource lifetime, configuration/state mutation, transactions, persistence, and data writes: no additional applicable regression was found in this build-only change.
- User focus: no additional focus was supplied.
| fi | ||
|
|
||
| if [[ "${NEED_ARROW_PAIMON_THIRDPARTY}" == "true" ]]; then | ||
| DEFAULT_ARROW_PAIMON_HOME="${DORIS_THIRDPARTY}/installed/${ARROW_INSTALL_SUBDIR}" |
There was a problem hiding this comment.
[P1] Keep the previous official prebuilt usable during rollout
This makes installed/arrow-24.0.0 mandatory, but the current automation archive is stamped 1731787677f0 and contains Arrow/Paimon 24 only at the unversioned root. Official compilation images expose only that installed tree as DORIS_THIRDPARTY, so a default build on the new master reaches rebuild_thirdparty_libraries() and exits because the install-only prefix has no build-thirdparty.sh. Publishing the dual layout first is not safe either: it replaces root 24 with 17 for live pre-merge master and the current branch-4.1 consumer (the latter is the existing unresolved thread); the manual arbitrary-ref workflow still clobbers this same shared release. Please add a transitional, fingerprint/version-validated root-24 fallback that prefers the versioned prefix, or stage a consumer/artifact rollout with no mismatched window, and cover the immediately previous official archive in the lifecycle test.
| local install_dir="$1" | ||
| ARROW_PAIMON_REBUILD_PACKAGES=() | ||
|
|
||
| if ! arrow_paimon_17_prebuilt_valid "${install_dir}"; then |
There was a problem hiding this comment.
[P2] Do not rebuild valid Arrow 17 for a Paimon-only change
This composite check appends arrow_17 paimon_cpp_17 even when arrow_17_prebuilt_valid succeeds and only the Paimon 17 marker/input is stale. For example, a Paimon-only cache change or retry after paimon_cpp_17 fails leaves Arrow 17 valid, yet the new selector repeats that large build; a fixture with valid Arrow and invalid Paimon markers selects all four packages. The patch already publishes independent component fingerprints, and build_paimon_cpp_17() validates/reuses Arrow through require_arrow_17_prebuilt_for_paimon(), so select only paimon_cpp_17 in this case and add the component-only recovery case to the lifecycle test.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#66546 Problem Summary: The branch-4.1 BE UT job can consume the master automation prebuilt, whose unversioned Arrow/Paimon stack no longer satisfies the shared Arrow 17/24 layout. The download path also certified only the versioned stack, library stripping assumed lib was a symlink, and record batch readers ended early for a leading empty row group. Rebuild missing stacks in a source checkout, validate both stacks before reuse or replacement, strip explicit library directories, and skip empty row groups before reading the next batch. ### Release note None ### Check List (For Author) - Test: Unit Test / Manual test - `./thirdparty/test/arrow-paimon-lifecycle-test.sh` - Arrow 17 and Arrow 24 `GetRecordBatchReaderSkipsEmptyLeadingRowGroup` tests - `./thirdparty/build-thirdparty.sh -j 8 arrow_17 paimon_cpp_17 arrow paimon_cpp` - `./build-support/check-format.sh` - Shell syntax and `git diff --check` - Behavior changed: Yes. Shared prebuilt reuse now requires both Arrow/Paimon stacks, and empty row groups no longer terminate the patched reader early. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66546 Problem Summary: Shared Arrow/Paimon recovery could validate the default tree while BE UT consumed independently overridden ABI roots, Paimon installation depended on the platform default libdir, Arrow empty-row-group skipping left Paimon on a zero-width logical range, and archive validation accepted incomplete SDK artifacts. Select one versioned build home for all BE entrypoints, fix Paimon installation at lib64, synchronize its row-group ranges with Arrow, and validate consumed headers plus nonempty static archives before publishing or installing a prebuilt. ### Release note None ### Check List (For Author) - Test: Unit Test / Manual test - `./thirdparty/test/arrow-paimon-lifecycle-test.sh` - `./thirdparty/build-thirdparty.sh -j 8 arrow_17 paimon_cpp_17 arrow paimon_cpp` - `./thirdparty/build-thirdparty.sh -j 8 paimon_cpp_17 paimon_cpp` - Arrow 17 and Arrow 24 Paimon empty-row-group smoke tests with production static libraries - Paimon `file_reader_wrapper_test.cpp` compilation against Arrow 17 and Arrow 24 - `./build-support/check-format.sh` - Shell syntax and `git diff --check` - Behavior changed: Yes. BE build entrypoints enforce one ABI pair, shared prebuilts require consumable SDK artifacts, and Paimon skips zero-width row-group ranges. - Does this need documentation: No
Reverts #66546. no need this.
4 participants
What problem does this PR solve?
Related PR: #66221
Problem Summary: master and branch-4.1 consume the same prebuilt thirdparty prefix. Upgrading the unversioned Arrow/Paimon stack to Arrow 24 makes unchanged branch-4.1 compile against incompatible headers and libraries. Keep Arrow 17/Paimon 17 in the legacy unversioned prefix, install Arrow 24/Paimon 24 in a versioned prefix selected by master, build and validate both source closures independently, and recover only stale stacks.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)