feat: new feature flag default-engine-rustls

[zachschuermann]

What changes are proposed in this pull request?

We had a request to remove our (implicit) dependency on native-tls. The default-engine feature flag pulls in reqwest with default features which requires native-tls. This PR introduces a new feature flag: default-engine-rustls which instead specifies default-features = false for reqwest enables the same features which are used in object_store: reqwest = { version = "0.12", default-features = false, features = ["rustls-tls-native-roots", "http2"], optional = true }

Details

This PR actually introduces two new feature flags: and 'internal' feature flag default-engine-base and the new default-engine-rustls. The former doesn't work on its own, we throw a compile error if you attempt to use the 'base' feature without either default-engine or default-engine-rustls:

cargo b -p delta_kernel --features default-engine-base
   Compiling delta_kernel v0.6.0 (/Users/zach.schuermann/dev/delta-kernel-rs2/kernel)
error: The default-engine-base feature flag is not meant to be used directly. Please use either default-engine or default-engine-rustls.
   --> kernel/src/lib.rs:470:1
    |
470 | / compile_error!(
471 | |     "The default-engine-base feature flag is not meant to be used directly. \
472 | |     Please use either default-engine or default-engine-rustls."
473 | | );
    | |_^

Lastly, a new crate feature-tests in the workspace was created to ensure that the features work as expected.

This PR affects the following public APIs

New default-engine-rustls feature flag. (New private _default-engine-base feature flag)

How was this change tested?

New feature-tests crate and added to CI.

Future work

1. this flag isn't exposed in the FFI (just normal default-engine is)
2. need to clean up feature flags in general (lots of overlap, etc.)
3. need to add a PresignedUrl test (does not yet exist)

[zachschuermann]

rust doesn't have a notion of 'private features' AFAIK, so although we could do something like this I've just left it in the simple (duplicated) way:

_default_engine_base = [
  "arrow-conversion",
  "arrow-expression",
  "arrow-array",
  "arrow-buffer",
  "arrow-cast",
  "arrow-json",
  "arrow-schema",
  "arrow-select",
  "futures",
  "object_store",
  "parquet/async",
  "parquet/object_store",
  "tokio",
  "uuid/v4",
  "uuid/fast-rng",
]

default-engine = [
  "_default_engine_base",
  "reqwest/default",
]

default-engine-rustls = [
  "_default_engine_base",
  "reqwest/rustls-tls",
  "reqwest/charset",
  "reqwest/http2",
  "reqwest/macos-system-configuration",
]

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 83.45%. Comparing base (e5c14eb) to head (548b042).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
feature-tests/src/lib.rs	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #572      +/-   ##
==========================================
- Coverage   83.49%   83.45%   -0.04%     
==========================================
  Files          74       75       +1     
  Lines       16915    16922       +7     
  Branches    16915    16922       +7     
==========================================
  Hits        14123    14123              
- Misses       2137     2144       +7     
  Partials      655      655              

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

[nicklan]

I think this can be a lot simpler.

If there's an issue for it, please note in the description that it closes it.

[zachschuermann]

request came from @sppalkia but just tried to capture it in the PR description - I can open an issue if that's better though!

[nicklan]

lgtm. thanks

[jkylling]

Maybe we could simplify this to use the same features of reqwest as the object_store crate with the cloud feature: https://github.com/apache/arrow-rs/blob/c4dbf0d8af6ca5a19b8b2ea777da3c276807fc5e/object_store/Cargo.toml#L53 ? Given that we use reqwest for pre-signed URLs, it seems likely that users would use the object_store crate with the cloud feature at the same time, and so be satisfied with the reqwest feature flags set by object_store.

[zachschuermann]

thanks @jkylling - agree that makes sense! I've updated the feature flags to mirror object_store when you use this new feature flag. (though I wonder if we should just rip the band-aid off now and make a breaking change to our default features?)
cc @nicklan @scovich

[jkylling]

thanks @jkylling - agree that makes sense! I've updated the feature flags to mirror object_store when you use this new feature flag. (though I wonder if we should just rip the band-aid off now and make a breaking change to our default features?) cc @nicklan @scovich

Agree, it makes sense to no not expose the TLS feature flags of reqwest to users, at least until we get requests for it to be configurable.

[nicklan]

If I don't specify the default-engine feature, and do specify default-engine-rustls, won't our current code not compile any of the default engine? Like all the places that do: #[cfg(feature = "default-engine")] have to become #[cfg(any(feature = "default-engine", feature = "default-engine-rustls"))]?

Would it work to just move everything to #[cfg(feature = "_default_engine_base")]?

It might be good to add a test that things compile with both sets of flags since currently we're only testing the standard set. Hopefully our use of reqwest is limited enough that it's not an issue, but it would be good to know it at least works to compile with the default-engine-rustls set.

[zachschuermann]

@nicklan looks like semver-checks are failing since the new crate doesn't exist in the old rev? Have you run into this before? (for now i'm just removing the breaking change label - we can see that the delta_kernel part passes and all the features/deps under default-engine should be identical)

[nicklan]

lgtm, thanks!

[scovich]

This seems like it would achieve the desired result, but I wonder how rustic it is? Does anyone else do it this way?

[zachschuermann]

yea I'd looked around and hoped for a better/cleaner solution but didn't really find one. considering we basically don't allow users to take a dependency on this feature only, I think it would be reasonable to change it if we find a better way in the future?