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.

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

Jump to bottom

fix parquet_derive with default features (and fix cargo publish) #837

Merged
merged 5 commits into from
Oct 18, 2021
Merged

fix parquet_derive with default features (and fix cargo publish) #837

merged 5 commits into from
Oct 18, 2021

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Oct 18, 2021 •
edited

Which issue does this PR close?

Resolves #833
Resolved #840

Rationale for this change

cargo publish for parquet_derive is broken (turns out parquet_derive with default feature flags is broken)

What changes are included in this PR?

(thanks @houqp!)

  1. Add cargo publish verification to verify_release.sh script, following model of datafusion's check and run caro test --all
  2. Add integration test to CI to ensure a crate that uses only parquet_derive can be build with default features
  3. Fix feature gates

Are there any user-facing changes?

parquet_derive can be used standalone again

@github-actions github-actions bot added parquet Changes to the parquet crate parquet-derive labels Oct 18, 2021
@alamb alamb changed the title fix parquet_derive publishing fix parquet_derive with default features (and fix cargo publish) Oct 18, 2021
alamb
alamb commented Oct 18, 2021
View reviewed changes
parquet/Cargo.toml
@@ -54,8 +54,8 @@ snap = "1.0"
brotli = "3.3"
flate2 = "1.0"
lz4 = "1.23"
arrow = { path = "../arrow", version = "7.0.0-SNAPSHOT" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this use of arrow with default features as a dev dependency masked the problem with parquet_derive in tests.

parquet/Cargo.toml
@@ -41,7 +41,7 @@ lz4 = { version = "1.23", optional = true }
zstd = { version = "0.9", optional = true }
chrono = "0.4"
num-bigint = "0.4"
arrow = { path = "../arrow", version = "7.0.0-SNAPSHOT", optional = true, default-features = false }
arrow = { path = "../arrow", version = "7.0.0-SNAPSHOT", optional = true, default-features = false, features = ["ipc"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fix of the actual problem: the arrow writer requires the ipc feature which is no longer a default

parquet_derive/test/dependency/README.md
@@ -0,0 +1,21 @@
<!---
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this follows the pattern of the tests in arrow/test/dependency

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2021 •
edited

Codecov Report

Merging #837 (89fa7c4) into master (d4bf0b6) will increase coverage by 0.02%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #837      +/-   ##
==========================================
+ Coverage   82.57%   82.60%   +0.02%     
==========================================
  Files         168      168              
  Lines       47946    48040      +94     
==========================================
+ Hits        39593    39685      +92     
- Misses       8353     8355       +2
Impacted Files Coverage Δ
arrow/src/array/array_map.rs 81.50% <ø> (ø)
arrow/src/array/equal/mod.rs 93.45% <ø> (ø)
arrow/src/array/equal/utils.rs 74.00% <ø> (ø)
arrow/src/compute/kernels/limit.rs 100.00% <ø> (ø)
arrow/src/record_batch.rs 92.65% <ø> (ø)
arrow/src/util/integration_util.rs 70.10% <ø> (ø)
integration-testing/src/lib.rs 0.00% <0.00%> (ø)
parquet/src/arrow/arrow_writer.rs 98.07% <ø> (ø)
parquet/src/arrow/levels.rs 83.56% <ø> (ø)
arrow/src/array/array.rs 83.38% <64.06%> (-0.25%) ⬇️
... and 31 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 f14d213...89fa7c4. Read the comment docs.

@alamb
Copy link
Contributor Author

alamb commented Oct 18, 2021

I don't really understand why tests like https://github.com/apache/arrow-rs/pull/837/checks?check_run_id=3927127187 are failing with errors like

  export CARGO_HOME="/github/home/.cargo"
  export CARGO_TARGET_DIR="/github/home/target"
  cd arrow
  cargo test --features "simd"
  shell: sh -e {0}
    Updating crates.io index
error: failed to select a version for the requirement `comfy-table = "^4.0"`
candidate versions found which didn't match: 2.1.0, 2.0.0, 1.6.0, ...
location searched: crates.io index
required by package `arrow v7.0.0-SNAPSHOT (/__w/arrow-rs/arrow-rs/arrow)`
Error: Process completed with exit code 101.

I am trying a workaround with 89fa7c4 (the theory being there is something wrong with the cargo cache that is cached in github). We'll see how that works

@alamb
Copy link
Contributor Author

alamb commented Oct 18, 2021

Filed #838 for the CI problem

@alamb alamb mentioned this pull request Oct 18, 2021
Merged
houqp
houqp approved these changes Oct 18, 2021
View reviewed changes
Copy link
Member

@houqp houqp left a comment

Choose a reason for hiding this comment

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

Thanks @alamb for the quick fix

@houqp houqp mentioned this pull request Oct 18, 2021
Closed
@alamb alamb merged commit d6f3ef7 into apache:master Oct 18, 2021
@alamb alamb deleted the alamb/fix-publish branch October 18, 2021 18:40
alamb added a commit that referenced this pull request Oct 24, 2021
@alamb
fix parquet_derive with default features (and fix cargo publish) (#837
4226e09 
)

* Run all tests and do dry runs of cargo publish

* Add test for building parquet derive with default features'

* fix feature flags in parquet crate

* fixup rat

* fix default feature test
@alamb alamb mentioned this pull request Oct 24, 2021
Merged
@alamb alamb added the cherry-picked PR that was backported to active release (will be included in maintenance release) label Oct 24, 2021
alamb added a commit that referenced this pull request Oct 25, 2021
@alamb
Cherry pick fix parquet_derive with default features (and fix cargo p…
94cddca 
…ublish) (#856)

* fix parquet_derive with default features (and fix `cargo publish`) (#837)

* Run all tests and do dry runs of cargo publish

* Add test for building parquet derive with default features'

* fix feature flags in parquet crate

* fixup rat

* fix default feature test

* Update parquet_derive/test/dependency/default-features/Cargo.toml

* Remove merge issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimexist jimexist jimexist approved these changes

@houqp houqp houqp approved these changes

Assignees
No one assigned
Labels
cherry-picked PR that was backported to active release (will be included in maintenance release) parquet Changes to the parquet crate parquet-derive
Projects
None yet
Milestone
No milestone
4 participants
@alamb @codecov-commenter @jimexist @houqp