Repository navigation
[fix](be) Make the BE unit tests build and run on macOS - #68520
Conversation
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
run-be-ut.sh initializes only three of the five submodules that build.sh
initializes: contrib/datasketches-cpp, contrib/apache-orc and
contrib/clucene. contrib/faiss and contrib/openblas are left out, but both
are required before a single file can be compiled: be/CMakeLists.txt adds
be/src/storage/index/ann unconditionally, and that directory's
cmake-protect/CMakeLists.txt calls add_subdirectory() on contrib/openblas
and contrib/faiss.
On a tree where those two submodules are not checked out yet -- a fresh
clone, or a new git worktree, which is the normal way to start work on this
repository -- run-be-ut.sh therefore dies in the configure step:
CMake Error at src/storage/index/ann/cmake-protect/CMakeLists.txt:48 (add_subdirectory):
The source directory .../contrib/openblas does not contain a CMakeLists.txt file.
CMake Error at src/storage/index/ann/cmake-protect/CMakeLists.txt:66 (add_subdirectory):
The source directory .../contrib/faiss does not contain a CMakeLists.txt file.
-- Configuring incomplete, errors occurred!
Running build.sh once beforehand masks the problem, because build.sh does
initialize those two, which is why it is easy to miss.
Fix: add the two update_submodule calls, using the same archive URLs that
build.sh already passes.
### Release note
None
### Check List (For Author)
- Test: Manual test
- Reproduced the configure failure on a tree with both submodules
uninitialized, ran `git submodule update --init contrib/faiss
contrib/openblas` by hand (the exact command the added lines run), and
configure then completed and the build proceeded.
- Behavior changed: No
- Does this need documentation: No
Co-Authored-By: Claude Code <[email protected]>
…macOS
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
Seven be/test translation units fail to compile on macOS while the Linux BE UT
build stays green, so `run-be-ut.sh` on a Mac stops at the first of them and a
Mac developer cannot run the backend unit tests at all. run-be-ut.sh runs ninja
without -k, so the developer fixes one, waits for another full compile, and
hits the next one: seven round trips. None of the seven is related to a local
change -- a clean master reproduces all of them; the last one was introduced by
a commit merged after the previous sweep, which is exactly the case that only a
real build catches.
Root causes, one per file:
1. exprs/function/function_map_inner_product_test.cpp
`fatal error: 'faiss/impl/platform_macros.h' file not found`.
be/test/CMakeLists.txt links vector_search_test on APPLE with
`-Wl,-force_load,$<TARGET_FILE:vector_search_test>`, which is a file path:
it carries no usage requirements, so the faiss include directories that
vector_search_test exports PUBLIC never reach this TU. The other platforms
link `-Wl,--whole-archive vector_search_test`, which does propagate them.
Fix: link ann_index explicitly in the APPLE branch as well. It is the target
that exports the faiss include directories, and the resulting link line then
matches what the other platforms already produce.
2. exprs/function/function_format_round_test.cpp
`ambiguous conversion for functional-style cast from 'long' to 'Decimal64'
(aka 'Decimal<long long>')`. Decimal64 is Decimal<int64_t>, so on Darwin the
constructor parameter is `long long` while these decimal literals are `long`,
leaving every integral and floating constructor an equally ranked conversion
away. Fix: wrap the literals in int64_t(), the spelling already used by
storage/key_coder_test.cpp and runtime/jsonb_document_test.cpp.
3. storage/index/snii/bench/bkd_native_vs_clucene_bench_test.cpp,
six -Werror,-Wformat failures: uint64_t is `unsigned long long` on Darwin
but `unsigned long` on Linux, so `%lu`/`%ld` is right on one and wrong on
the other. Fix: print with PRIu64/PRId64.
4. storage/index/snii/bench/snii_vs_v3_benchmark_test.cpp
- mincore(2) takes `char*` on Darwin and `unsigned char*` on Linux.
- POSIX_FADV_DONTNEED / posix_fadvise do not exist on Darwin.
Fix: one element type for the mincore buffer, and guard the posix_fadvise
call.
5. storage/index/snii/bkd/bkd_corruption_test.cpp
`deduced conflicting types ('size_type' (aka 'unsigned long') vs
'uint64_t' (aka 'unsigned long long'))`: an initializer list mixing
payload.size() (size_t) with uint64_t literals. Fix: hoist the size into a
uint64_t first.
6. storage/index/snii/writer/spimi_locality_bench_test.cpp
libc++ reports `no matching function for call to object of type
'key_equal'`: the transparent equality functor only defined one argument
order, and libc++ calls it as (key, lookup key) while libstdc++ calls it as
(lookup key, key). The other order is added, and both are marked
[[maybe_unused]] because either standard library leaves one of the two
unused, which -Wunused-member-function (clang-only, and the Linux BE UT
build also uses clang) turns into an error.
7. storage/segment/hierarchical_data_iterator_test.cpp
`no matching function for call to 'min'`: kRows is size_t while
current_ordinal is ordinal_t (uint64_t), distinct types on Darwin. Fix:
narrow the remaining row count before the min() call.
On the other platforms the equivalent link line already puts vector_search_test
on the link list as a target, so every test TU is compiled with the faiss
include directories there; the APPLE branch now produces the same compile and
link inputs as the rest. The Linux build is otherwise unaffected: every other
change is either an APPLE-only path or a strictly more portable spelling of the
same value.
Note for macOS developers: because the APPLE link line now propagates include
directories, the compile command of every be/test translation unit changes
once, so the first build after this lands rebuilds be/test (~1230 TUs). Later
builds are incremental again.
### Release note
None
### Check List (For Author)
- Test: Manual test
- Each of the seven translation units was compiled on macOS with the exact
command line taken from the build's own compile_commands.json
(`clang++ -fsyntax-only ...`), not with a hand-written flag set.
- Full macOS BE UT build: `BUILD_TYPE_UT=Debug bash run-be-ut.sh`, 1231
translation units, 0 failures, doris_be_test linked.
- Tests run on macOS: `run-be-ut.sh --run` with a filter over the affected
suites -- 43 tests from 5 suites ran, all passed
(FormatRoundTest 8, FunctionMapInnerProductTest 11, BkdCorruptionTest 16,
SniiSpimiLocalityBenchTest 3, HierarchicalDataIteratorTest 5). The two
bench files contain only DISABLED_ cases, so for them compilation is the
whole check.
- The remaining platform-only differences (libstdc++ vs libc++ key_equal
call order, PRIu64 spelling) were verified against the compile flags of
the Linux build rather than by building on Linux.
- Behavior changed: No
- Does this need documentation: No
Co-Authored-By: Claude Code <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run beut |
|
/review |
There was a problem hiding this comment.
Static review conclusion: request changes for one benchmark-correctness issue.
The clean-worktree bootstrap and the other macOS compile fixes are otherwise sound. The direct ann_index edge restores the required faiss/OpenMP usage requirements; the new submodule calls match build.sh; and the Decimal, format-macro, mincore, heterogeneous-lookup, corruption-length, and iterator-type edits preserve their values and contracts.
Critical checkpoints:
- Goal and test proof: the PR largely makes the BE unit-test build portable, but the changed Apple cache path can publish warm local benchmark samples as cold. The author reports a full 1,231-TU macOS build plus 43 affected tests; this automated review was required to remain static, so those results were not independently executed here. The disabled SNII benchmark was compile-checked only and therefore did not test this semantic path.
- Scope, reuse, and parallel paths: the changes are focused.
run-be-ut.shnow matches the relevantbuild.shsubmodule setup, and Apple/non-Apple CMake linkage and clean checkout/fallback paths were traced end to end. - Concurrency, lifecycle/static initialization, configuration, persistence/transactions, data writes, and FE-BE protocol compatibility: not involved by this test/build-only change.
- Compatibility and error handling: the supported 64-bit ABI conversions and platform API signatures are correct. No storage, serialization, or rolling-upgrade format changes are introduced. The remaining conditional is the reported eviction gap.
- Performance and observability: there is no production hot-path change. For benchmark validity, logging one file's residency without gating aggregation is insufficient when eviction is unavailable.
- Tests and expected results: no generated result files change. The author-provided macOS build/test evidence covers compilation and ordinary affected suites, but not the disabled cold-cache benchmark semantics.
- User focus: no additional focus points were supplied.
The review converged after two full normal/risk-focused rounds. All other candidates were dismissed with code evidence or duplicate-fenced.
| } | ||
| ::fsync(fd); | ||
| // posix_fadvise is Linux-only; elsewhere the fsync above is all this can do. | ||
| #if defined(POSIX_FADV_DONTNEED) |
There was a problem hiding this comment.
[P2] Do not report fsync-only samples as cold on macOS
On Apple this guard removes the only page-cache eviction operation. fsync(fd) writes dirty data but does not discard clean file pages, so _drop_caches() can leave the just-built/compacted rowset resident. The later _resident_pages() probe only prints one file's retained count and never fails or skips, and these samples are still aggregated under cold_query_p50/p99. That makes the local benchmark publish warm or partially warm results as cold on the platform this hunk enables. Please either evict the complete local query working set with a Darwin-supported mechanism, or fail/skip/unlabel the local cold phase before its samples are aggregated.
|
skip buildall |
### What problem does this PR solve? Issue Number: N/A Problem Summary: When BE starts the CDC client, `CdcClientMgr` installs a process-wide SIGCHLD handler. The handler previously called `waitpid(-1, ..., WNOHANG)`, which could reap any child of BE. The embedded JVM also starts subprocesses and waits for their exit codes. If the CDC handler reaps one first, the JVM can observe `ECHILD` and report exit code 0 even when the command failed. One observed result was a failed libc-detection probe appearing successful, causing RocksDB JNI to select the musl library on a glibc host. The handler now waits only for the CDC child. A generation-qualified identity and an exclusive operation claim coordinate it with startup, health inspection, and shutdown, so those paths cannot act on a PID after it has been reaped and reused. A pending-reap handoff preserves SIGCHLD notifications arriving during inspection. The latest review identified another handoff window: an old handler could read generation N before claiming, then claim N after N+1 was published and hide N+1's only exit notification. The handler now rechecks the published generation after releasing that stale claim. A deterministic test pauses at both sides of this window and verifies that N+1 is reaped. The affected tests now fail if their process setup fails, observe SIGTERM through child-reported markers and phase handshakes instead of `kill(pid, 0)`, and reap an unrelated direct child even when a fatal handler checkpoint fails. The forced-shutdown test uses a direct child that ignores SIGTERM; the handler/shutdown test waits for an actual failed ownership claim; and the stale-generation helper starts with a controlled signal environment. The ownership rule changes what a failed startup does to its forked child. The health-check failure path now terminates and reaps it, where the PID used to be dropped and the child left running unmanaged. That abandoned child could finish warming up and be adopted by a later `start_cdc_client` call. On a host whose cold `cdc-client.jar` JVM consistently answers `/actuator/health` later than the unchanged startup window, pre-start the CDC client so the adoption path finds it, or give the JVM more room. The cleanup also removes the unmanaged JVM left holding `cdc_client_port`. The macOS BE test compatibility changes, including the Decimal64 literal fix, were merged separately in #68520. This branch was rebased onto that merge and its duplicate test-only commit was dropped.
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
Context.
run-be-ut.shis the only entry point for the BE unit tests, and on macOS it is also the only way to run them: theBE UT (macOS)workflow runsbuild.sh --be, which configures with-DMAKE_TEST=OFF, so nothing underbe/testis compiled there (the workflow says so in its own comment at.github/workflows/be-ut-mac.yml:123). On a clean worktree the script therefore fails twice before a single test executes, and then stops seven more times while compilingbe/test.1. The problem, and what it cost
The script never initializes two of the submodules it needs.
run-be-ut.shinitializes three submodules (contrib/datasketches-cpp,contrib/apache-orc,contrib/clucene);build.shinitializes five, and the two it adds —contrib/faissandcontrib/openblas— are required before a single file is compiled.be/CMakeLists.txt:1038addsbe/src/storage/index/annunconditionally, and that directory'scmake-protect/CMakeLists.txt:48,66callsadd_subdirectory()on both. On a fresh clone or a new git worktree the configure step therefore ends withRunning
build.shonce first hides it, which is why it goes unnoticed.Seven
be/testtranslation units do not compile on macOS. The Linux BE UT build is green for all seven.run-be-ut.shinvokes ninja without-k, so the developer fixes one file, waits out another full compile, and meets the next one — seven round trips, each of them tens of minutes, for a task whose whole point is to run a test.exprs/function/function_map_inner_product_test.cpp'faiss/impl/platform_macros.h' file not foundAPPLEbranch ofbe/test/CMakeLists.txt:170linksvector_search_testas-Wl,-force_load,$<TARGET_FILE:vector_search_test>: a file path, which carries no usage requirements. Every other platform links-Wl,--whole-archive vector_search_test, which is a target in the link list and does propagate them.exprs/function/function_format_round_test.cppambiguous conversion for functional-style cast from 'long' to 'Decimal64'(11 sites)Decimal64isDecimal<int64_t>, so on Darwin the constructor parameter islong longwhile these literals arelong; every integral and floating constructor is then an equally ranked conversion away.storage/index/snii/bench/bkd_native_vs_clucene_bench_test.cpp-Werror,-Wformatuint64_tisunsigned long longon Darwin andunsigned longon Linux, so%lu/%ldis right on one and wrong on the other.storage/index/snii/bench/snii_vs_v3_benchmark_test.cppno member named 'posix_fadvise',mincoreargument typeposix_fadvise/POSIX_FADV_DONTNEEDdo not exist on Darwin;mincore(2)takeschar*there andunsigned char*on Linux.storage/index/snii/bkd/bkd_corruption_test.cppdeduced conflicting types ('size_type' vs 'uint64_t')payload.size()(size_t) withuint64_tliterals.storage/index/snii/writer/spimi_locality_bench_test.cppno matching function for call to object of type 'key_equal'key_equalas(key, lookup key), libstdc++ as(lookup key, key).storage/segment/hierarchical_data_iterator_test.cppno matching function for call to 'min'kRowsissize_tandcurrent_ordinalisordinal_t(uint64_t), which are distinct types on Darwin, sostd::mincannot deduce a common type.None of the seven is related to a local change; a clean master reproduces all of them. #2 arrived on 2026-09-24 with #68397 (
[fix](be) Preserve decimal precision in format_round, by TengJianPing), which is the commit that addedfunction_format_round_test.cpp— after the last full macOS sweep. So this class of breakage is still being produced: "green on Linux, red on macOS" is invisible to every CI job we have, and the two obstacles above make the one manual path that would find it expensive enough that nobody walks it.2. What this PR does, and why it helps
run-be-ut.sh: theupdate_submodulelist gainscontrib/openblasandcontrib/faiss, with the same archive URLsbuild.sh:941already uses, plus a comment that the list has to stay in sync withbuild.sh.be/test/CMakeLists.txt: theAPPLEbranch linksann_indexin addition to theforce_loadflag.ann_indexis the target that exports the faiss include directoriesPUBLIC, so the APPLE link line now feeds the same compile and link inputs as every other platform, instead of being a macOS special case.PRIu64/PRId64(core/decimal12.h,util/histogram.cpp),int64_t(...)around the literals (storage/key_coder_test.cpp:392,runtime/jsonb_document_test.cpp:433),#if defined(POSIX_FADV_DONTNEED)around the call.Linux behaviour is unchanged: every edit is either inside an
APPLE-only path or a strictly more portable spelling of the same value. On macOSrun-be-ut.shbecomes a usable test line for the first time.One cost worth naming: because the APPLE link line now propagates include directories, the compile command of every
be/testTU changes once, so the first macOS build after this lands rebuildsbe/test(about 1230 TUs). Later builds are incremental again.Branch pick. #68397 was never picked to
branch-4.1, sofunction_format_round_test.cppdoes not exist there. Of the seven files, onlystorage/segment/hierarchical_data_iterator_test.cppexists onbranch-4.1, with the same threestd::min(*rows, ROWS - current_ordinal)sites and the same types (ROWSissize_t,ordinal_tisuint64_t), so it fails to compile on macOS there too; that branch also has noAPPLEforce_loadbranch inbe/test/CMakeLists.txt, so the faiss part is master-only. If 4.1 is meant to run BE UT on macOS, the pick is that one file's hunk.3. The classes, and how they call each other
run-be-ut.sh(changed):update_submodule()callsgit submodule update --init --recursive <path>and falls back to the branch tarball; the two added lines are identical in shape to the three that were already there.be/test/CMakeLists.txt(changed): theAPPLEbranch of thedoris_be_testlink step.be/src/storage/index/ann/cmake-protect/CMakeLists.txt(unchanged, the reason the submodules are mandatory):add_subdirectory()oncontrib/openblasandcontrib/faiss, reached unconditionally frombe/CMakeLists.txt:1038.ann_index(unchanged target inbe/src/storage/index/ann): linksfaissPUBLIC, so itsINTERFACE_INCLUDE_DIRECTORIEScarry the faiss include path.vector_search_test(unchanged target inbe/test/storage/index/ann): linksann_indexPUBLIC; on APPLE it reachesdoris_be_testas forced-in objects rather than as a target.Release note
None
Check List (For Author)
Test
Build.
BUILD_TYPE_UT=Debug bash run-be-ut.shon macOS 26.5 / arm64 / clang 20 / libc++: 1231 translation units, 0 failures,doris_be_testlinked (340 MB after thedsymutil+llvm-strippost-build step). Before this PR the same command stops at configure, and aftercontrib/{faiss,openblas}are initialized by hand it stops at the first line of the table above and at six more after that.Tests.
run-be-ut.sh --runwith a filter over the affected suites: 43 tests from 5 suites ran, all passed —FormatRoundTest8,FunctionMapInnerProductTest11,BkdCorruptionTest16,SniiSpimiLocalityBenchTest3 (itsInternSetReserveUpperBoundis the case that exercises thekey_equalfix, and it still assertsintern.size() == distinct),HierarchicalDataIteratorTest5. The two bench files containDISABLED_cases only, so compilation is the whole check for them.Per-file compile check. Each of the seven TUs was also compiled on its own with the exact command line from the build's own
compile_commands.json(clang++ -fsyntax-only ...), which is how the seventh was found without a full rebuild.Not run: Linux. The portability of each edit was argued from the Linux compile flags (libstdc++'s
key_equalcall order, LP64PRIu64) rather than from a Linux build; the CI BE UT job covers that side.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)