Skip to content

ARROW-8423: [Rust] [Parquet] Serialize Arrow schema metadata #7917

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

Closed
nevi-me wants to merge 5 commits into from
Closed

ARROW-8423: [Rust] [Parquet] Serialize Arrow schema metadata #7917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f11b322
ARROW-8289: [Rust] Parquet Arrow writer with nested support
nevi-me Aug 13, 2020
96a83e6
ARROW-8243: [Rust] [Parquet] Serialize Arrow schema metadata
nevi-me Aug 8, 2020
57628f4
various changes
nevi-me Aug 14, 2020
e8a43e0
move changes made to schema.rs
nevi-me Aug 14, 2020
8abf5cf
address review feedback
nevi-me Aug 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rust/arrow/src/array/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ pub use self::array::StructArray;
pub use self::null::NullArray;
pub use self::union::UnionArray;

pub(crate) use self::array::make_array;
pub use self::array::make_array;

pub type BooleanArray = PrimitiveArray<BooleanType>;
pub type Int8Array = PrimitiveArray<Int8Type>;
Expand Down
3 changes: 2 additions & 1 deletion rust/parquet/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ zstd = { version = "0.5", optional = true }
chrono = "0.4"
num-bigint = "0.3"
arrow = { path = "../arrow", version = "2.0.0-SNAPSHOT", optional = true }
base64 = { version = "*", optional = true }
Copy link
Contributor Author

@nevi-me nevi-me Aug 8, 2020

Choose a reason for hiding this comment

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

Not using a specific version as we already transitively depend on an older version, but I'll change this when the feature changes are merged

serde_json = { version = "1.0", features = ["preserve_order"] }

[dev-dependencies]
Expand All @@ -52,4 +53,4 @@ zstd = "0.5"
arrow = { path = "../arrow", version = "2.0.0-SNAPSHOT" }

[features]
default = ["arrow", "snap", "brotli", "flate2", "lz4", "zstd"]
default = ["arrow", "snap", "brotli", "flate2", "lz4", "zstd", "base64"]
Copy link
Contributor Author

@nevi-me nevi-me Aug 8, 2020

Choose a reason for hiding this comment

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

@vertexclique arrow and base64 would have to go together, should we create a feature that refers to both crates?

Copy link
Contributor

@vertexclique vertexclique Aug 8, 2020

Choose a reason for hiding this comment

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

Would be super nice. Meanwhile I was looking to parquet in this PR I realized that "snap", "brotli", "flate2", "lz4", "zstd" part is not always needed. Since these compressions are not additive features(I am not sure, we might need to check this).

What about a feature gating like this? wdyt?:

default = ["core"]
core = ["arrow", "base64"]
all = ["arrow", "snap", "brotli", "flate2", "lz4", "zstd", "base64"]

Copy link
Contributor Author

@nevi-me nevi-me Aug 8, 2020

Choose a reason for hiding this comment

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

I assume the various compression types could depend on what the parquet source being read supports. In that case, could a user run the risk of not being able to read in a file because it supports compression? I like the core, arrow would have been more intuitive, but I suppose it clashes with the arrow crate

Copy link
Contributor

@vertexclique vertexclique Aug 8, 2020

Choose a reason for hiding this comment

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

I assume the various compression types could depend on what the parquet source being read supports. In that case, could a user run the risk of not being able to read in a file because it supports compression?

Yes, that's true, for having less build dependencies for default set yet another idea (FWIW bikeshedding):

default = ["core"]
core = ["arrow", "base64"]

comp_snap = ["snap"]
comp_brotli = ["brotli"]
comp_flate = ["flate2"]
comp_lz4 = ["lz4"]
comp_zstd = ["zstd"]

and users add their desired compression to enable. Just tested this, it passes parquet test on master as like that (without base64).

Copy link
Contributor Author

@nevi-me nevi-me Aug 8, 2020

Choose a reason for hiding this comment

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

I think that's fine, if users expect to read parquet data which they didn't write (and thus might not know if it's compressed), they can turn on the various compression features.

Copy link
Member

@sunchao sunchao Aug 13, 2020

Choose a reason for hiding this comment

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

Sounds fine. What will the be error message like in this case?

Copy link
Contributor Author

@nevi-me nevi-me Aug 14, 2020

Choose a reason for hiding this comment

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

I'm not sure, removing the compression features and running tests doesn't throw any failures, so I can't see what the behaviour should be.

Loading