Repository navigation
Conversation
github-actions
Bot
added
the
arrow-core
Rich-T-kid
requested changes
Oct 27, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
test_replacing_name_releases_previous_allocation() is failing locally for me
can u run this
cargo +nightly miri test -p arrow-schema --lib --features ffi \
ffi::tests::test_replacing_name_releases_previous_allocation
Comment on lines
+196
to
+199
| std::ptr::fn_addr_eq( | ||
| release, | ||
| release_schema as unsafe extern "C" fn(*mut FFI_ArrowSchema), | ||
| ) |
Contributor
There was a problem hiding this comment.
fn_addr_eq(release, release_schema) can return false even for the same function. The Rust documentation explicitly permits this.
Contributor
Author
There was a problem hiding this comment.
Thanks for catching this! I checked — there really is a problem here. Still thinking about the best way to fix it.
Contributor
Author
There was a problem hiding this comment.
@Rich-T-kid I've thought this through and updated the fix to follow the same ownership model as with_metadata: the name is now owned by SchemaPrivateData, and with_name is unsafe. Could you take another look?
Rich-T-kid
self-requested a review
November 1, 2026 22:44
This branch has not been deployed
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.
Which issue does this PR close?
None — standalone bug fix.
Rationale for this change
Repeated calls to
FFI_ArrowSchema::with_nameleak the previous name allocation. For example,.with_name("a")?.with_name("b")?leaves the allocation for"a"unreleased even after the schema is dropped.What changes are included in this PR?
Release the previous name allocation when replacing it.
Keep
with_namesafe by rejecting schemas that do not use this crate's original release callback, since ownership of their name allocation cannot be assumed.Are these changes tested?
Added a regression test in
src/ffi.rsusing a test allocator to verify that the previous name is freed on replacement and the final name is freed when the schema is dropped.The regression test fails before the fix and passes afterward in debug and release builds.
Additional tests cover rejection of empty schemas and foreign release callbacks, including cleanup through the original producer's callback.
Are there any significant user-facing changes?
with_namenow returns an error for schemas without this crate's original release callback. This includes empty schemas and schemas whose release callback has been replaced or wrapped. The method remains safe.AI assistance
Codex identified the issue, generated the fix and regression tests, ran validation, and drafted this PR description.