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

feat: new feature flag default-engine-rustls #572

Merged
4 changes: 4 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ jobs:
run: cargo clippy --benches --tests --all-features -- -D warnings
- name: lint without default features
run: cargo clippy --no-default-features -- -D warnings
- name: check kernel builds with default-engine
run: cargo build -p feature_tests --features default-engine
- name: check kernel builds with default-engine-rustls
run: cargo build -p feature_tests --features default-engine-rustls
test:
runs-on: ${{ matrix.os }}
strategy:
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ members = [
"kernel",
"kernel/examples/*",
"test-utils",
"feature-tests",
]
# Only check / build main crates by default (check all with `--workspace`)
default-members = ["acceptance", "kernel"]
Expand Down
16 changes: 16 additions & 0 deletions feature-tests/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[package]
name = "feature_tests"
edition.workspace = true
homepage.workspace = true
keywords.workspace = true
license.workspace = true
repository.workspace = true
readme.workspace = true
version.workspace = true

[dependencies]
delta_kernel = { path = "../kernel" }

[features]
default-engine = [ "delta_kernel/default-engine" ]
default-engine-rustls = [ "delta_kernel/default-engine-rustls" ]
12 changes: 12 additions & 0 deletions feature-tests/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/// This is a compilation test to ensure that the default-engine feature flags are working
/// correctly. Run (from workspace root) with:
/// 1. `cargo b -p feature_tests --features default-engine-rustls`
/// 2. `cargo b -p feature_tests --features default-engine`
/// These run in our build CI.
pub fn test_default_engine_feature_flags() {
#[cfg(any(feature = "default-engine", feature = "default-engine-rustls"))]
{
#[allow(unused_imports)]
use delta_kernel::engine::default::DefaultEngine;
}
}
21 changes: 18 additions & 3 deletions kernel/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ hdfs-native-object-store = { workspace = true, optional = true }
# Used in default and sync engine
parquet = { workspace = true, optional = true }
# Used for fetching direct urls (like pre-signed urls)
reqwest = { version = "0.12.7", optional = true }
reqwest = { version = "0.12.8", default-features = false, optional = true }
strum = { version = "0.26", features = ["derive"] }


Expand All @@ -76,7 +76,10 @@ cloud = [
"hdfs-native-object-store",
]
default = []
default-engine = [

# this is an 'internal' feature flag which has all the shared bits from default-engine and
# default-engine-rustls
default-engine-base = [
"arrow-conversion",
"arrow-expression",
"arrow-array",
Expand All @@ -89,12 +92,24 @@ default-engine = [
"object_store",
"parquet/async",
"parquet/object_store",
"reqwest",
"tokio",
"uuid/v4",
"uuid/fast-rng",
]

# the default-engine use the reqwest crate with default features which uses native-tls. if you want
# to instead use rustls, use 'default-engine-rustls' which has no native-tls dependency
default-engine = [
"default-engine-base",
"reqwest/default",
]

default-engine-rustls = [
zachschuermann marked this conversation as resolved.
Show resolved Hide resolved
"default-engine-base",
"reqwest/rustls-tls-native-roots",
"reqwest/http2",
]

developer-visibility = []
sync-engine = [
"arrow-cast",
Expand Down
2 changes: 1 addition & 1 deletion kernel/src/actions/set_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl SetTransactionScanner {
}
}

#[cfg(all(test, feature = "default-engine"))]
#[cfg(all(test, feature = "sync-engine"))]
mod tests {
use std::path::PathBuf;

Expand Down
6 changes: 3 additions & 3 deletions kernel/src/engine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ pub(crate) mod arrow_conversion;

#[cfg(all(
feature = "arrow-expression",
any(feature = "default-engine", feature = "sync-engine")
any(feature = "default-engine-base", feature = "sync-engine")
))]
pub mod arrow_expression;

#[cfg(feature = "default-engine")]
#[cfg(feature = "default-engine-base")]
pub mod default;

#[cfg(feature = "sync-engine")]
Expand All @@ -25,7 +25,7 @@ macro_rules! declare_modules {
};
}

#[cfg(any(feature = "default-engine", feature = "sync-engine"))]
#[cfg(any(feature = "default-engine-base", feature = "sync-engine"))]
declare_modules!(
(pub, arrow_data),
(pub, parquet_row_group_skipping),
Expand Down
6 changes: 3 additions & 3 deletions kernel/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub enum Error {
},

/// An error performing operations on arrow data
#[cfg(any(feature = "default-engine", feature = "sync-engine"))]
#[cfg(any(feature = "default-engine-base", feature = "sync-engine"))]
#[error(transparent)]
Arrow(arrow_schema::ArrowError),

Expand Down Expand Up @@ -75,7 +75,7 @@ pub enum Error {
#[error("Object store path error: {0}")]
ObjectStorePath(#[from] object_store::path::Error),

#[cfg(feature = "default-engine")]
#[cfg(any(feature = "default-engine", feature = "default-engine-rustls"))]
#[error("Reqwest Error: {0}")]
Reqwest(#[from] reqwest::Error),

Expand Down Expand Up @@ -303,7 +303,7 @@ from_with_backtrace!(
(std::io::Error, IOError)
);

#[cfg(any(feature = "default-engine", feature = "sync-engine"))]
#[cfg(any(feature = "default-engine-base", feature = "sync-engine"))]
impl From<arrow_schema::ArrowError> for Error {
fn from(value: arrow_schema::ArrowError) -> Self {
Self::Arrow(value).with_backtrace()
Expand Down
12 changes: 12 additions & 0 deletions kernel/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,3 +456,15 @@ pub trait Engine: AsAny {
/// Get the connector provided [`ParquetHandler`].
fn get_parquet_handler(&self) -> Arc<dyn ParquetHandler>;
}

// we have an 'internal' feature flag: default-engine-base, which is actually just the shared
// pieces of default-engine and default-engine-rustls. the crate can't compile with _only_
// default-engine-base, so we give a friendly error here.
Comment on lines +460 to +462
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

#[cfg(all(
feature = "default-engine-base",
not(any(feature = "default-engine", feature = "default-engine-rustls",))
))]
compile_error!(
"The default-engine-base feature flag is not meant to be used directly. \
Please use either default-engine or default-engine-rustls."
);
Loading