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.

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

Set default-features = false for zstd in the parquet crate to support wasm32-unknown-unknown #1414

Merged
merged 3 commits into from
Mar 16, 2022

Conversation

kylebarron
Copy link
Contributor

@kylebarron kylebarron commented Mar 10, 2022

Which issue does this PR close?

Relevant to #180 but does not fully close it, as LZ4 still does not build on wasm32-unknown-unknown.

Rationale for this change

What changes are included in this PR?

Add default-features = false to the zstd dependency.

Are there any user-facing changes?

None

@github-actions github-actions bot added the parquet Changes to the parquet crate label Mar 10, 2022
@kylebarron kylebarron changed the title Update zstd version for wasm support Update zstd version in the parquet crate for wasm support Mar 10, 2022
@alamb
Copy link
Contributor

alamb commented Mar 10, 2022

Related: #1415

@kylebarron
Copy link
Contributor Author

kylebarron commented Mar 10, 2022

I did also have to add default-features = false for wasm support; it seemed like the parquet crate wasn't using any default features.

Is the failing integration test CI run related to this?

@kylebarron
Copy link
Contributor Author

Version 0.11.0 is missing the shim header files for wasm support. I updated the Cargo.toml's include array in gyscos/zstd-rs#145 and that was included in 0.11.1.

Now the parquet crate builds with

cargo build --target wasm32-unknown-unknown --no-default-features --features zstd

(after adding getrandom = { version = "0.2.3", features = ["js"] }, needed for wasm, though usually marked as a dependency by a downstream package).

@kylebarron
Copy link
Contributor Author

I was made aware that bumping to 0.11.1 in Cargo.toml might not be necessary, as cargo update will choose 0.11.1 automatically. But regardless, wasm support still needs default-features = false.

@kylebarron kylebarron changed the title Update zstd version in the parquet crate for wasm support Set default-features = false for zstd in the parquet crate to support wasm32-unknown-unknown Mar 15, 2022
@alamb
Copy link
Contributor

alamb commented Mar 16, 2022

I was made aware that bumping to 0.11.1 in Cargo.toml might not be necessary, as cargo update will choose 0.11.1 automatically. But regardless, wasm support still needs default-features = false.

Putting 0.11.1 in Cargo.toml will ensure that at least 0.11.1 is used (not 0.11.0, if there is such a thing). I think it is fine to leave in

@codecov-commenter
Copy link

Codecov Report

Merging #1414 (835acb9) into master (717216f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1414   +/-   ##
=======================================
  Coverage   82.67%   82.67%           
=======================================
  Files         185      185           
  Lines       53866    53866           
=======================================
  Hits        44535    44535           
  Misses       9331     9331           
Impacted Files Coverage Δ
arrow/src/array/transform/mod.rs 86.31% <0.00%> (-0.12%) ⬇️
parquet_derive/src/parquet_field.rs 66.21% <0.00%> (+0.22%) ⬆️

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 717216f...835acb9. Read the comment docs.

@alamb alamb merged commit 486b084 into apache:master Mar 16, 2022
@alamb
Copy link
Contributor

alamb commented Mar 16, 2022

Thanks @kylebarron

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants