fix(bindings): fix SpanSet serialization size and floatspan distance datum#106
fix(bindings): fix SpanSet serialization size and floatspan distance datum#106estebanzimanyi wants to merge 1 commit into
Conversation
…datum
Two runtime correctness bugs in span arithmetic operators:
1. Union/minus SpanSet serialization truncated: Union_value_span,
Union_span_value, Union_span_span, Minus_value_span, Minus_span_value,
and Minus_span_span all used sizeof(*ret) (fixed SpanSet header only)
when copying the result into DuckDB blob storage. Multi-element SpanSets
(e.g. [1,6]-[3,4] = {[1,3),(4,6]}) silently lost trailing elements,
causing corrupted output or segfaults. Fix: use spanset_mem_size(ret)
and copy directly via StringVector::AddStringOrBlob.
Additionally, minus_span_span returns NULL (not an empty SpanSet) for
non-overlapping inputs; update 005_span_ops.test expected value from {}
to NULL, and activate the three union assertions that were in a mode-skip
block due to the old bug.
2. Floatspan distance datum: Distance_span_value and Distance_value_span
for T_FLOATSPAN assigned the raw Datum returned by distance_span_value()
directly to a double variable, producing the bit-pattern of the double
as an integer (e.g. 4.6e18 instead of 1.0). Fix: wrap with
DatumGetFloat8().
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Consolidation note — overlaps with #107Attempted to consolidate this PR with #107 The cherry-pick hit substantive conflicts in
Per the policy I won't blind-resolve — the maintainer's judgment is needed on:
Suggested resolution: either close this PR in favour of #107 (which is the broader correctness fix) and confirm #107 covers the SpanSet serialization size fix, or rebase #107 on top of this one and reconcile the two. Posting here rather than guessing. |
|
Superseded by the consolidated parity batch at #126 ( The work on this branch — SpanSet serialization size + floatspan distance datum fix — is now part of the squashed addressable-parity surface that brings MobilityDuck to 943/943 = 100.0% of the active addressable temporal + geo names. Closing to keep the review queue focused on the three active stacked PRs (#126 → #130 → #129) and the four orthogonal PRs (#105 / #111 / #113 / #117 / #120). Branch |
1 participant
Summary
Union_*andMinus_*span functions usedsizeof(*ret)(fixed-size header only) when copyingSpanSetresults into DuckDB blob storage — multi-element SpanSets (e.g.[1,6]-[3,4]={[1,3),(4,6]}) silently lost trailing elements, causing corrupted output or segfaults. Fixed by usingspanset_mem_size(ret)with directStringVector::AddStringOrBlob.minus_span_spanreturnsNULL(not empty SpanSet) when inputs are equal — test expectation corrected from{}toNULL.Distance_span_value/Distance_value_spanT_FLOATSPAN case was missingDatumGetFloat8()around the MEOS return value, causing the rawint64_tbit pattern (~4.6e18) to be stored instead of the actual double. Fixed both call sites;Distance_span_spanwas already correct.005_span_ops: unskipped the 3 union queries now that union works; corrected expected values to SpanSet format{[1,3]}/{[1,5]}.Test plan
005_span_ops.testpasses fully (union + minus + distance sections){[1,3),(4,6]}round-trips correctly1.0 <-> floatspan '[2,3]'returns1not4.6e18🤖 Generated with Claude Code