feat!: update storage configuration system #3383

roeap · 2025-04-13T12:22:57Z

Description

Along with the addition of a caching layer, this PR also attempts to formalise how we handle configuration a bit more. With the caching, we can now configure four layers that can be applied to object stores.

limiting
io runtime
retry
cache

Additionally, we currently need to wrap stores in a PrefixStore pointing at the table root.

Each of these layers has its own configuration that is parsed from the same global map of configuration keys. The configuration for all of these is now aggregated in a new struct StorageConfig.

Our previous abstraction StorageOptions had over time lost its utility since no implementations lived in the core crate anymore and we were just wrapping a HashMap.

The same configuration is also passed again to LogStore and ObjectStore factories where again some parsing happens. With this PR we now present the already parsed configuration (StorageConfig) which exposes utility methods for integrations to decorate / augment the storages the produce.

The new caching store is fairly contained, and I'd be happy to carve that out if it helps reviews.

Signed-off-by: Robert Pack <[email protected]>

codecov · 2025-04-13T12:28:43Z

Codecov Report

Attention: Patch coverage is 62.17617% with 219 lines in your changes missing coverage. Please review.

Project coverage is 71.90%. Comparing base (7538c58) to head (e35c9ca).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/core/src/logstore/config.rs	65.16%	42 Missing and 12 partials ⚠️
crates/core/src/logstore/storage/runtime.rs	9.30%	34 Missing and 5 partials ⚠️
crates/aws/src/storage.rs	61.90%	16 Missing ⚠️
crates/mount/src/file.rs	0.00%	15 Missing ⚠️
crates/azure/src/lib.rs	0.00%	14 Missing ⚠️
crates/mount/src/lib.rs	0.00%	14 Missing ⚠️
crates/core/src/logstore/mod.rs	84.21%	4 Missing and 8 partials ⚠️
crates/catalog-unity/src/lib.rs	0.00%	10 Missing ⚠️
crates/gcp/src/lib.rs	0.00%	9 Missing ⚠️
crates/hdfs/src/lib.rs	0.00%	8 Missing ⚠️
... and 9 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3383      +/-   ##
==========================================
- Coverage   71.97%   71.90%   -0.07%     
==========================================
  Files         145      147       +2     
  Lines       45774    45848      +74     
  Branches    45774    45848      +74     
==========================================
+ Hits        32944    32966      +22     
- Misses      10746    10786      +40     
- Partials     2084     2096      +12

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Robert Pack <[email protected]>

ion-elgreco

Looks very good, happy to see the simplification of StorageConfig, makes it also easier in the future to extend it!

Just left some minor comments

crates/core/src/logstore/storage/cache.rs

crates/core/src/logstore/config.rs

Signed-off-by: Robert Pack <[email protected]>

crates/core/src/logstore/config.rs

Signed-off-by: Robert Pack <[email protected]>

roeap · 2025-04-14T22:27:20Z

crates/core/src/logstore/config.rs

+        let prefix = match ObjectStoreScheme::parse(table_root) {
+            Ok((ObjectStoreScheme::AmazonS3, _)) => Path::parse(table_root.path())?,
+            Ok((_, path)) => path,
+            _ => Path::parse(table_root.path())?,
+        };


This part is concerning me a bit - why does AWS need special treatment? However as we move to working with URLs, I hope we can create a very narrow code path and convert urls to paths in exactly one way ...

Seems not necessary both return the same

agreed! but if we don't do this, tests start to fail .. i was also wondering if maybe the aws cli behaves differently encoding paths when uploading files?

@roeap @ion-elgreco Why do we wrap with a PrefixStore() always? This totally breaks the DeltaTableBuilder:
/// Set the storage backend.
///
/// If a backend is not provided then it is derived from table_uri.
///
/// # Arguments
///
/// * storage - A shared reference to an ObjectStore with
/// "/" pointing at delta table root (i.e. where _delta_log is located).
/// * location - A url corresponding to the storagle location of storage.
pub fn with_storage_backend(mut self, storage: Arc, location: Url) -> Self {
self.storage_backend = Some((storage, location));
self
}

this api is now broken, the url paths will be all wrong since the passed location is now prefixed and not relative to the assumed root of the storage backend

This is to prepare for support for absolute paths, but if you are seeing something wrong. Please create a repro :)

roeap · 2025-04-14T22:29:55Z

crates/core/src/logstore/factories.rs

+pub trait ObjectStoreFactory: Send + Sync {
+    /// Parse URL options and create an object store instance.
+    ///
+    /// The object store instance returned by this method must point at the root of the storage location.
+    /// Root in this case means scheme, authority/host and maybe port.
+    /// The path segment is returned as second element of the tuple. It must point at the path
+    /// corresponding to the path segment of the URL.
+    ///
+    /// The store should __NOT__ apply the decorations via the passed `StorageConfig`
+    fn parse_url_opts(
+        &self,
+        url: &Url,
+        options: &HashMap<String, String>,
+        retry: &RetryConfig,
+    ) -> DeltaResult<(ObjectStoreRef, Path)>;


Here we document the new expected behavior of this method.

roeap · 2025-04-14T22:32:49Z

crates/core/src/logstore/mod.rs

+trait LogStoreFactoryExt {
+    fn with_options_internal(
+        &self,
+        store: ObjectStoreRef,
+        location: &Url,
+        options: &StorageConfig,
+        io_runtime: Option<IORuntime>,
+    ) -> DeltaResult<Arc<dyn LogStore>>;
+}
+
+impl<T: LogStoreFactory + ?Sized> LogStoreFactoryExt for T {
+    fn with_options_internal(
+        &self,
+        store: ObjectStoreRef,
+        location: &Url,
+        options: &StorageConfig,
+        io_runtime: Option<IORuntime>,
+    ) -> DeltaResult<Arc<dyn LogStore>> {
+        let store = options.decorate_store(store, location, io_runtime.map(|r| r.get_handle()))?;
+        self.with_options(Arc::new(store), location, options)
+    }
+}


Right now we use the internal trait to make sure we are injection ourselves into the flow to wrap our layers around the object store. I feel this can be improved, bit hoping to do that in a follou up where we can have a more targeted discussion around this.

roeap · 2025-04-14T22:36:08Z

crates/core/src/logstore/mod.rs

+impl LogStoreConfig {
+    pub fn decorate_store<T: ObjectStore + Clone>(
+        &self,
+        store: T,
+        table_root: Option<&url::Url>,
+        handle: Option<Handle>,
+    ) -> DeltaResult<Box<dyn ObjectStore>> {
+        let table_url = table_root.unwrap_or(&self.location);
+        self.options.decorate_store(store, table_url, handle)
+    }
+
+    pub fn object_store_factory(&self) -> ObjectStoreFactoryRegistry {
+        self::factories::object_store_factories()
+    }
 }


the logstore config is a good way for us to inject functionality into the logstore, since the API dictates log stores also expose it again, but we control the implementation... this should alos gove us a way to just inject a registry taken from - let's say datafusion session - and wire it through.

roeap · 2025-04-14T22:38:07Z

crates/core/src/logstore/storage/mod.rs

-/// Storage option keys to use when creating [ObjectStore].
-///
-/// The same key should be used whether passing a key in the hashmap or setting it as an environment variable.
-/// Must be implemented for a given storage provider
-pub mod storage_constants {


i could not find any way we actually inspect the environment to pichk up this variable. that said, wit the updated configuration parsing we should now have a clear way how we can enable much more config via then environment in a manageable way.

roeap · 2025-04-14T22:39:41Z

crates/core/src/table/builder.rs

-use super::DeltaTable;
-use crate::errors::{DeltaResult, DeltaTableError};
-use crate::logstore::storage::{factories, IORuntime, StorageOptions};
-use crate::logstore::LogStoreRef;
-
-#[allow(dead_code)]
-#[derive(Debug, thiserror::Error)]
-enum BuilderError {
-    #[error("Store {backend} requires host in storage url, got: {url}")]
-    MissingHost { backend: String, url: String },
-    #[error("Missing configuration {0}")]
-    Required(String),
-    #[error("Failed to find valid credential.")]
-    MissingCredential,
-    #[error("Failed to decode SAS key: {0}\nSAS keys must be percent-encoded. They come encoded in the Azure portal and Azure Storage Explorer.")]
-    Decode(String),
-    #[error("Delta-rs must be build with feature '{feature}' to support url: {url}.")]
-    MissingFeature { feature: &'static str, url: String },
-    #[error("Failed to parse table uri")]
-    TableUri(#[from] url::ParseError),
-}
-
-impl From<BuilderError> for DeltaTableError {
-    fn from(err: BuilderError) -> Self {
-        DeltaTableError::Generic(err.to_string())
-    }
-}


we never used any of these 😄

roeap · 2025-04-14T22:40:31Z

crates/core/src/table/builder.rs

-    /// Indicates whether our use case requires tracking tombstones.
-    /// This defaults to `true`
-    ///
-    /// Read-only applications never require tombstones. Tombstones
-    /// are only required when writing checkpoints, so even many writers
-    /// may want to skip them.
-    pub require_tombstones: bool,


we never reference this config ... reading tomstones has been lazy for a while now.

roeap · 2025-04-14T22:42:10Z

python/src/lib.rs

-    pub fn get_py_storage_backend(&self) -> PyResult<filesystem::DeltaFileSystemHandler> {
-        Ok(filesystem::DeltaFileSystemHandler {
-            inner: self.object_store()?,
-            config: self._config.clone(),
-            known_sizes: None,
-        })
-    }


also a relic ...

ion-elgreco

Can you resolve these open comments? Then we are good to go

Signed-off-by: Robert Pack <[email protected]>

roeap · 2025-04-15T12:33:42Z

@ion-elgreco - your comments should be addressed now.

ion-elgreco · 2025-04-15T12:49:21Z

crates/core/src/logstore/storage/cache.rs

+            let path = std::fs::canonicalize(dir)?;
+            (path, None)
+        } else {
+            let tmp_dir = tempfile::tempdir()?;


Have you tested this? I feel like this will still get destroyed before actually access those tmp files

this is not hooked up yet, the plan is to actually wire this up in the next PR, then I'll also add tests.

happy to also just remove the cache impl from this PR, and put it all in hte next one.a follow up.

went with just removing it ... better then introducing dead code. should hopefully make for a cleaner review in the next one 😄.

Yeah because the docs say, once TempDir goes out of scope, it's deconstructed

Signed-off-by: Robert Pack <[email protected]>

roeap added 2 commits April 13, 2025 00:21

feat: harmonize storage config parsing

044f533

Signed-off-by: Robert Pack <[email protected]>

refactor: remove RetryConfigParse trait

c86bc98

Signed-off-by: Robert Pack <[email protected]>

github-actions bot added the binding/rust Issues for the Rust crate label Apr 13, 2025

feat!: formalize parsing of storage options

5a04abe

Signed-off-by: Robert Pack <[email protected]>

roeap force-pushed the cached-store branch from a17279e to 5a04abe Compare April 13, 2025 12:37

ion-elgreco reviewed Apr 13, 2025

View reviewed changes

roeap force-pushed the cached-store branch 2 times, most recently from 2e8c4fe to dba8d52 Compare April 13, 2025 18:22

roeap mentioned this pull request Apr 14, 2025

[tracking] Kernelize! #3298

Open

feat: centrally apply object store layers

5566b67

Signed-off-by: Robert Pack <[email protected]>

roeap force-pushed the cached-store branch from dba8d52 to 5566b67 Compare April 14, 2025 08:09

github-actions bot added the binding/python Issues for the Python package label Apr 14, 2025

ion-elgreco reviewed Apr 14, 2025

View reviewed changes

crates/core/src/logstore/config.rs Show resolved Hide resolved

roeap force-pushed the cached-store branch 3 times, most recently from 85e9e93 to f70d4d8 Compare April 14, 2025 14:39

refactor: isolate factories for storage / log store integrations

a878029

Signed-off-by: Robert Pack <[email protected]>

roeap force-pushed the cached-store branch from f70d4d8 to a878029 Compare April 14, 2025 14:43

rtyler self-assigned this Apr 14, 2025

fix: url parsing inconsistencies

6233d97

Signed-off-by: Robert Pack <[email protected]>

roeap force-pushed the cached-store branch from 1fb01af to 6233d97 Compare April 14, 2025 21:24

roeap marked this pull request as ready for review April 14, 2025 21:47

roeap requested review from wjones127, fvaleye, rtyler and hntd187 as code owners April 14, 2025 21:47

roeap requested a review from ion-elgreco April 14, 2025 21:47

roeap commented Apr 14, 2025

View reviewed changes

ion-elgreco requested changes Apr 15, 2025

View reviewed changes

fix: PR feedback

fbfefc0

Signed-off-by: Robert Pack <[email protected]>

ion-elgreco reviewed Apr 15, 2025

View reviewed changes

fix: PR feedback

e35c9ca

Signed-off-by: Robert Pack <[email protected]>

roeap changed the title ~~feat!: add caching layer and update storage configuration system~~ feat!: update storage configuration system Apr 15, 2025

ion-elgreco approved these changes Apr 15, 2025

View reviewed changes

rtyler approved these changes Apr 16, 2025

View reviewed changes

rtyler added this pull request to the merge queue Apr 16, 2025

Merged via the queue into delta-io:main with commit b2dd8fa Apr 16, 2025
31 checks passed

roeap deleted the cached-store branch April 16, 2025 07:30

feat!: update storage configuration system #3383

feat!: update storage configuration system #3383

Uh oh!

Conversation

roeap commented Apr 13, 2025

Description

Uh oh!

codecov bot commented Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ion-elgreco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ion-elgreco left a comment

Choose a reason for hiding this comment

Uh oh!

roeap commented Apr 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 13, 2025 •

edited

Loading