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.

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

ARROW-3347: [Abandoned] [Rust] Implement PrimitiveArrayBuilder #2648

Closed
wants to merge 3 commits into from

Conversation

paddyhoran
Copy link
Contributor

@paddyhoran paddyhoran commented Sep 28, 2018

This is a sub task of ARROW-3089. The scope of this builder is just for types that implement the ArrowPrimitiveType trait, i.e. PrimitiveArray's.

There are a number of follow up items that need to be addressed below. I will create JIRA's for these once/if this PR is accepted. I wanted to ensure that others agree with the approach and get some feedback.

The general idea here was to implement the Write trait for Buffer this allows Buffer to use the WriteBytesExt extension trait from the byteorder crate which allows Buffer to remain untyped while allowing types that implement ArrowPrimitiveType to be written to a Buffer instance.

The write_to_buffer method is then implemented for each primitive type leveraging these extension methods from byteorder. byteorder did not have an implementation for bool so I had to implement it myself.

The builder itself was pretty simple and leverages these other updates.

Follow up items (JIRA's to be created upon merging this PR):

cc @kszucs @sunchao @andygrove

@codecov-io
Copy link

Codecov Report

Merging #2648 into master will increase coverage by 0.97%.
The diff coverage is 92.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2648      +/-   ##
==========================================
+ Coverage   87.19%   88.17%   +0.97%     
==========================================
  Files         381       13     -368     
  Lines       59223     1463   -57760     
==========================================
- Hits        51642     1290   -50352     
+ Misses       7507      173    -7334     
+ Partials       74        0      -74
Impacted Files Coverage Δ
rust/src/lib.rs 100% <ø> (ø) ⬆️
rust/src/datatypes.rs 75.31% <100%> (+1.89%) ⬆️
rust/src/array.rs 86.25% <100%> (+1.07%) ⬆️
rust/src/buffer.rs 91.04% <64.28%> (-3.13%) ⬇️
python/pyarrow/ipc.pxi
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/ipc/metadata-internal.h
... and 362 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a503ef...7ae8f35. Read the comment docs.

@sunchao
Copy link
Member

sunchao commented Sep 28, 2018

Thanks @paddyhoran , and my apologies for being late on ARROW-3089.

The general idea here was to implement the Write trait for Buffer this allows Buffer to use the WriteBytesExt extension trait from the byteorder crate which allows Buffer to remain untyped while allowing types that implement ArrowPrimitiveType to be written to a Buffer instance.

I wonder if we can add a MutableBuffer struct, alongside the existing Buffer. The former supports growing as well as modification (perhaps by implementing the Write trait), and also conversion into a Buffer.

@paddyhoran
Copy link
Contributor Author

@sunchao no worries, thanks for the feedback.

I think Buffer is designed to be immutable (in fact I just saw that in a comment that I should have updated), adding MutableBuffer seems like a good idea.

@sunchao
Copy link
Member

sunchao commented Sep 29, 2018

Great. Actually I’ve already have an implementation for this. Do you mind if I post a PR for this and review it perhaps? I can create a separate Jira for it.

@paddyhoran
Copy link
Contributor Author

Sure, sounds good.

@paddyhoran paddyhoran changed the title ARROW-3347: [Rust] Implement PrimitiveArrayBuilder ARROW-3347: [WIP] [Rust] Implement PrimitiveArrayBuilder Sep 29, 2018
@paddyhoran
Copy link
Contributor Author

Closing as there is no need for the byteorder crate dependency when we have MutableBuffer and ToByteSlice

@paddyhoran paddyhoran closed this Oct 1, 2018
@paddyhoran paddyhoran changed the title ARROW-3347: [WIP] [Rust] Implement PrimitiveArrayBuilder ARROW-3347: [Abandoned] [Rust] Implement PrimitiveArrayBuilder Oct 25, 2018
@paddyhoran paddyhoran deleted the primitive_array_builder branch November 7, 2018 02:45
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.

3 participants