fix: Allow parquet to be compiled without arrow (fix --no-default-features)

[Marwes]

Resolves #733

--no-default-features is currently broken in the parquet crate due to
arrow being required. With some small tweaks it can be made entirely
optional.

Added some extra steps to catch when --no-default-features does not
work on CI as well.

Are there any user-facing changes?

BREAKING CHANGE

The items exported from parquet's test_common module is now only exported if the test feature is used.

[alamb]

Thank you @Marwes for the find

I think the CI is failing because, confusingly, cargo test compiles the crate without the test feature and then compiles the test running code with test

[alamb]

this was copied from arrow/src/util/bit_util.rs (which is fine, I am just pointing it out)

[alamb]

💯 Thank you for adding the tests

[alamb]

I think if you call this feature something other than test (test_common perhaps) and add it to the default list the CI will work

diff --git a/parquet/Cargo.toml b/parquet/Cargo.toml
index 830e33505e..7ad02298b7 100644
--- a/parquet/Cargo.toml
+++ b/parquet/Cargo.toml
@@ -58,9 +58,9 @@ arrow = { path = "../arrow", version = "6.0.0-SNAPSHOT" }
 serde_json = { version = "1.0", features = ["preserve_order"] }
 
 [features]
-default = ["arrow", "snap", "brotli", "flate2", "lz4", "zstd", "base64"]
+default = ["arrow", "snap", "brotli", "flate2", "lz4", "zstd", "base64", "test_common"]
 cli = ["serde_json", "base64", "clap"]
-test = []
+test_common = []
 
 [[ bin ]]
 name = "parquet-read"
diff --git a/parquet/src/util/mod.rs b/parquet/src/util/mod.rs
index 2c653ceef9..94787a5b85 100644
--- a/parquet/src/util/mod.rs
+++ b/parquet/src/util/mod.rs
@@ -22,9 +22,9 @@ pub mod bit_util;
 mod bit_packing;
 pub mod cursor;
 pub mod hash_util;
-#[cfg(feature = "test")]
+#[cfg(feature = "test_common")]
 pub(crate) mod test_common;
-#[cfg(feature = "test")]
+#[cfg(feature = "test_common")]
 pub use self::test_common::page_util::{
     DataPageBuilder, DataPageBuilderImpl, InMemoryPageIterator,
 };

[Marwes]

Ideally this should not be part of the default features as it appears (to me at least) to be a module only used for tests, so end users shouldn't ever need the module.

[codecov-commenter]

Codecov Report

Merging #731 (ea2151b) into master (deb31a0) will increase coverage by 0.01%.
The diff coverage is 86.66%.

❗ Current head ea2151b differs from pull request most recent head 1b9bafc. Consider uploading reports for the commit 1b9bafc to get more accurate results

@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
+ Coverage   82.51%   82.53%   +0.01%     
==========================================
  Files         168      168              
  Lines       47647    47658      +11     
==========================================
+ Hits        39317    39333      +16     
+ Misses       8330     8325       -5     

Impacted Files	Coverage Δ
parquet/src/data_type.rs	77.32% <ø> (ø)
parquet/src/schema/printer.rs	74.65% <83.33%> (+1.95%)	⬆️
parquet/src/util/bit_util.rs	93.18% <100.00%> (+0.03%)	⬆️

---

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 deb31a0...1b9bafc. Read the comment docs.

[nevi-me]

I've reviewed the change, and am happy with it

[jhorstmann]

I just ran into this issue myself, would be great to get this merged.

The get_test_path function in test_common/file_util depends on arrow::util::test_util::parquet_test_data(), so maybe the test_common feature needs to include the arrow feature. Or if that is the only arrow usage than that method could maybe also be copied as it's just 4 lines.

[jhorstmann]

And the arrow feature again depends on base64 for encoding/decoding the arrow schema

[alamb]

Thanks @Marwes . Sorry for the late review + merge. I have been away and am now catching up on PRs

[alamb]

merged @jhorstmann -- sorry for the delay

[alamb]

I don't plan to backport this to arrow 5.4 (it will be included in 6.0) so as to avoid causing downstream projects to break