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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add support for run-end encoded array #507
feat: Add support for run-end encoded array #507
Changes from 1 commit
b7b280e
65d9185
89a0f1d
7d52451
d6f0a05
7d39533
0ce34e5
a50c84d
8986399
7953380
e625704
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that you need a loop here? (i.e., is it possible to just extract the last run end?)
I wonder if this would be a better fit for
ArrowArrayFinishBuilding()
? Or maybe we needFinishAppending()
? I am not sure I would have thought ofFinishElement
to do these checks/updates since they really only need to happen once per array.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
ArrowArrayFinishRunEndEncoded
might be better as it's more suggestive from its name and less confusion?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, but it is also very similar to the piece of code we have that updates the
null_count
based on the validity buffer, which I think happens inArrowArrayFinishBuilding()
(although that "feature" has caused me some personal confusion because I sometimes forget that it happens and wonder why thenull_count
isn't what I set it to be).The most backward compatible thing to do would be to not do any updating of lengths (i.e., force the caller to set the length of the parent run-end-encoded array). This would not necessarily be difficult to do because they would have to be keeping track of that length to append to the run-ends child (so they would have to keep a counter somewhere anyway). Perhaps for this PR the helper could be omitted and we can learn from the experience of implementing this elsewhere what the best intervention would be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, I've removed it :)