[fix](thirdparty) order Paimon codec dependencies - #67158
Open
Gabriel39 wants to merge 2 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
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_eppredecessor. 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_objlibwaits forzstd_epandsnappy_epbut notlz4_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
lz4to this dependency fence is sufficient because the importedlz4target already ownsadd_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'sPAIMON_USE_EXTERNAL_ARROW=ONpath 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
zstdandsnappyin 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.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
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, andlz4to 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 everypaimon_objlibobject 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, andlz4_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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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?
Check List
Release note
None.