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 JSON support optional via a feature flag (#2300) #2601

Merged
merged 4 commits into from Sep 1, 2022

Conversation

tustvold
Copy link
Contributor

Which issue does this PR close?

Closes #2300

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Aug 28, 2022
@github-actions github-actions bot added the parquet Changes to the parquet crate label Aug 28, 2022
pub use self::data::ArrayData;
pub use self::data::ArrayDataBuilder;
pub use self::data::ArrayDataRef;
pub(crate) use self::data::BufferSpec;

#[cfg(feature = "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.

Drive by fix for unused code warning when compiling with --no-default-features

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Code looks good

I think we should also:

  1. Mention json as a feature in the readme: https://github.com/apache/arrow-rs/tree/master/arrow#feature-flags
  2. Update the CI jobs https://github.com/apache/arrow-rs/blob/master/.github/workflows/arrow.yml

Specifically for CI:

  1. Update clippy test to also have json feature
  2. Possibly also update wasm test to use json

I am not sure if there are any other jobs that used to test json but now do not.

cc @nevi-me

@alamb alamb changed the title Add json feature (#2300) Make JSON optional, put behind a feature flag (#2300) Sep 1, 2022
@alamb alamb changed the title Make JSON optional, put behind a feature flag (#2300) Make JSON support optional via a feature flag (#2300) Sep 1, 2022
@tustvold
Copy link
Contributor Author

tustvold commented Sep 1, 2022

I think CI doesn't need updating as this feature is enabled by default?

@alamb
Copy link
Contributor

alamb commented Sep 1, 2022

I think CI doesn't need updating as this feature is enabled by default?

Several of the CI jobs explicitly run with --no-default-features so I think the ones I mentioned should be updated otherwise we will lose coverage

@tustvold
Copy link
Contributor Author

tustvold commented Sep 1, 2022

Several of the CI jobs explicitly run with --no-default-features so I think the ones I mentioned should be updated otherwise we will lose coverage

Can you point me to the tests you are referring to, the ones with --no-default-features are complimentary to other jobs that run with the default feature selection and therefore check compilation. As far as I can tell none of them make sense to be updated, but I could be wrong

Edit: aah I see the wasm build is --no-default-features

@alamb
Copy link
Contributor

alamb commented Sep 1, 2022

Yeah, I double checked -- the WASM target looks like the only one. I was mistaken about the clippy target -- it looks fine as it doesn't disable default features 👍

@tustvold tustvold merged commit b792ff7 into apache:master Sep 1, 2022
@ursabot
Copy link

ursabot commented Sep 1, 2022

Benchmark runs are scheduled for baseline = 20282d1 and contender = b792ff7. b792ff7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

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

Successfully merging this pull request may close these issues.

Make JSON support Optional via Feature Flag
3 participants