Skip to content

fix(arrow): Defer release callbacks for 32-bit decimal imports (mhaseeb123 2026-09-10 option 2) - #16

Open
simoneves wants to merge 1 commit into
simoneves/redo_of_kjmph_tpcds_decimal_fixes_squashed_rebasedfrom
simoneves/haseeb_fix_2b_20260910
Open

fix(arrow): Defer release callbacks for 32-bit decimal imports (mhaseeb123 2026-09-10 option 2)#16
simoneves wants to merge 1 commit into
simoneves/redo_of_kjmph_tpcds_decimal_fixes_squashed_rebasedfrom
simoneves/haseeb_fix_2b_20260910

Conversation

@simoneves

Copy link
Copy Markdown
Owner

importFromArrowAsOwner documents that the returned vector calls the ArrowSchema and ArrowArray release callbacks when it is destroyed. Importing a 32-bit decimal array with no nulls ran them before the import call had returned. The values are widened into a fresh 64-bit buffer, so the vector views no Arrow memory, and a null_count of zero leaves no nulls buffer to view either.

Nothing dangled, because the widened vector holds no pointer into Arrow memory. A producer that frees or unrefs its own state from a release callback still saw that happen while the caller was holding the vector.

The import now takes a view over the Arrow input even though it copies out of it, and gives that view to a deleter on the returned vector. A FlatVector has no slot for a buffer its values do not reference, so the deleter is where the view lives. It drops the vector first, so Arrow memory outlives anything that might still reference it. In viewer mode the view is inert and costs one small allocation per import.

A companion commit fixes the same defect for every import path by binding the releasers in owner-mode importFromArrowImpl. That one also covers times, timestamps, 128-bit-to-short decimals and misaligned long decimals, which still release early here, at the cost of changing release timing and control-block identity for every importFromArrowAsOwner caller. Both need the same deleter and both are barred from shared_ptr's aliasing constructor for the same reason, so the two differ in blast radius rather than in difficulty.

`importFromArrowAsOwner` documents that the returned vector calls the `ArrowSchema` and `ArrowArray` release callbacks when it is destroyed. Importing a 32-bit decimal array with no nulls ran them before the import call had returned. The values are widened into a fresh 64-bit buffer, so the vector views no Arrow memory, and a `null_count` of zero leaves no nulls buffer to view either.

Nothing dangled, because the widened vector holds no pointer into Arrow memory. A producer that frees or unrefs its own state from a release callback still saw that happen while the caller was holding the vector.

The import now takes a view over the Arrow input even though it copies out of it, and gives that view to a deleter on the returned vector. A `FlatVector` has no slot for a buffer its values do not reference, so the deleter is where the view lives. It drops the vector first, so Arrow memory outlives anything that might still reference it. In viewer mode the view is inert and costs one small allocation per import.

A companion commit fixes the same defect for every import path by binding the releasers in owner-mode `importFromArrowImpl`. That one also covers times, timestamps, 128-bit-to-short decimals and misaligned long decimals, which still release early here, at the cost of changing release timing and control-block identity for every `importFromArrowAsOwner` caller. Both need the same deleter and both are barred from `shared_ptr`'s aliasing constructor for the same reason, so the two differ in blast radius rather than in difficulty.
@simoneves simoneves changed the title fix(arrow): Defer release callbacks for 32-bit decimal imports fix(arrow): Defer release callbacks for 32-bit decimal imports (mhaseeb123 2026-09-10 option 2) Sep 10, 2026
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