Skip to content

DO NOT LAND fix: Bug fixes for macOS arm64 unit test failures (reference, do not merge) - #18615

Draft
karthikeyann wants to merge 25 commits into
facebookincubator:mainfrom
karthikeyann:fix/macos-arm64-test-failures
Draft

DO NOT LAND fix: Bug fixes for macOS arm64 unit test failures (reference, do not merge)#18615
karthikeyann wants to merge 25 commits into
facebookincubator:mainfrom
karthikeyann:fix/macos-arm64-test-failures

Conversation

@karthikeyann

@karthikeyann karthikeyann commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Important

DO NOT MERGE. This PR is intended to serve as a reference for these bug fixes.
There are currently no consumers of the macOS build, so it is not proposed for merging as-is.
Individual fixes can be cherry-picked into their own PRs if wanted.

Summary

All failures below were found by building Velox and running the full unit test suite on a
Mac M3 Pro (macOS, arm64, Apple Clang, Release). Every fix in this PR is verified:
the suite goes from 98 failing tests to 0.

100% tests passed, 0 tests failed out of 714

Base commit: 38d78c605b. Intermittent fixes were additionally stress-tested (details in the
tables below).

macOS unit tests have never run in CI — .github/workflows/macos.yml gates the test step
behind if: false, so only the build is validated. That is why these defects went unnoticed.

Most of these are not macOS-specific bugs. They are latent defects — undefined behaviour,
reliance on implementation-defined behaviour, and lifetime errors — that x86/Linux happens to
tolerate. Several would misbehave anywhere given a different standard library or allocator.

Bugs and root causes

Product defects

# Area Symptom Root cause
1 type/Type.cpp Interval printed as 0 00:00:45.-1982811744 int64_t and int128_t passed to "%d" conversions of snprintf. Variadic format specifiers are unchecked, and AArch64 lays out the variadic argument area differently, so the wrong bytes were read. Undefined behaviour.
2 vector/FlatMapVector.cpp Wrong values returned for a repeated map key Duplicate keys were resolved by taking the first element from std::unordered_multimap::equal_range(), whose order for equivalent elements is unspecified. libstdc++ happened to yield last-inserted-first.
3 common/memory/MemoryAllocator.h Per-size-class stats reported wrong timings SizeClassStats::operator- assigned to result.allocateClocks twice; result.freeClocks was never set.
4 common/memory/MemoryArbitrator.h Error messages and logs varied between runs/platforms Config::toString() iterated an unordered_map, so emitted order depended on the hash implementation.
5 functions/lib/KllSketch.h approx_percentile returned different results for the same seed std::default_random_engine is an implementation-defined alias (minstd_rand0 in libstdc++, minstd_rand in libc++), breaking the reproducibility the fixed-seed config promises.
6 common/caching/StringIdMap.cpp Silent memory corruption in Release release() guarded an erase with VELOX_DCHECK, which compiles to nothing under NDEBUG → erase(end()), undefined behaviour.
7 dwio/dwrf/test/CacheInputTest.cpp SIGSEGV in AsyncDataCacheEntry::setExclusiveToShared() Teardown shut the cache down before joining the IO executor, so in-flight prefetches touched a destroyed cache.
8 exec/benchmarks/RowContainerSortBenchmark.cpp SIGSEGV asFlatVector<StringView>() returns null for a non-flat column; and each StringView was built over a temporary std::string (use-after-free).
9 functions/sparksql/benchmarks/CastBenchmark.cpp SIGSEGV at process exit Benchmark lambda captured RowVectorPtr/ExprSet by value; folly holds callbacks in a global destroyed after main(), so buffers were freed to an already-destroyed memory pool.

Build / packaging

# Area Symptom Root cause
10 s2geometry + folly SIGBUS in S2FunctionsTest.cells s2 declares nallocx as a function with a weak default. On Mach-O a weak symbol cannot be left undefined, so folly builds with FOLLY_HAVE_WEAK_SYMBOLS=0 and defines size_t (*nallocx)(size_t,int) = nullptr — a data symbol — which wins at link time. s2's call jumps into __DATA.
11 scripts/setup-common.sh HDFS tests could not load libhdfs on aarch64 The generic Hadoop tarball ships x86-64 native libraries.

Tests asserting non-portable behaviour

# Test Root cause
12 ProbabilityTest Exact EXPECT_EQ on boost transcendental results (differ by 1–8 ULP across libm). Two cases use denormal parameters where Apple's libm is genuinely less accurate.
13 TimestampTest Relied on timegm() setting errno (macOS does not); macOS also supports a narrower range of negative years.
14 GeometryFunctionsTest Compared ST_Buffer WKT as exact text; one coordinate differed by 1 ULP.
15 SetDigestTest Fixed seed, but drew through std::uniform_*_distribution, whose mapping is implementation-defined.
16 QuantileDigestTest Genuine test bug: built digestWeighted but added to and asserted on digest.
17 ArrayShuffleTest Pinned one standard library's std::shuffle permutation.
18 ArithmeticTest (spark) Exact equality on expm1.
19 SkewedPartitionBalancerTest Fuzz test could call rebalance() with zero rows, violating a contract another test asserts.
20 KllSketchTest Memory bound pinned one allocator's exact byte total.
21 MemoryAllocatorTest Assumed sub-tick timer resolution; ARM's counter is far coarser than the x86 TSC.
22 TableScanTest Assumed randomly generated data contained no nulls; the filter drops null rows.
23 MultiFragmentTest Waited for kAborted, but a task can legitimately reach terminal kFinished first.
24 DynamicLinkTest Expected glibc's exact dlerror() wording.

Fixes

Product defects

# Fix
1 Format through fmt::format — type-safe by construction, removing the whole class of defect (not just this call site).
2 Select the largest matching channel explicitly, and document the duplicate-key contract in FlatMapVector.h.
3 Assign result.freeClocks. Added a unit test that fails without the fix.
4 Emit extra configs sorted by key; updated the two tests that encoded the old hash order.
5 Name std::minstd_rand0 explicitly — same small state, and preserves current libstdc++ behaviour so Linux results do not move.
6 Use VELOX_CHECK with a diagnostic message naming the id and string.
7 Join the IO executor before shutting the cache down.
8 Decode the columns instead of assuming an encoding; return owned strings so views stay valid.
9 Capture raw pointers; keep ownership in main(), where objects are released while the memory manager is alive.

Build / packaging

# Fix
10 Apple-only patch making s2 use the requested size directly — exactly what s2's own weak implementation does. Applied from both the bundled CMake resolver and the setup script.
11 Download the -aarch64 tarball on aarch64 hosts.

Tests

# Fix
12 Compare within a tolerance. Expected values were not changed — the denormal cases are only checked where boost is accurate enough to produce the mathematically correct answer.
13 Treat -1 as failure unless the input really is 1969-12-31T23:59:59; check extreme negative years only where representable.
14 Compare coordinates numerically (via strtod, so exponents are handled) and the remaining text exactly.
15 Draw from the engine directly, so every platform sees identical data.
16 Use digestWeighted consistently.
17 Assert the two real guarantees: same seed → same ordering, and result holds exactly the input elements.
18 Compare within a tolerance.
19 Add at least one partition row before rebalancing.
20 Leave a small margin and document that the bound guards against O(N) growth.
21 Repeat the allocate/free pair so accumulated time is measurable on any platform.
22 Build the input explicitly without nulls.
23 Accept either terminal state.
24 Match only the prefix Velox produces.

Verification

Check Result
Full suite 714/714 passed, 0 failed
Build 3494 targets, 0 errors
MultiFragmentTest.abortMergeExchange 0 failures / 12 runs (was 7/10)
velox_dwio_cache_test 0 failures / 25 runs (was 4 in ~14)
velox_sort_benchmark, velox_sparksql_benchmarks_cast exit 0 (were SIGSEGV)
SizeClassStats::operator- test Confirmed failing without the fix

Environment note (not part of this PR)

Two local environment problems accounted for 81 of the original 98 failures and required no
code change:

  • folly built Debug while Velox built Release. -DNDEBUG changes folly's string hash, so
    F14 heterogeneous lookups (std::string vs std::string_view) always missed.
  • Homebrew fmt 12 vs the pinned fmt 11.

Worth knowing for anyone reproducing this: mismatched dependency builds look exactly like
widespread product breakage.

Follow-up worth considering

macos.yml has Run Tests behind if: false. Re-enabling it (after these fixes) would prevent
this class of regression from accumulating again.

🤖 Generated with Claude Code

karthikeyann and others added 24 commits August 20, 2026 22:16
@karthikeyann @claude
fix(type): Format interval day-time values with fmt
21ac6b3 
IntervalDayTimeType::valueToString() passed int64_t and int128_t values to
the "%d" conversions of a variadic format string. The conversion specifiers
of snprintf() are not checked against the argument types, and on AArch64 the
variadic argument area is laid out differently, so the wrong bytes were read
and the milliseconds field printed garbage.

Format through fmt::format(), which is type safe and cannot reproduce this
class of defect.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
fix(vector): Return the last channel for a duplicated flat map key
5baa92c 
The distinct keys vector can hold the same key more than once, for example
when it is wrapped in a dictionary with repeated indices. getKeyChannel()
returned the first element that std::unordered_multimap::equal_range()
happened to yield, but the order of equivalent elements there is unspecified
and differs between standard libraries.

Select the largest matching channel explicitly, so that the last channel that
set a key is the one visible, and document that contract in the header.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
fix(functions): Name the random engine used by KllSketch
82b6c1e 
std::default_random_engine is an implementation defined alias: libstdc++ maps
it to minstd_rand0 and libc++ to minstd_rand. The same seed therefore produced
a different sketch on different platforms, which broke the reproducibility that
the fixed seed configuration is meant to provide.

Name std::minstd_rand0 explicitly. It has the same small state the comment asks
for and keeps the behaviour libstdc++ builds have today.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
fix(memory): Set freeClocks in SizeClassStats::operator-
1ab9851 
The subtraction assigned to result.allocateClocks twice, so the free clocks
delta overwrote the allocate clocks delta and result.freeClocks was left at
zero. Stats differences are used to report per size class timings, for example
through Stats::operator-, so both fields were wrong.

Add a unit test for the subtraction, which fails without the fix.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
fix(memory): Emit arbitrator extra configs in key order
0250f27 
MemoryArbitrator::Config::toString() iterated the extraConfigs unordered map
directly, so the order of the emitted settings depended on the hash
implementation and differed between standard libraries. That order shows up in
error messages and logs, and made them non reproducible across platforms.

Sort the entries by key and update the two tests that encoded the previous
hash order.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
fix(caching): Fail loudly when StringIdMap is inconsistent
4d674aa 
release() looked the string up in stringToId_ and erased the returned
iterator, guarding the lookup with VELOX_DCHECK. VELOX_DCHECK expands to
nothing under NDEBUG, so in a release build a broken invariant erased an
end() iterator, which is undefined behaviour and corrupts memory silently.

Use VELOX_CHECK with a message naming the id and the string instead.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
fix(dwio): Join the IO executor before shutting the cache down
65fe38c 
The cache test fixture called cache_->shutdown() first and only then released
the IO executor. Prefetches run on that executor and pin cache entries, so a
task that was still in flight dereferenced a cache that had already been shut
down, crashing in AsyncDataCacheEntry::setExclusiveToShared().

Join the executor first, so no load is running by the time the cache goes
away. The test crashed in 4 of about 14 runs before this change and passed 25
consecutive runs after it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
build(s2geometry): Avoid folly's nallocx symbol on macOS
2918130 
s2geometry declares nallocx as a function and ships a weak definition that
returns the requested size. On Apple platforms a weak symbol cannot be left
undefined in Mach-O, so folly builds with FOLLY_HAVE_WEAK_SYMBOLS=0 and
instead defines a null function pointer with that name in the global
namespace. The linker binds s2's call to folly's data symbol, so the call
jumps into __DATA and crashes with SIGBUS.

Patch s2 on Apple to use the requested size directly, which is exactly what
its own weak implementation does. The patch is applied from both the bundled
CMake resolver and the dependency setup script, since either can build s2.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
build(hadoop): Download the aarch64 tarball on aarch64 hosts
44f9c14 
The generic Hadoop tarball ships x86-64 native libraries, so the HDFS tests
could not load libhdfs on an aarch64 host.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
fix(benchmarks): Keep the sort benchmark's strings alive
6d6db00 
getDataFromFile() had two defects. It called asFlatVector<StringView>() on the
parquet columns, which returns null when a column is not flat and then
segfaulted, and it built each StringView over a temporary std::string, so the
returned views pointed at freed memory.

Decode the columns instead of assuming an encoding, and return owned strings so
the callers can build views that stay valid.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
fix(benchmarks): Release cast benchmark inputs before exit
e3af689 
The benchmark callback captured the input RowVectorPtr and the ExprSet by
value. folly keeps registered benchmarks in a global that is destroyed after
main() returns, at which point the memory pools backing those vectors are
already gone, so the process crashed in AlignedBuffer::freeToPool() during
static destruction.

Capture raw pointers and keep ownership in main(), where the objects are
released while the memory manager is still alive.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(functions): Compare probability results within a tolerance
10ca8b4 
The CDF tests compared results of boost's transcendental routines with
EXPECT_EQ. Those results are correct to within a few ULP but are not bit
identical across platforms, so the comparisons are not portable.

Compare within a tolerance instead. The two cases that pass denormal
parameters are only checked where boost is accurate enough to produce the
mathematically correct answer; their expected values are left unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(type): Detect timegm() failure portably
eda6de4 
checkUtcToEpoch() treated timegm() as successful unless it both returned -1
and set errno. macOS does not set errno, so out of range calendar values were
compared against the -1 sentinel as if it were a real epoch value. Its
supported range of negative years is also narrower than glibc's.

Recognise -1 as a failure unless the input really is 1969-12-31T23:59:59, and
only check the two extreme negative years where the platform can represent
them.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(functions): Compare ST_Buffer output numerically
143d30b 
The buffer test compared the WKT text exactly, so a one ULP difference in a
single coordinate failed the comparison. Compare the coordinates numerically
with a tolerance and the remaining text exactly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(functions): Generate SetDigest inputs portably
5f833a0 
The tests seed std::mt19937 with a fixed value for reproducibility, but drew
values through std::uniform_int_distribution and std::uniform_real_distribution.
Neither is specified to map engine output to values identically across standard
libraries, so the same seed produced different data on libstdc++ and libc++ and
the estimated error rates drifted past their thresholds.

Draw from the engine directly so every platform sees the same inputs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(functions): Use the weighted digest in largeInputSize
a97e4ee 
The second half of the test built digestWeighted but added the weighted values
to digest, which already held the first batch, and then compared that digest
against the values of the second batch only. The mismatch was hidden by the
particular random data libstdc++ generated.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(sparksql): Do not pin std::shuffle's permutation
de58357 
The shuffle tests compared against one specific permutation. shuffle() draws
through std::shuffle, which uses std::uniform_int_distribution internally, so
the permutation for a given seed differs between standard libraries.

Check the two properties the function actually guarantees: the same seed
produces the same ordering, and the result holds exactly the input elements.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(sparksql): Compare expm1 within a tolerance
2137f23 
std::expm1 is accurate to within a ULP but its exact bits differ between
platforms, so an equality comparison is not portable.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(common): Add a partition row before rebalancing in the fuzz test
36a11f6 
rebalance() requires at least one recorded row, and SkewedPartitionRebalancerTest
asserts that it throws when only processed bytes were added. The fuzz test could
draw zero partitions for an iteration and then call rebalance(), violating that
contract. Whether it did depended on the standard library's distribution, so it
only failed on libc++.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(functions): Leave a margin in the KLL memory bound
5a0a04c 
The bound was the exact byte total one standard library's vector growth
produces. Its purpose is to catch the sketch growing with the number of
inserted elements, so leave a small margin and say so.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(memory): Repeat allocations when measuring allocation clocks
795288d 
A single allocate and free pair can take less than one tick of the hardware
timestamp counter, whose resolution is far coarser on ARM than the x86 TSC, so
the recorded clocks were zero. Repeat the pair so the accumulated time is
measurable on any platform.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(exec): Use data without nulls in filterColumnHandles
957b37f 
The remaining filter evaluates to null, and therefore drops the row, whenever
'a' is null, but the test expected every generated row in the result. Whether
the randomly generated column contained a null depended on the standard
library, so the expectation only held on libstdc++.

Build the input explicitly so the expectation is guaranteed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(exec): Accept either terminal state in abortMergeExchange
5f275e6 
A partial sort task can run to completion before it observes the abort
request, in which case it terminates as finished rather than aborted. The test
waited for the aborted state only, so it timed out on a state that could never
arrive, returned early, and left the merge task running for the teardown check
to trip over.

Accept either terminal state. The test aborted in 7 of 10 runs before this
change and passed 12 consecutive runs after it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@karthikeyann @claude
test(common): Match only Velox's part of the load error
d6bb3c5 
The expected message included the text dlerror() appends after the symbol
name, which each platform's dynamic loader words differently. Match the prefix
Velox produces instead.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@netlify

netlify Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit b03c579
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/6a87e4517fdcc60008cc73fe

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 21, 2026
@karthikeyann karthikeyann added the help wanted Extra attention is needed label Aug 21, 2026
@karthikeyann karthikeyann changed the title fix: Bug fixes for macOS arm64 unit test failures (reference, do not merge) DO NOT LAND fix: Bug fixes for macOS arm64 unit test failures (reference, do not merge) Aug 21, 2026
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Selective Build Plan

Linux release with adapters is running a full build (changes touch velox/experimental/ or velox/external/). See the CI workflows README for what this means.

Selective build plan

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

Reviewers

@jinchengchenghh jinchengchenghh Awaiting requested review from jinchengchenghh jinchengchenghh will be requested when the pull request is marked ready for review jinchengchenghh is a code owner
@rui-mo rui-mo Awaiting requested review from rui-mo rui-mo will be requested when the pull request is marked ready for review rui-mo is a code owner
@majetideepak majetideepak Awaiting requested review from majetideepak majetideepak will be requested when the pull request is marked ready for review majetideepak is a code owner
@assignUser assignUser Awaiting requested review from assignUser assignUser will be requested when the pull request is marked ready for review assignUser is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. help wanted Extra attention is needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@karthikeyann