Skip to content

[fix](thirdparty) order Paimon codec dependencies - #67158

Merged
hello-stephen merged 3 commits into
apache:masterfrom
Gabriel39:fix/paimon-zstd-build-dependency
Aug 28, 2026
Merged

[fix](thirdparty) order Paimon codec dependencies#67158
hello-stephen merged 3 commits into
apache:masterfrom
Gabriel39:fix/paimon-zstd-build-dependency

Conversation

@Gabriel39

@Gabriel39 Gabriel39 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

A clean parallel Paimon C++ build can start compiling the object library before bundled codec external projects install their headers. This makes block_compression_factory.cpp fail with a missing codec header depending on the build graph state.

What is changed and how does it work?

  • Add the zstd, snappy, and lz4 targets to the Paimon object library dependencies so their ExternalProject install steps finish before codec sources compile.
  • Keep existing complete Paimon prebuilts compatible because dependency ordering does not change installed artifacts.
  • Add a focused regression test that applies the Doris patch and verifies all three codec dependencies, and run it in the Arrow/Paimon workflow.

Check List

  • Arrow/Paimon lifecycle test
  • Paimon codec dependency regression test
  • Patch dry-run and application against paimon-cpp-0a4f4e2
  • Shell syntax and workflow YAML validation

Release note

None.

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review status: complete after 2 rounds. One blocking build-graph issue remains, so I am requesting changes.

Critical checkpoint conclusions

  • Goal and proof: The PR correctly adds explicit object-library ordering for ZSTD, but it does not fully achieve the stated clean-parallel-build guarantee because the same object library directly requires LZ4 and still has no lz4_ep predecessor. The focused test passes while that edge is absent.
  • Scope and clarity: The four-file change is small and focused. The workflow routing, patch application, and semantic-fingerprint update are otherwise coherent.
  • Concurrency: The relevant concurrency is CMake/Ninja scheduling. In Doris's external-Arrow configuration, paimon_objlib waits for zstd_ep and snappy_ep but not lz4_ep; favorable scheduling or a host header can mask the race but is not an ordering guarantee.
  • Lifecycle: The ExternalProject configure/build/install lifecycle was traced through object compilation. Adding lz4 to this dependency fence is sufficient because the imported lz4 target already owns add_dependencies(lz4 lz4_ep). No runtime or static-initialization lifecycle is involved.
  • Configuration and compatibility: No Doris runtime configuration is added. The current Paimon semantic hash matches the updated compatibility pin, only known complete legacy markers are admitted, artifact validation remains mandatory, and dependency ordering does not alter installed runtime artifacts.
  • Parallel paths and conditions: Bundled Arrow is safe through arrow_ep -> lz4_ep; Doris's PAIMON_USE_EXTERNAL_ARROW=ON path is the affected path. Static and shared outputs consume the same object library and have the same missing predecessor. Linux and macOS workflow routing is otherwise correct.
  • Tests and results: The Arrow/Paimon lifecycle test and the new focused script pass locally and in focused CI. The new script checks only zstd and snappy in a synthetic fragment, so it misses the LZ4 edge. Full Linux/macOS thirdparty builds and general compile/BE checks were still pending at submission; a passing run would not prove the missing graph edge because the race can be masked.
  • Error handling and observability: Patch/test failures are propagated with actionable diagnostics. No additional runtime logs or metrics are needed for this build-only change.
  • Persistence, transactions, data correctness, memory accounting, and FE/BE protocol: Not applicable to this thirdparty build-graph change.
  • Performance: No runtime performance impact was found; the dependency edges affect build scheduling only. No separate valuable performance issue survived review.
  • Other issues: Two complete review rounds, including separate risk-focused scans, found no additional valuable finding. Every candidate was accepted, dismissed with evidence, or deduplicated.

User focus

No additional user-provided review focus was specified; the complete PR was reviewed.

Comment thread thirdparty/patches/paimon-cpp-buildutils-static-deps.patch
@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review status: complete after 1 convergence round. Approval opinion: no blocking issue remains on the current head.

Critical checkpoint conclusions

  • Goal and proof: The PR now adds zstd, snappy, and lz4 to the object-library dependency fence. In the exact patched Paimon external-Arrow/static-only configuration, generated Ninja and Unix Make graphs both place all three codec ExternalProject install targets before every paimon_objlib object compile. This resolves the earlier LZ4 thread; no distinct unordered codec-header path remains.
  • Scope and clarity: The four-file change is small and focused: one build-graph correction, the corresponding semantic-fingerprint migration, and focused CI coverage.
  • Concurrency: The relevant concurrency is CMake/Ninja/Make scheduling. The imported codec targets transitively own zstd_ep, snappy_ep, and lz4_ep, so their install stamps complete before object compilation while the independent codec builds can still run in parallel. No runtime shared-state concurrency is involved.
  • Lifecycle: The source-patch fingerprint forces stale source trees to be reset and repatched, component markers are invalidated before rebuild and republished only after required artifacts exist, and the ExternalProject install lifecycle now precedes header consumers. No runtime, ownership, or static-initialization lifecycle is changed.
  • Configuration and compatibility: No Doris runtime configuration is added. The current Paimon semantic fingerprint computes exactly to 9887cf1ec13106da4250f8d7ea931a258879089c; legacy markers are admitted only at that selected-input fingerprint, artifact validation remains mandatory, and this ordering-only change does not alter installed archives or ABI, so no schema bump is required.
  • Parallel paths and conditions: External and internal Arrow paths, static-only and shared-capable object creation, direct and transitive codec-header users, Linux/macOS generators, workflow path filters, source rebuild, and prebuilt reuse were checked. The same object fence covers the affected paths.
  • Tests and results: bash -n, thirdparty/test/paimon-codec-dependency-test.sh, thirdparty/test/arrow-paimon-lifecycle-test.sh, exact-tag checksum verification, and full patch dry-run all pass locally. The live Arrow/Paimon Lifecycle Test and Thirdparty Script Test pass. The focused test is textual rather than graph-generating, but independent Ninja/Make graph inspection proves the current edge; full Linux and both macOS thirdparty builds plus general compile/BE checks were still running at submission.
  • Error handling and observability: Shell, patch, and fingerprint failures propagate with actionable messages. No runtime logs, metrics, or additional observability are warranted for this build-only change.
  • Persistence, transactions, data correctness, memory accounting, and FE/BE protocol: Not applicable; no runtime data or protocol behavior is modified.
  • Performance: No runtime performance impact was found. The added prerequisites only constrain the necessary build order and retain parallelism among independent codec builds.
  • Other issues: Two complete full-coverage aspects and a separate risk-focused pass all returned NO_NEW_VALUABLE_FINDINGS; the main review independently rechecked and closed every initial risk. No inline comment is warranted.

User focus

No additional user-provided review focus was specified; the complete PR was reviewed.

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review status: complete after 1 convergence round on 4113dadea5fe0a8b72a3e49ab7cc5b8d71311a39. Approval opinion: no blocking issue remains on the current head.

Critical checkpoint conclusions

  • Goal and proof: The Paimon change now places zstd, snappy, and lz4 on the object-library dependency fence. Configure-only inspection of the exact patched paimon-cpp-0a4f4e2 Ninja graphs proves that paimon_objlib waits for zstd_ep, snappy_ep, and lz4_ep in both the Doris external-Arrow mode and the bundled-Arrow mode. This resolves the existing LZ4 thread; no distinct unordered generated-header path remains.
  • Scope and clarity: The codec ordering change, semantic-fingerprint migration, and focused CI coverage are small and coherent. The current head also includes a separate but narrow external-regression ADBC configuration parity repair with its own focused test; its packaging and startup behavior were independently traced and found correct.
  • Concurrency: The relevant concurrency is clean parallel CMake/Ninja scheduling. Imported codec targets transitively own their ExternalProject install targets, so header installation completes before object compilation while the independent codec projects may still build concurrently. The new scripts use isolated temporary state. No runtime shared-memory concurrency or lock ordering is involved.
  • Lifecycle: Source fingerprinting resets stale patch states, Paimon component markers are invalidated before rebuild and published only after required artifacts exist, and the codec install lifecycle now precedes all direct header consumers. For ADBC, build.sh copies libadbc_driver_jni.so to output/fe/lib, and start_fe.sh exports and expands DORIS_HOME before passing the JDK-17 options to Java. No ownership, cleanup, or static-initialization defect was found.
  • Configuration and compatibility: The ADBC item is a startup JVM system property, not a dynamic Doris configuration; restart semantics are appropriate. The Paimon semantic fingerprint computes exactly to 9887cf1ec13106da4250f8d7ea931a258879089c. Legacy reuse remains limited by that exact selected-input pin, exact historical markers, independent component markers, Arrow version validation, and complete required-library checks. The ordering-only delta changes no installed archive, ABI, protocol, or storage format.
  • Parallel paths and conditions: External and bundled Arrow, static-only and shared-capable object creation, codec target propagation, Linux/macOS generators, workflow path filters, source rebuild, prebuilt reuse, packaged versus external FE configuration, and ADBC library fallback behavior were checked. The affected parallel paths are covered.
  • Tests and results: Local thirdparty/test/paimon-codec-dependency-test.sh, thirdparty/test/adbc-jni-config-test.sh, and thirdparty/test/arrow-paimon-lifecycle-test.sh all pass; shell syntax, workflow YAML parsing, and diff whitespace checks also pass. The live Arrow/Paimon Lifecycle Test and Thirdparty Script Test pass. The focused codec test is textual, but inspection of the real generated graphs provides the stronger ordering proof. Full Linux and both macOS thirdparty builds, plus general compile/BE/performance checks, were still running at submission.
  • Error handling and observability: Patch, fingerprint, configuration, and shell-test failures propagate with actionable diagnostics. Existing ADBC load errors identify the expected property/directory. No additional runtime metrics or logging are warranted for these build/configuration changes.
  • Persistence, transactions, data correctness, memory accounting, and FE/BE protocol: Not applicable; this head changes build ordering, prebuilt selection metadata, CI checks, and an external test-deployment JVM option, not runtime data or protocol semantics.
  • Performance and other issues: No runtime performance impact was found. The dependency fence constrains only the necessary build order and retains concurrency among independent codec builds. Two normal full-coverage reviews and a separate risk-focused review all returned NO_NEW_VALUABLE_FINDINGS; the main review independently validated and closed every initial risk. No inline comment is warranted.

User focus

No additional user-provided review focus was specified; the complete six-file PR diff was reviewed.

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 62.60% (29234/46698)
Line Coverage 47.60% (306003/642797)
Region Coverage 43.25% (247098/571289)
Branch Coverage 44.82% (115053/256679)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100% (0/0) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 65.35% (29557/45231)
Line Coverage 50.13% (318524/635452)
Region Coverage 45.95% (261526/569125)
Branch Coverage 47.15% (120680/255954)

@Gabriel39
Gabriel39 force-pushed the fix/paimon-zstd-build-dependency branch from 4113dad to e6f2cd0 Compare August 28, 2026 02:45
@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@hello-stephen

Copy link
Copy Markdown
Contributor

skip buildall

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review status: complete after 2 convergence rounds. One P2 test-guard issue remains, so I am requesting changes.

Critical checkpoint conclusions

  • Goal and proof: The Paimon change accomplishes its clean-parallel-build goal. In generated graphs from the exact patched paimon-cpp-0a4f4e2 source, paimon_objlib waits for zstd_ep, snappy_ep, and lz4_ep before object compilation. The added ADBC guard, however, does not always prove that the runtime-effective JDK-17 assignment retains the required JNI path; the inline comment covers that defect.
  • Scope and clarity: The codec ordering change, compatibility-marker migration, workflow wiring, external-FE comment, and two focused scripts are understandable and bounded. The remaining issue is isolated to the new ADBC guard.
  • Concurrency: The affected concurrency is CMake/Ninja scheduling. The common object-library fence covers external/internal Arrow and static/shared outputs without a reverse edge or dependency cycle. No runtime shared-state concurrency, locks, or atomics are changed.
  • Lifecycle: Codec ExternalProject install completion now precedes header consumers. Paimon markers are invalidated before rebuild and published only after install and required-artifact validation. For FE startup, start_fe.sh applies active configuration assignments sequentially, which is why the guard must validate the effective final value.
  • Configuration and compatibility: The current external JDK-17 option expands correctly to the deployed FE lib directory and is a restart-time JVM property, not a dynamic Doris setting. The Paimon semantic fingerprint computes to 9887cf1ec13106da4250f8d7ea931a258879089c; legacy reuse is restricted to that selected-input gate, enumerated markers, a matching Arrow component, and complete artifact closures. The ordering-only hunk changes no installed archive, ABI, protocol, or storage format.
  • Parallel paths and conditions: External and bundled Arrow, static/shared object reuse, codec target propagation, Linux/macOS workflow paths, source rebuild, prebuilt reuse, packaged versus external FE configuration, and the resolved LZ4 thread were checked. No additional distinct path issue survived review.
  • Tests and results: bash -n and all three focused scripts (arrow-paimon-lifecycle-test.sh, paimon-codec-dependency-test.sh, and adbc-jni-config-test.sh) pass locally; the live Arrow/Paimon Lifecycle Test and Thirdparty Script Test pass. A duplicate-assignment negative reproduction makes the ADBC guard pass while the runtime-effective value lacks the JNI override. Full Linux and both macOS thirdparty builds were still pending at submission; no builds were run in this review environment.
  • Error handling and observability: Patch, fingerprint, and script failures remain fail-fast with actionable diagnostics. No additional runtime logging or metrics are warranted for these build/configuration changes.
  • Persistence, transactions, data correctness, memory accounting, FE/BE protocol, and security: Not applicable; this PR changes build ordering, prebuilt-selection metadata, CI guards, and an external deployment comment rather than runtime data or protocol behavior.
  • Performance: No runtime cost was found. The dependency fence constrains only the necessary build order while independent codec projects may still run concurrently.
  • Other issues: Round 2's two normal full-review tracks and separate risk-focused track all returned NO_NEW_VALUABLE_FINDINGS. Every candidate is accepted, dismissed with evidence, or deduplicated; M-1 is the only inline comment.

User focus

No additional user-provided review focus was specified; the complete six-file PR diff was reviewed.


# External Regression replaces the packaged FE configuration instead of merging it.
# Keep the JNI override in that replacement so ADBC cannot fall back to the jar binary.
java_opts_assignment="$(grep '^JAVA_OPTS_FOR_JDK_17=' "${EXTERNAL_FE_CONF}" || true)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Validate the runtime-effective JDK-17 assignment

grep returns every active JAVA_OPTS_FOR_JDK_17 assignment, so an earlier correct line can satisfy this check even after a later definition overrides it without the JNI option. start_fe.sh exports these assignments sequentially, making the later value effective; with the current line followed by JAVA_OPTS_FOR_JDK_17="-Xmx4096m", this script still reports PASS while FE lacks the override and can fall back to the Maven-bundled JNI library. Please validate the effective final assignment (or reject duplicates) and add that later-override negative case.

@hello-stephen

Copy link
Copy Markdown
Contributor

skip buildall

@hello-stephen
hello-stephen merged commit 8026894 into apache:master Aug 28, 2026
39 of 40 checks passed
@u70b3

u70b3 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Hi @Gabriel39 — I believe this change inadvertently breaks Paimon prebuilt reuse on the current CI build image, which is now failing the COMPILE and performance pipelines of PRs merged with master.

Failure (before any compilation starts):

Paimon build fingerprint does not match selected inputs
Arrow/Paimon thirdparty libraries need to be rebuilt ...
Cannot rebuild thirdparty libraries: /var/local/thirdparty/build-thirdparty.sh is missing.
DORIS_THIRDPARTY=/var/local/thirdparty is an install-only or incomplete prefix.

Analysis:

  • On current master, paimon_build_fingerprint computes 9887cf1ec13106da4250f8d7ea931a258879089c, matching the updated PAIMON_LEGACY_COMPATIBLE_SEMANTIC_FINGERPRINT — that part is correct.
  • However, the prebuilt marker installed in the current build image (apache/doris:build-env-ldb-toolchain-latest, digest sha256:5d474521b72d…) is still the previous semantic fingerprint cb82e41ba46f534e611cdd52e66b53c227d49bf8, which is not in PAIMON_LEGACY_BUILD_FINGERPRINTS (2bbf52e7…, dbb6ca6e…), so paimon_prebuilt_valid now rejects it.
  • Pinpoint by elimination: the same image digest passed the same pipeline one day earlier (pre-[fix](thirdparty) order Paimon codec dependencies #67158 checkout: expected == cb82e41ba == installed marker, direct match), and fails today (installed ∉ {9887cf1ec, 2bbf52e7…, dbb6ca6e…}).

Suggested fix: since the dependency-ordering change does not alter the installed artifacts (as the commit message notes), the old semantic marker should remain accepted — add cb82e41ba46f534e611cdd52e66b53c227d49bf8 to PAIMON_LEGACY_BUILD_FINGERPRINTS (with a comment noting it is a semantic marker), or republish the prebuilt stamped with the new semantic fingerprint and roll it into the build image.

Example failing builds on TeamCity: COMPILE 1034233, performance 1034228 (both on a PR merged with master after this change). Happy to send the one-line PR if that helps.

Update (2026-08-29): the one-line fix is now up as #67305. Meanwhile, two manual retries of the COMPILE/performance pipelines on another PR (builds 1034257, 1034249, same image digest) failed at this same fingerprint check, so the issue is currently blocking all PRs merged with master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants