Address benchmarks that aren't compiling #1001
Conversation
carols10cents
force-pushed
the
fix-parquet-bench
branch
3 times, most recently
from
7216c5d
to
307da0d
Compare
.github/workflows/rust.yml
Outdated
| run: | | ||
| export CARGO_HOME="/github/home/.cargo" | ||
| export CARGO_TARGET_DIR="/github/home/target" | ||
| cargo build --benches --features test_common --workspace |
There was a problem hiding this comment.
that's strange, i was expecting any build error to be caught by the cargo clippy --features test_common run in the clippy job above 🤔
There was a problem hiding this comment.
Ah, I can change it to check. I was keeping this new job consistent with the wasm32-build job.
There was a problem hiding this comment.
that's strange, i was expecting any build error to be caught by the cargo clippy --features test_common run in the clippy job above 🤔
That job enables the test_common feature, which fixes the problem :) The original issue (#770) is that cargo bench by itself fails to compile.
There was a problem hiding this comment.
We might able to fix it by adding required-features.
test json::reader::tests::test_json_basic_schema ... ignored
diff --git a/parquet/Cargo.toml b/parquet/Cargo.toml
index f777b10f..d111abf2 100644
--- a/parquet/Cargo.toml
+++ b/parquet/Cargo.toml
@@ -79,4 +79,5 @@ harness = false
[[bench]]
name = "arrow_array_reader"
+required-features = ["test_common"]
harness = false
OOooohhh!! TIL!! This seems to work, update incoming! |
carols10cents
force-pushed
the
fix-parquet-bench
branch
from
ad5ea8e
to
c2ea7b6
Compare
|
Ok, revised: the first commit fails at the new CI job, the second commit passes. The failing job looks like an ICE with Rust nightly that should be fixed in the next nighly; I'll try to rerun tomorrow. |
|
@carols10cents I have worked around the issue with the internal compiler error with rust in #1023 so if you merge from master this PR should be green now |
There was a problem hiding this comment.
This looks great -- thanks @carols10cents . I just hit this as well
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
carols10cents
force-pushed
the
fix-parquet-bench
branch
from
5013e60
to
ed1ec6b
Compare
|
Thanks @carols10cents |
Which issue does this PR close?
Connects to #770. I'm not sure if this PR completely fixes it or not 😅
Rationale for this change
The parquet benchmarks use some types that are behind the test_common feature. The benchmarks don't build without turning on that feature. CI doesn't currently check that benches build, nor is this feature requirement documented anywhere.
What changes are included in this PR?
The first commit adds a job to CI to build (but not run) all benches in the workspace. I pushed this commit up first to demonstrate that it's currently failing.
The second commit adds
--features test_commonto the CI job, and it now passes. I've also documented the need for this flag in parquet/CONTRIBUTING.md.Ideally, it'd be nice to be able to turn on a feature in Cargo.toml
[bench]targets, but afaik that isn't possible currently.Another possible solution is removing the
cfgfrom these types, but I wasn't sure if that was desired or not.Are there any user-facing changes?
Users trying to build parquet's benchmarks should be able to if they read the docs or check what CI does :)