Update parquet to depend on arrow subcrates #3028

tustvold · 2022-11-06T09:16:08Z

Which issue does this PR close?

Part of #2594

Rationale for this change

time cargo build --release before

________________________________________________________
Executed in  101.75 secs    fish           external
   usr time  715.02 secs    0.00 millis  715.02 secs
   sys time   14.18 secs    2.08 millis   14.18 secs

After

________________________________________________________
Executed in   71.16 secs    fish           external
   usr time  463.27 secs    0.72 millis  463.27 secs
   sys time   10.95 secs    1.50 millis   10.95 secs

The tall pole is now actually the compression codecs and not arrow 🎉

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2022-11-06T09:17:52Z

parquet/Cargo.toml

@@ -30,6 +30,14 @@ edition = "2021"
 rust-version = "1.62"

 [dependencies]
+arrow-array = { version = "26.0.0", path = "../arrow-array", default-features = false, optional = true }


It is somewhat unfortunate the number of these, perhaps we should provide re-exports to reduce this. On the flip side, parquet is a very complex crate and so perhaps it is just a bit special in needing all the things 😅

Maybe for some basic crates like arrow-buffer, arrow-data, arrow-schema, perhaps we can provide re-export (arrow-core?) for them.

Like you said, if this is just a special case, then it is fine.

tustvold · 2022-11-06T09:43:15Z

Need to revisit how we plumb in test utilities 😢

tustvold · 2022-11-10T03:15:56Z

parquet_derive/Cargo.toml

+syn = { version = "1.0", features = ["extra-traits"] }
+parquet = { path = "../parquet", version = "26.0.0", default-features = false }


This is part drive-by-cleanup, part fix as it was relying on multiversion which is a transitive dependency of arrow to enable the syn features it requires. As parquet no longer depends on arrow, this caused issues

tustvold · 2022-11-10T03:16:55Z

parquet/src/errors.rs

@@ -19,8 +19,8 @@

 use std::{cell, io, result, str};

-#[cfg(any(feature = "arrow", test))]


This is necessary to avoid having to declare all the various subcrates as dev-dependencies

You mean removing test?

Yeah, this only worked because arrow is a dev-dependency

Hmm, I tried to restore only this line locally, and looks like the tests are still okay to run?

Did you do cargo test --no-default-features?

Ah, yea, arrow is default feature. I didn't exclude it.

viirya · 2022-11-10T04:00:24Z

parquet/Cargo.toml

 default = ["arrow", "snap", "brotli", "flate2", "lz4", "zstd", "base64"]
 # Enable arrow reader/writer APIs
-arrow = ["dep:arrow", "base64"]
+arrow = ["base64", "arrow-array", "arrow-buffer", "arrow-cast", "arrow-data", "arrow-schema", "arrow-select", "arrow-ipc"]


Seems two lines can combined?

What do you mean?

Nvm, I thought if this arrow is unnecessary and we can just have these "base64" features. But I saw you use arrow as a whole feature in many places.

viirya · 2022-11-10T04:07:07Z

parquet/src/column/reader.rs

+    #[cfg(feature = "arrow")]
    pub(crate) fn peek_next(&mut self) -> Result<bool> {


Hmm, is peek_next optional? It is used by some APIs like skip_records. If arrow is not enabled, what would happen?

These changes were necessary to placate clippy as they aren't currently used by anything outside the arrow module

viirya

Looks good to me.

ursabot · 2022-11-10T21:32:39Z

Benchmark runs are scheduled for baseline = 132152c and contender = 4dd7fea. 4dd7fea is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the parquet Changes to the parquet crate label Nov 6, 2022

tustvold commented Nov 6, 2022

View reviewed changes

tustvold marked this pull request as draft November 6, 2022 09:42

Update parquet to depend on arrow subcrates (apache#3044)

620a04c

tustvold force-pushed the parquet-arrow-subcrates branch from f20ea2a to 620a04c Compare November 10, 2022 01:32

tustvold added 3 commits November 10, 2022 14:36

Fix parquet_derive

9286607

Fix parquet_derive

d973e13

Fix no-default-features test compilation

a7d0038

github-actions bot added the parquet-derive label Nov 10, 2022

tustvold added 2 commits November 10, 2022 14:58

Fix parquet-fromcsv

d341f57

Clippy

7dcd70a

tustvold marked this pull request as ready for review November 10, 2022 03:14

tustvold commented Nov 10, 2022

View reviewed changes

tustvold mentioned this pull request Nov 10, 2022

Release Arrow 27.0.0 (next release after 26.0.0) #3045

Closed

8 tasks

viirya reviewed Nov 10, 2022

View reviewed changes

viirya approved these changes Nov 10, 2022

View reviewed changes

tustvold merged commit 4dd7fea into apache:master Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update parquet to depend on arrow subcrates #3028

Update parquet to depend on arrow subcrates #3028

tustvold commented Nov 6, 2022 •

edited

tustvold Nov 6, 2022

viirya Nov 10, 2022

tustvold commented Nov 6, 2022

tustvold Nov 10, 2022

tustvold Nov 10, 2022

viirya Nov 10, 2022

tustvold Nov 10, 2022

viirya Nov 10, 2022 •

edited

tustvold Nov 10, 2022

viirya Nov 10, 2022

viirya Nov 10, 2022 •

edited

tustvold Nov 10, 2022

viirya Nov 10, 2022

viirya Nov 10, 2022

tustvold Nov 10, 2022

viirya left a comment •

edited

ursabot commented Nov 10, 2022

		syn = { version = "1.0", features = ["extra-traits"] }
		parquet = { path = "../parquet", version = "26.0.0", default-features = false }

		@@ -19,8 +19,8 @@

		use std::{cell, io, result, str};

		#[cfg(any(feature = "arrow", test))]

		#[cfg(feature = "arrow")]
		pub(crate) fn peek_next(&mut self) -> Result<bool> {

Update parquet to depend on arrow subcrates #3028

Update parquet to depend on arrow subcrates #3028

Conversation

tustvold commented Nov 6, 2022 • edited

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Nov 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Nov 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Nov 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya left a comment • edited

Choose a reason for hiding this comment

ursabot commented Nov 10, 2022

tustvold commented Nov 6, 2022 •

edited

viirya Nov 10, 2022 •

edited

viirya Nov 10, 2022 •

edited

viirya left a comment •

edited