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

Make parquet support optional for datafusion-common crate #1886

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

jonmmease
Copy link
Contributor

Rationale for this change

Hi! This is a small PR that makes the parquet dependency of the new datafusion-common crate optional. The motivation is that I'd like to depend on datafusion-common in the WebAssembly logic of VegaFusion, and the parquet dependency is the only thing preventing datafusion-common from compiling to WASM.

What changes are included in this PR?

The parquet dependency in datafusion-common has been marked as optional, and feature gates are used in the error.rs file (following the pattern of the optional avro dependency). This feature is then enabled by the datafusion crate.

Are there any user-facing changes?

No

@github-actions github-actions bot added the datafusion Changes in the datafusion crate label Feb 25, 2022
@jonmmease
Copy link
Contributor Author

Thanks for taking a look @Dandandan! All green now

@Dandandan
Copy link
Contributor

This is I think a uncontroversial change, so going ahead and merge this.
Thanks @jonmmease for the contribution. VegaFusion looks really interesting!

@Dandandan Dandandan merged commit 36279ed into apache:master Feb 25, 2022
@alamb
Copy link
Contributor

alamb commented Mar 1, 2022

This looks good. Very cool @jonmmease

FWIW it might make sense to add a test for this to CI (e.g. something like cargo check --no-default-features -p datafusion-common so that we don't accidentally introduce a dependency again)

@jonmmease
Copy link
Contributor Author

Thanks @alamb. That's a good idea. I'll play with it.

The other thing to test would be to check that the crate compiles successfully to the wasm32-unknown-unknown target with wasm-pack. Would that be reasonable? Over time, I would be interested in helping to get more of DataFusion working in the browser (c.f. #177), and having the smaller crates now seems like a good opportunity to make progress toward that goal incrementally.

@alamb
Copy link
Contributor

alamb commented Mar 1, 2022

The other thing to test would be to check that the crate compiles successfully to the wasm32-unknown-unknown target with wasm-pack. Would that be reasonable?

Yes definitely! Thank you

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

Successfully merging this pull request may close these issues.

3 participants