Support --all-features again

[nicklan]

What changes are proposed in this pull request?

If both arrow_53 and arrow_54 are specified, just use arrow_53

Also update the README for the new situation.

I'd like to emit a warning if more than one feature is enabled, but there is no compiler_warning!. See here for some discussion. If reviewers feel the must_use hack would be worth it, I can add it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.03%. Comparing base (ca18e7f) to head (22394c8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #708      +/-   ##
==========================================
- Coverage   84.36%   84.03%   -0.33%     
==========================================
  Files          75       77       +2     
  Lines       17654    18063     +409     
  Branches    17654    18063     +409     
==========================================
+ Hits        14893    15179     +286     
- Misses       2052     2165     +113     
- Partials      709      719      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zachschuermann]

ah very awesome! and maybe worth having a quick line in the readme that says when you do --all-features we default to the oldest arrow supported?

[scovich]

This seems a bit counterproductive, given that arrow is only useful if default-engine or sync-engine feature is specified (unless they're default features, which I doubt)?

[nicklan]

we always depend on kernel with default-engine and sync-engine (see integration-tests/Cargo.toml). This flag on this crate just adds the dependency on kernel with the particular specified arrow_x flag.

[scovich]

The current combo works but I suspect it would get out of hand quickly if we ever need to support 3+ arrow versions? Also not super easy to read. What if we did:

#[cfg(feature = "arrow_53")]
pub use arrow_53::*;

#[cfg(all(feature = "arrow_54", not(feature = "arrow_53")))]
pub use arrow_54::*;

#[cfg(all(feature = "arrow_55", not(feature = "arrow_54"), not(feature = "arrow_53")))]
pub use arrow_55::*;

etc

Should be easier to tell what's going on, and it's "only" a quadratic number of terms instead of a combinatoric explosion. When it's time to drop support for the oldest version, we just delete the top entry and remove it from the end of each remaining list.

[nicklan]

Yep, this is much nicer. Updated, thanks!

[nicklan]

ah very awesome! and maybe worth having a quick line in the readme that says when you do --all-features we default to the oldest arrow supported?

Good call. The README was actually totally wrong, so I've fixed it now :)

[zachschuermann]

for some reason it looks like the semver checks are still failing with the "multiple versions of arrow specified"? not sure if we just need to rerun? Shouldn't this PR fix that?

[nicklan]

for some reason it looks like the semver checks are still failing with the "multiple versions of arrow specified"? not sure if we just need to rerun? Shouldn't this PR fix that?

It tries to build main which is the part that fails. It should fix things once this merges.

[zachschuermann]

thanks sorry forgot it's also building base rev :)

[scovich]

Shouldn't we also select arrow/parquet 53 if no version was specifically requested? Otherwise people have to specify something just to compile, even if they don't care what arrow version they use.

[zachschuermann]

restamping :) LGTM!