Skip to content

fix(arrow/util): release protobuf enum dictionaries - #1083

Merged
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/protobuf-enum-dictionary-cleanup
Aug 5, 2026
Merged

fix(arrow/util): release protobuf enum dictionaries#1083
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/protobuf-enum-dictionary-cleanup

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

What changed

Release the temporary string builder and array used to initialize protobuf enum dictionaries.

Why

InsertStringDictValues copies values from its input. The temporary builder and array remained owned by the conversion path and leaked after each enum append.

Testing

  • go test ./arrow/util
  • Added checked-allocator coverage for enum conversion

@fallintoplace
fallintoplace force-pushed the fix/protobuf-enum-dictionary-cleanup branch from 72a2d8d to 9280452 Compare August 1, 2026 22:50

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — clean, minimal leak fix.

  • getDictValues now defer bldr.Release()s the temp string builder; since the defer runs after bldr.NewArray() computes the return value, the returned array keeps the transferred buffers and the builder's own allocation is freed.
  • The DICTIONARY path releases dictValues after InsertStringDictValues copies from it, and before the err check, so the error path doesn't leak either.
  • The regression test is a real reproduction: with the fix it passes, and reverting protobuf_reflect.go makes the checked allocator report LEAK of 64 bytes for the temp builder/array.

Verified locally: go test ./arrow/util passes with no regressions. (The go vet note on protobuf_reflect.go:199 about copying a protobuf Any lock is pre-existing and unrelated to this change.)

@zeroshade
zeroshade merged commit f189595 into apache:main Aug 5, 2026
23 checks passed
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.

2 participants