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
Conversation
`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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
importFromArrowAsOwnerdocuments that the returned vector calls theArrowSchemaandArrowArrayrelease 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 anull_countof 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
FlatVectorhas 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 everyimportFromArrowAsOwnercaller. Both need the same deleter and both are barred fromshared_ptr's aliasing constructor for the same reason, so the two differ in blast radius rather than in difficulty.