Revert "[fix](build) Keep Arrow 17 and 24 in shared thirdparty" - #67330
Conversation
This reverts commit e5f6299.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
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: the fresh single-root Arrow/Paimon revert is mechanically coherent, but five supported build and rollout paths remain unsafe.
Accepted findings:
- P1: a prebuilt produced by this head carries a Paimon semantic marker that live
branch-4.1rejects from the sharedautomationasset. - P1: interrupted dual-to-single recovery can leave root Arrow 24 beside Paimon 17, and direct
run-be-ut.shconfiguration consumes that root without pair validation. - P1: Cloud-only builds are forced through an irrelevant BE Arrow/Paimon preflight and can fail on otherwise sufficient install-only prefixes.
- P2: standalone compile-bench is normalized after the clean-only exit and dependency gate, so
--cleancan exit without benchmarking and ordinary runs can skip required preflight. - P2:
PAIMON_HOMEis accepted as a cache/environment input but is ignored by all headers and imported libraries.
Checkpoint conclusions:
- Goal, scope, and correctness: all 13 changed paths were reviewed. The steady-state single-root files close cleanly, but target classification, explicit-root semantics, cross-branch publication, and failure-safe migration are incomplete.
- Lifecycle and compatibility: no runtime concurrency, static initialization, transaction, persistence, FE/BE protocol, or data-write change applies. The material lifecycle boundaries are completed artifact publication and the non-atomic intermediate filesystem state; they are separate defects.
- Configuration, errors, parallel paths, and performance: marker-using paths generally fail closed, while the direct BE unit-test entry point bypasses that boundary and
PAIMON_HOMEfails silently. Compile-bench is a real BE consumer that skips validation; Cloud is a nonconsumer that incurs unnecessary validation/recovery cost and failure exposure. No runtime hot path is changed. - Tests and results: changed shell syntax checks and the retained lifecycle harness passed; no build was run in this review environment. Existing steady-state coverage does not exercise the five negative cases above. The ADBC DBL_MAX display concern was dismissed after tracing the actual JDBC path: FLOAT/DOUBLE values are read with
getBytes, so the checked-in raw text remains reachable. - Review status: complete after three rounds. The final normal and risk-focused reviewers returned
NO_NEW_VALUABLE_FINDINGS; every candidate was accepted, deduplicated, or dismissed with evidence. No live review thread duplicates these comments. There was no additional user-provided focus. Reviewed exact headb3b89e3824301c798cd4bd25f1ab0f9b5e536e7cover basead179b5afac6a4420729da98a74860c90bf917a7.
|
|
||
| option(ENABLE_PAIMON_CPP "Enable Paimon C++ integration" ON) | ||
| set(PAIMON_HOME "" CACHE PATH "Paimon install prefix") | ||
| option(ENABLE_TDE "Enable TDE feature module" OFF) |
There was a problem hiding this comment.
[P2] Do not accept an ignored Paimon install prefix
This cache/env input no longer selects anything: the PR removes ${PAIMON_HOME}/include, every Paimon archive in be/cmake/thirdparty.cmake now comes from ${THIRDPARTY_DIR}, and no other PAIMON_HOME reference remains. A caller can therefore set a valid custom prefix and silently link the default tree. Either honor the same root consistently for preflight, headers, and libraries, or remove/reject the unsupported override explicitly.
| if [[ "${BUILD_BE}" -eq 1 || "${BUILD_META_TOOL}" == "ON" || | ||
| "${BUILD_FILE_CACHE_MICROBENCH_TOOL}" == "ON" || | ||
| if [[ "${BUILD_BE}" -eq 1 || "${BUILD_CLOUD}" -eq 1 || | ||
| "${BUILD_META_TOOL}" == "ON" || "${BUILD_FILE_CACHE_MICROBENCH_TOOL}" == "ON" || |
There was a problem hiding this comment.
[P1] Do not require the BE Arrow/Paimon stack for Cloud-only builds
cloud/cmake/thirdparty.cmake imports neither Arrow nor Paimon, but this condition makes ./build.sh --cloud validate them anyway. With an external/install-only prefix whose BE stack is stale or absent (including the current layout transition), the build enters rebuild_thirdparty_libraries() and exits because no source builder exists before Cloud configuration starts. Remove BUILD_CLOUD from this BE-stack gate and restore the Cloud-only dispatch case.
| ! arrow_paimon_prebuilt_valid "${DORIS_THIRDPARTY}/installed"; then | ||
| echo "Arrow/Paimon thirdparty libraries need to be rebuilt ..." | ||
| rebuild_thirdparty_libraries false "${ARROW_PAIMON_BUILD_PACKAGES[@]}" | ||
| fi |
There was a problem hiding this comment.
[P1] Make the shared prebuilt acceptable to live branch-4.1
This rebuild will publish the root Paimon marker 9887cf1ec13106da4250f8d7ea931a258879089c, but live branch-4.1 (211f6165abaf) validates the same automation asset against cb82e41ba46f534e611cdd52e66b53c227d49bf8 and only permits legacy marker dbb6ca6e243cb3aa783b7a8011f97afda9e7ea28. Its staged archive installer therefore rejects a prebuilt produced by this revert before replacing installed; the branch-4.1 dual-layout companion (#67320) was closed. Coordinate a branch-4.1 compatibility update (or preserve a marker it accepts) and cover that live validator contract before publishing the reverted asset.
| # Everything that is not the BE C++ build would only add noise, so force | ||
| # a BE-only build regardless of the other options. | ||
| BUILD_BE=1 | ||
| BUILD_FE=0 |
There was a problem hiding this comment.
[P2] Normalize compile-bench before the clean-only exit
Standalone --compile-bench leaves every BUILD_* target zero until this block. With --clean, the earlier clean-only branch therefore exits successfully without a benchmark; without it, Arrow/Paimon preflight is skipped even though the mode later configures BE (so a stale or dual-layout root is used unvalidated). Move this BE-target normalization before both gates and restore dispatch coverage for standalone and --clean benchmarks.
| build_arrow() { | ||
| check_if_source_exist "${ARROW_SOURCE}" | ||
| invalidate_arrow_prebuilt_marker "${TP_INSTALL_DIR}" | ||
| cd "${TP_SOURCE_DIR}/${ARROW_SOURCE}/cpp" |
There was a problem hiding this comment.
[P1] Invalidate the paired Paimon install before overwriting root Arrow
A dual-layout tree still has Arrow/Paimon 17 in the root. This recovery invalidates only Arrow, installs and publishes root Arrow 24, and does not invalidate or replace root Paimon 17 until build_paimon_cpp() starts. If the process is interrupted in that window, run-be-ut.sh can configure BE directly from the mixed root without calling the pair validator, with Paimon enabled by default; the imported Paimon 17 archives expose Arrow types and are not ABI-safe with Arrow 24. Invalidate/remove the co-located Paimon artifacts before overwriting Arrow (or stage/swap the pair atomically), and add a dual-to-single interruption case covering the direct BE entry point.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
3 participants
Reverts #66546.
no need this.