Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RleDecoder Inline Buffer #1061

Closed
tustvold opened this issue Dec 19, 2021 · 0 comments · Fixed by #1062
Closed

RleDecoder Inline Buffer #1061

tustvold opened this issue Dec 19, 2021 · 0 comments · Fixed by #1062
Labels
enhancement Any new improvement worthy of a entry in the changelog

Comments

@tustvold
Copy link
Contributor

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

RleDecoder has an buffer inlined within the struct here. This is perhaps surprising as it results in a struct that is 4Kb in size, with all the implications that has for the cost of moves, etc...

I only noticed this because clippy's large enum variant lint tripped whilst working on #1054.

My solution there was to use Box<RleDecoder> but I think it might be better to heap allocate this buffer, especially since it is only used by RleDecoder::get_batch_with_dict which may not even be called.

Describe the solution you'd like

Change the type of index_buf from [i32; 1024] to Option<Box<[i32; 1024]>>

Describe alternatives you've considered

It could be left unchanged
@tustvold tustvold added the enhancement Any new improvement worthy of a entry in the changelog label Dec 19, 2021
tustvold added a commit to tustvold/arrow-rs that referenced this issue Dec 19, 2021
@tustvold
Box RleDecoder index buffer (apache#1061)
6b73a7d
@tustvold tustvold mentioned this issue Dec 19, 2021
Merged
@alamb alamb mentioned this issue Dec 20, 2021
Merged
alamb pushed a commit that referenced this issue Dec 20, 2021
@tustvold
Box RleDecoder index buffer (#1061) (#1062)
127fb29 
* Box RleDecoder index buffer (#1061)

* Format
alamb pushed a commit that referenced this issue Dec 21, 2021
@tustvold @alamb
Box RleDecoder index buffer (#1061) (#1062)
fc30a39 
* Box RleDecoder index buffer (#1061)

* Format
@alamb alamb mentioned this issue Dec 21, 2021
Merged
alamb added a commit that referenced this issue Dec 22, 2021
@alamb @tustvold
Box RleDecoder index buffer (#1061) (#1062) (#1081)
2b5c53e 
* Box RleDecoder index buffer (#1061)

* Format

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jan 12, 2022
@tustvold
Remove now unnecessary box (apache#1061)
3ab4125
alamb pushed a commit that referenced this issue Jan 13, 2022
@tustvold
Improve parquet reading performance for columns with nulls by preserv…
231cf78 
…ing bitmask when possible (#1037) (#1054)

* Preserve bitmask (#1037)

* Remove now unnecessary box (#1061)

* Fix handling of empty bitmasks

* More docs

* Add nested nullability test case

* Add packed decoder test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Box RleDecoder index buffer (#1061) tustvold/arrow-rs
1 participant
@tustvold