[chore](thirdparty) introduce thridparty about lance-c - #65304
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Gabriel39
previously approved these changes
Jul 7, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
Author
|
run buildall |
zhangstar333
force-pushed
the
rust_thridpaty
branch
from
July 14, 2026 03:08
02c2e05 to
a4f0ee5
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found several blocking build/distribution issues in this third-party integration.
Checkpoint conclusions:
- Goal: add lance-c as a Doris third-party dependency. The source-build path installs the expected lib/header when the right toolchain and network are available, but the PR does not fully satisfy Doris' third-party reproducibility and distribution requirements.
- Scope/focus: the code changes are narrow, but some parallel build-image and prebuilt-thirdparty paths were not updated.
- Concurrency/lifecycle/config/storage/transaction/FE-BE protocol: not applicable; this PR only changes third-party build and license metadata.
- Compatibility/parallel paths: issues were found in offline/mirrored source builds, stale prebuilt third-party acceptance, and ARM/legacy GCC Docker image paths.
- Testing/validation: no builds/tests were run in this review-only runner. Static review found missing coverage that CI may not catch until the affected source-build/image paths run.
- User focus: no additional user-provided review focus was present.
After three review rounds, all subagents converged with no additional valuable findings beyond the inline comments below.
Contributor
Author
|
run buildall |
zhangstar333
force-pushed
the
rust_thridpaty
branch
from
July 15, 2026 12:49
a4f0ee5 to
3e0d624
Compare
Contributor
Author
|
run buildall |
github-actions
Bot
removed
the
approved
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: the Lance-C integration still has three blocking build/distribution issues.
Blocking findings:
- The new global
-lmordering bypasses Doris's default Linux glibc-compatibility interposition. - The non-Darwin
-std=gnu17CFLAGS breaks the intended GCC 7.3 third-party path even after Cargo is provisioned. - Required Apache DataFusion and Apache Arrow Object Store NOTICE attributions are absent from the packaged NOTICE material.
Checkpoint conclusions:
- Goal and proof: the patch downloads, source-builds, installs, links, and inventories Lance-C 0.1.2. The source checksum matched, both changed shell files parsed, and the hosted Linux, macOS, and macOS-arm64 third-party source-build checks passed. There is still no Doris consumer-link test, post-Cargo GCC 7 coverage, or target-complete license/NOTICE validation.
- Scope and minimality: the six changed files are the expected build/link/license surfaces, but the unconditional global
mis not a safe focused integration. Existing discussions already cover target-specific license closure/texts, offline Cargo dependencies, stale prebuilts, and missing Cargo in ARM/legacy builders; those issues were not duplicated here. - Compatibility and parallel paths: default and explicit builds, prebuilts, Linux/macOS, x86/ARM, legacy/current GCC, cleanup, stripping, product packaging, and license/NOTICE paths were checked. The concrete additional failures are the Linux old-glibc link ordering and GCC 7 native-C flag issues described inline. No storage, protocol, or function-format compatibility change applies.
- Concurrency, lifecycle, runtime configuration, transactions, persistence, data writes, and FE/BE variable propagation: not applicable to these CMake/shell/license-only changes.
- Error handling and observability: Cargo/protoc/version checks fail loudly and build command output is sufficient for this build-time path; no runtime logging or metrics surface is introduced.
- Performance: making Lance-C a default dependency adds substantial source-build time and archive size, but the available jobs completed and no separate correctness threshold was demonstrated.
- Validation limits: no local build or unit test was run because the review contract prohibits builds and this checkout lacks
thirdparty/installedand itsprotoc. The macOS BE UT check failed during unrelated JDK 25 versus required JDK 17 setup; TeamCity COMPILE and BE UT were still pending at submission. - User focus: no additional user-provided review focus was present.
After the final maximum-round review, all three subagents returned NO_NEW_VALUABLE_FINDINGS for this exact three-comment set.
| # liblance_c.a contains compiler_builtins cbrt symbols. Place libm before it | ||
| # so the final linker resolves C math symbols from the system library first. | ||
| add_thirdparty(lance_c LIB64 NOTADD) | ||
| list(APPEND COMMON_THIRDPARTY m lance_c) |
Contributor
There was a problem hiding this comment.
COMMON_THIRDPARTY is linked before glibc-compatibility, so adding bare m here puts system -lm ahead of the compatibility archive on every default Linux link. That contradicts the explicit ordering invariant in be/CMakeLists.txt requiring glibc-compatibility before -lc -lm: existing Doris pow/log/exp references can now be satisfied by the build host's libm before the corresponding compatibility objects are extracted, reintroducing newer libm symbol versions into binaries intended for older glibc. Please keep every -lm after the compatibility targets and solve Lance's compiler-builtins collision without changing the global system-library order.
| fi | ||
|
|
||
| if [[ "${KERNEL}" != 'Darwin' ]]; then | ||
| cargo_env+=("CFLAGS=${CFLAGS:-} -std=gnu17") |
Contributor
There was a problem hiding this comment.
This CFLAGS value is consumed by cc-rs native dependencies; the locked lz4-sys crate, for example, compiles four C sources through cc::Build. The GCC 7 builder installs GCC 7.3 and now default-builds Lance-C, but GCC documents -std=gnu17 as a new option in GCC 8, so that path still fails after Rust/Cargo is provisioned (the separate issue already raised in discussion r3578107999). Please gate this flag by compiler support, use a compatible C mode, or select a newer C compiler for Lance-C, and cover the intended legacy-toolchain build.
| * opentelemetry-proto: 0.18.0 | ||
| * opentelemetry-cpp: 1.4.0 | ||
| * clucene: 2.4.6 | ||
| * lance-c: 0.1.2 |
Contributor
There was a problem hiding this comment.
The new static archive includes Apache DataFusion 52.4.0 and Apache Arrow Object Store 0.12.5 (both are present in the locked inventory), and each published crate carries a NOTICE.txt attribution. Apache-2.0 section 4(d) requires those relevant attributions to accompany redistributed derivatives, but neither NOTICE.txt nor dist/NOTICE-dist.txt mentions either project; build.sh packages the former with Doris binaries. Please add these NOTICE entries and audit the remaining locked crates for any additional NOTICE files before distributing liblance_c.a.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
approved these changes
Jul 17, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
### What problem does this PR solve? introduce thridparty about lance-c https://github.com/lance-format/lance-c
yiguolei
pushed a commit
that referenced
this pull request
Jul 31, 2026
wyxxxcat
pushed a commit
to wyxxxcat/doris
that referenced
this pull request
Aug 17, 2026
### What problem does this PR solve? introduce thridparty about lance-c https://github.com/lance-format/lance-c
HappenLee
pushed a commit
that referenced
this pull request
Sep 2, 2026
) Problem Summary: PR #65304 added the following libraries to `COMMON_THIRDPARTY`: ```cmake list(APPEND COMMON_THIRDPARTY m lance_c) ``` As a result, some math symbols are resolved by the system `libm` before the linker scans Doris' optimized compatibility implementations, causing a noticeable performance regression for functions such as `exp`, `log`, and `pow`: - exp: approximately 220% - ln: approximately 167% - log: approximately 197% - log2: approximately 165% Simply moving `-lm` and `lance_c` after `glibc-compatibility` exposes another issue. Once Lance is actually referenced, `liblance_c.a` pulls in Rust std objects containing late unresolved references to: - `posix_spawnp` - `posix_spawn_file_actions_init` - `posix_spawn_file_actions_destroy` - `posix_spawn_file_actions_adddup2` - `preadv` - `splice` The LDB toolchain resolves these references by extracting its monolithic `glibc-compatibility.c.o`, which also defines `getrandom`. This conflicts with Doris' explicitly linked `getrandom.c.o` and causes a duplicate-symbol error. #### Solution 1. Restore the required link order For `GLIBC_COMPATIBILITY=ON`, use the following order: ```text glibc-compatibility-explicit → glibc-compatibility → -lm → lance_c ``` This ordering ensures that: - Doris' optimized math implementations are scanned before `-lm`. - `-lm` is scanned before `lance_c`, preventing Rust compiler_builtins symbols such as cbrt from being selected. - `lance_c` remains available to its consumers. 2. Resolve the late Lance/Rust libc references Add a small set of hidden symbol-version adapters for the six strong libc references introduced by the Rust std object. Each adapter has two parts. For example, the `splice` adapter is conceptually: ```c extern __typeof__(splice) __doris_old_splice; __asm__( ".symver __doris_old_splice," "splice@GLIBC_2.5" ); __attribute__((visibility("hidden"))) ssize_t splice(...) { return __doris_old_splice(...); } ``` The hidden `splice` wrapper is included in the `glibc-compatibility-explicit` OBJECT target, so its symbol is visible to the linker before `liblance_c.a` is processed. #### performance ```text Doris> select count(exp(db)) from double_ranges; +----------------+ | count(exp(db)) | +----------------+ | 50000000 | +----------------+ 1 row in set (0.980 sec) Doris> select count(exp(db)) from double_ranges; +----------------+ | count(exp(db)) | +----------------+ | 50000000 | +----------------+ 1 row in set (0.347 sec) Doris> select count(ln(db)), count(ln(in_one)) from double_ranges; +---------------+-------------------+ | count(ln(db)) | count(ln(in_one)) | +---------------+-------------------+ | 50000000 | 50000000 | +---------------+-------------------+ 1 row in set (1.486 sec) Doris> select count(ln(db)), count(ln(in_one)) from double_ranges; +---------------+-------------------+ | count(ln(db)) | count(ln(in_one)) | +---------------+-------------------+ | 50000000 | 50000000 | +---------------+-------------------+ 1 row in set (0.607 sec) Doris> select count(log(db, db)), count(log(in_one, db)), count(log(db, in_one)), count(log(db, in_ten)) from double_ranges; +--------------------+------------------------+------------------------+------------------------+ | count(log(db, db)) | count(log(in_one, db)) | count(log(db, in_one)) | count(log(db, in_ten)) | +--------------------+------------------------+------------------------+------------------------+ | 50000000 | 49999990 | 50000000 | 50000000 | +--------------------+------------------------+------------------------+------------------------+ 1 row in set (5.944 sec) Doris> select count(log(db, db)), count(log(in_one, db)), count(log(db, in_one)), count(log(db, in_ten)) from double_ranges; +--------------------+------------------------+------------------------+------------------------+ | count(log(db, db)) | count(log(in_one, db)) | count(log(db, in_one)) | count(log(db, in_ten)) | +--------------------+------------------------+------------------------+------------------------+ | 50000000 | 49999990 | 50000000 | 50000000 | +--------------------+------------------------+------------------------+------------------------+ 1 row in set (2.356 sec) Doris> select count(log2(db)), count(log2(in_one)) from double_ranges; +-----------------+---------------------+ | count(log2(db)) | count(log2(in_one)) | +-----------------+---------------------+ | 50000000 | 50000000 | +-----------------+---------------------+ 1 row in set (1.616 sec) Doris> select count(log2(db)), count(log2(in_one)) from double_ranges; +-----------------+---------------------+ | count(log2(db)) | count(log2(in_one)) | +-----------------+---------------------+ | 50000000 | 50000000 | +-----------------+---------------------+ 1 row in set (0.655 sec) ```
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
4 participants
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?
introduce thridparty about lance-c https://github.com/lance-format/lance-c
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)