Skip to content

fix(bindings): fix SpanSet serialization size and floatspan distance datum#106

Open
estebanzimanyi wants to merge 1 commit intomainfrom
fix/span-arithmetic-correctness
Open

fix(bindings): fix SpanSet serialization size and floatspan distance datum#106
estebanzimanyi wants to merge 1 commit intomainfrom
fix/span-arithmetic-correctness

Conversation

@estebanzimanyi
Copy link
Copy Markdown
Member

@estebanzimanyi estebanzimanyi commented May 4, 2026

👀 Reviewers: tier ranking, dependency chains and the standards checklist live in doc/contributing/reviewer-guide.md.

Summary

  • Union_* and Minus_* span functions used sizeof(*ret) (fixed-size header only) when copying SpanSet results 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 using spanset_mem_size(ret) with direct StringVector::AddStringOrBlob.
  • minus_span_span returns NULL (not empty SpanSet) when inputs are equal — test expectation corrected from {} to NULL.
  • Distance_span_value / Distance_value_span T_FLOATSPAN case was missing DatumGetFloat8() around the MEOS return value, causing the raw int64_t bit pattern (~4.6e18) to be stored instead of the actual double. Fixed both call sites; Distance_span_span was already correct.
  • Parity test 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.test passes fully (union + minus + distance sections)
  • Multi-element SpanSet difference {[1,3),(4,6]} round-trips correctly
  • Float distance 1.0 <-> floatspan '[2,3]' returns 1 not 4.6e18

🤖 Generated with Claude Code

…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>
@estebanzimanyi
Copy link
Copy Markdown
Member Author

Consolidation note — overlaps with #107

Attempted to consolidate this PR with #107 fix(bindings): fix distance return types; add + shift alias for tstzset/tstzspan+interval (both span correctness fixes — qualify as a thematically-related batch per docs/PR-COORDINATION.md on #120).

The cherry-pick hit substantive conflicts in src/temporal/span_functions.cpp:

Per the policy I won't blind-resolve — the maintainer's judgment is needed on:

  1. Which helper name to keep (SecondsToInterval or SecsToInterval).
  2. Whether to take the simpler (this PR) or the per-type-correct (PR fix(bindings): fix distance return types; add + shift alias for tstzset/tstzspan+interval #107) Distance_span_span shape.

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant