fix(arrow/array): preserve run-end encoded empty values - #1156
fix(arrow/array): preserve run-end encoded empty values#1156fallintoplace wants to merge 3 commits into
Conversation
|
Since this is a draft, i'll hold off on further review until the conflicts are resolved and it's marked ready |
e9b956f to
6d5a42c
Compare
zeroshade
left a comment
There was a problem hiding this comment.
The empty-value behavior is correct for ordinary string values, but it introduces two correctness issues in mixed-builder and dictionary-backed cases. One silently converts a null to an empty value; the other creates an invalid dictionary reference that panics when read. Details and reproductions are inline.
This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. After you’ve addressed the points above and pushed an update, an Apache Arrow Go maintainer — a real person — will take the next look at the PR. If you think a finding is misapplied, please reply on the PR and a maintainer will weigh in.
More on how Apache Arrow Go handles contributions:
CONTRIBUTING.md.
|
|
||
| func (b *RunEndEncodedBuilder) AppendEmptyValue() { | ||
| b.AppendNull() | ||
| b.finishRun() |
There was a problem hiding this comment.
Blocking: After this call resets the unmarshalling state, the pending non-null empty value is indistinguishable from a previously unmarshalled null to UnmarshalOne. Calling AppendEmptyValue() followed by UnmarshalOne(null) produces runEnds=[2], values=[""], and logical values ["", ""]; the appended null is silently converted to an empty string. Expected run ends and values are [1,2] and ["", null]. Please track this state explicitly so a direct empty-value append cannot be mistaken for a null run, and add this sequence as a regression test.
| func (b *RunEndEncodedBuilder) AppendEmptyValue() { | ||
| b.AppendNull() | ||
| b.finishRun() | ||
| b.values.AppendEmptyValue() |
There was a problem hiding this comment.
Major: This delegation creates an invalid result when the encoded type is a dictionary. dictionaryBuilder.AppendEmptyValue() appends index 0 without adding a dictionary entry. Consequently, REE<int16, dictionary<int8, string>> contains an index into a zero-length dictionary, and GetOneForMarshal(0) panics with arrow/array: index out of range. Please provide valid dictionary empty-value behavior or explicitly reject this case, with coverage.
What
Make RunEndEncodedBuilder.AppendEmptyValue and AppendEmptyValues append the value builder empty value instead of a null.
Test