chore: refactoring AWS code out of the core crate #1995

rtyler · 2023-12-27T23:14:57Z

This change was quite time-consuming and as such changes a lot of files to ensure that unit and integration tests are cleaned up properly.

What should be done in a follow up pull request is the creation of the deltalake-gcs and deltalake-azure crates to contain their objectstore registration code and integration tests. I consider that out of the scope of this change.

This is a prerequisite for my work on #1601

ion-elgreco · 2023-12-30T21:36:05Z

crates/deltalake-aws/src/logstore.rs

-                    source: Box::new(err),
-                },
+                LockClientError::ProvisionedThroughputExceeded => todo!(
+                    "deltalake-aws does not yet handle DynamoDB providioned throughput errors"


ion-elgreco · 2023-12-30T21:37:16Z

crates/deltalake-aws/src/logstore.rs

+                    TransactionError::LogStoreError {
+                        msg: format!(
+                            "lock table '{}' not found",
+                            self.lock_client.get_lock_table_name()


Can't you reuse table_name here?

ion-elgreco · 2023-12-30T21:49:13Z

crates/deltalake-aws/src/storage.rs

+pub struct S3ObjectStoreFactory {}
+
+impl S3ObjectStoreFactory {
+    fn with_env_s3(&self, options: &StorageOptions) -> StorageOptions {


What's the reason we are not using the S3Builder here and then from_env()? https://docs.rs/object_store/latest/object_store/aws/struct.AmazonS3Builder.html#method.from_env

The specifics of why this code does things these way are a little outside of my realm of understanding 😆

I was mostly moving code around, with some refactoring here and there. This code is largely doing the same thing it did when it lived in deltalake-core. That said, this takes StorageOptions rather than S3StorageOptions because the result of this is intended to be passed into the ObjectStoreFactory.parse_url_opts which is a more generic trait and just operates on the StorageOptions struct

ion-elgreco · 2023-12-30T21:55:07Z

crates/deltalake-aws/src/storage.rs

+pub struct S3ObjectStoreFactory {}
+
+impl S3ObjectStoreFactory {
+    fn with_env_s3(&self, options: &StorageOptions) -> StorageOptions {


Why doesn't this take the S3StorageOptions struct?

ion-elgreco · 2023-12-30T21:59:44Z

crates/deltalake-core/Cargo.toml

 ]
 datafusion-ext = ["datafusion"]
 gcs = ["object_store/gcp"]
-hdfs = ["datafusion-objectstore-hdfs"]


Why is this dependency removed?

Lots of the functionality tied to the hdfs feature is totally untested, it's ignored code, so I removed the feature for now. The hdfs code needs to be moved into its own crate and probably its own repo tbh

ion-elgreco · 2023-12-30T22:03:43Z

crates/deltalake-core/src/logstore/mod.rs

+impl LogStoreFactory for DefaultLogStoreFactory {}
+
+/// Registry of [LogStoreFactory] instances
+pub type FactoryRegistry = Arc<DashMap<Url, Arc<dyn LogStoreFactory>>>;


Just for my own understanding, what does DashMap solve here instead of a normal hashmap? I read the docs but I don't grasp what it's purpose is here : P

Dashmap supports lock-free concurrency, so it can be used across threads in the process. HashMap would need to be put into a Mutex<T> first in order to do the same.

ion-elgreco · 2023-12-30T22:16:50Z

crates/deltalake-core/src/table/builder.rs

        }

-        Ok(DeltaTableBuilder::from_uri(table_uri))
+        let url = ensure_table_uri(&table_uri).expect("The specified table_uri is not valid");


This seems like unnecessarily check if you do from_uri already

The module level ensure_table_uri does some behaviors that needed to be preserved for file URLs in particular. DeltaTableBuilder::from_uri and from_valid_uri both have slightly different functionality which I do think we might want to get rid of before 1.0

ion-elgreco · 2023-12-30T22:21:04Z

crates/deltalake-core/src/table/builder.rs

@@ -483,6 +411,8 @@ mod tests {
        assert!(uri.is_ok());
        let _uri = ensure_table_uri("./nonexistent");
        assert!(uri.is_ok());
+        let _uri = ensure_table_uri("file:///tmp/nonexistent/some/path");
+        assert!(uri.is_ok());


It's not checking _uri here

ion-elgreco · 2023-12-30T22:27:32Z

crates/deltalake-test/src/utils.rs

-    OnelakeAbfs,
-}
-
-impl StorageIntegration {


Are these things not used anymore for the other cloud stores?

For the other cloud stores they should be implementing the trait. I don't actually know why our integration tests were implemented the way they that they were, basically with everything in kind of hard-coded lists with feature flags.

The model used in deltalake-aws/tests/common.rs is what I would expect to see more of for the other cloud provider traits

roeap · 2024-01-01T17:05:51Z

@rtyler - took the liberty to disable auto-merge since I think we should squash that one. The indivisula commits don't seem to be valuable on their own.

More importantly though, I started to do a proper review. Off the bat I was wondering if we need a dedicated test crate to place all the data, or if we can just keep that in the core crate, and then test integrations.

In any case i the way things are going, it makes sense to just move the example table to the root of the repository, rather then in a crate. Eventually we would likely add or even replace with the DAT tables.

roeap

Exited to see we are working to untagle all this!!

Left some initial comments mainly orounf how we set up the registries.

Haven't full worked throught the factiories / registires yet, but at first glace it seems like we may be doing a bit too much work inside there.. My thinking would be users need to register a fuly functioning store for a given ObjectStoreUrl (just scheme and host) - where we may or may not wnat to define our own ObjectStoreUrl.

Maybe it looks already like that, just dumping some thought :).

crates/deltalake-core/src/protocol/time_utils.rs

roeap · 2024-01-01T18:26:27Z

crates/deltalake-core/src/table/builder.rs

+        debug!("build_storage() with {}", &self.options.table_uri);
+        let location = Url::parse(&self.options.table_uri).map_err(|_| {
+            DeltaTableError::NotATable(format!(
+                "Could not turn {} into a URL",
+                self.options.table_uri
+            ))


seems like in practice we should never hit this, if the builder was crated via a rout that ensure URIs are fine ... since we do have to dela with relative paths and the likes, how about we make the field on DeltaLoadOptios a URL, and avoid throwing here?

roeap · 2024-01-01T18:30:18Z

crates/deltalake-core/src/logstore/mod.rs

+        return logstore_with(store, location, options);
+    }
+    Err(DeltaTableError::InvalidTableLocation(
+        location.clone().into(),


do we need this clone, as this is the last usage?

roeap · 2024-01-01T18:33:01Z

crates/deltalake-core/src/logstore/mod.rs

+    let scheme = Url::parse(&format!("{}://", location.scheme()))
+        .map_err(|_| DeltaTableError::InvalidTableLocation(location.clone().into()))?;
+
+    if let Some(factory) = logstores().get(&scheme) {


i think we need to consider both scheme and host in the registries. Otherwise we cannot register stores for differnt buckets with differnet credentials.

The file:/// stores will always have an empty host, except in some arcane windows cases, that I have not yet encountered in the wild...

roeap · 2024-01-01T18:40:02Z

crates/deltalake-core/src/storage/mod.rs

+pub type FactoryRegistry = Arc<DashMap<Url, Arc<dyn ObjectStoreFactory>>>;
+
+/// TODO
+pub fn factories() -> FactoryRegistry {


do we need to create global state for this? Ie. could we just place this on DeltaTable?

My main concer is the integration with datafusion and other query engines, where such state is usually handled on a session. I also plan to to leverage the datafusion interns etc for our SCAN et. al. operations. In think in these cases having a global registry might limit hopw we can integrate.

roeap

Exited to see we are working to untagle all this!!

Left some initial comments mainly orounf how we set up the registries.

Haven't full worked throught the factiories / registires yet, but at first glace it seems like we may be doing a bit too much work inside there.. My thinking would be users need to register a fuly functioning store for a given ObjectStoreUrl (just scheme and host) - where we may or may not wnat to define our own ObjectStoreUrl.

Maybe it looks already like that, just dumping some thought :).

There are a number of changes here to untangle the coupling inside of deltalake-core to allow deltalake-aws to be separated properly

This test fails on main but passes in this branch because the URL handling logic introduced properly encodes file URLs. No need for object_store updates here Fixes delta-io#1806

roeap

👍

github-actions bot added binding/rust Issues for the Rust crate crate/core labels Dec 27, 2023

rtyler force-pushed the separating-aws-crate branch 9 times, most recently from 6f92689 to 1d64783 Compare December 30, 2023 20:33

github-actions bot added the binding/python Issues for the Python package label Dec 30, 2023

rtyler marked this pull request as ready for review December 30, 2023 21:12

rtyler requested review from wjones127, fvaleye, roeap and ion-elgreco as code owners December 30, 2023 21:12

rtyler added this to the Rust v0.17 milestone Dec 30, 2023

rtyler force-pushed the separating-aws-crate branch from 00c56ed to b49ebe2 Compare December 30, 2023 21:32

ion-elgreco reviewed Dec 30, 2023

View reviewed changes

rtyler enabled auto-merge (rebase) December 31, 2023 05:08

roeap mentioned this pull request Jan 1, 2024

Proposal to create logical plans for operations #2006

Open

roeap disabled auto-merge January 1, 2024 16:49

roeap requested changes Jan 1, 2024

View reviewed changes

rtyler force-pushed the separating-aws-crate branch from 4649034 to 241db34 Compare January 2, 2024 03:55

Refactor AWS specific code into the deltalake-aws crate

4647d65

There are a number of changes here to untangle the coupling inside of deltalake-core to allow deltalake-aws to be separated properly

rtyler force-pushed the separating-aws-crate branch from 10db6c3 to e288b61 Compare January 3, 2024 01:22

roeap force-pushed the separating-aws-crate branch from ae1d379 to 3de10c3 Compare January 3, 2024 01:43

roeap force-pushed the separating-aws-crate branch from 3de10c3 to 581c381 Compare January 3, 2024 02:19

fix: windows tests

df560a6

roeap force-pushed the separating-aws-crate branch 3 times, most recently from 5185517 to 11c1883 Compare January 3, 2024 12:40

github-actions bot added the delta-inspect label Jan 3, 2024

roeap force-pushed the separating-aws-crate branch 3 times, most recently from 13126ab to bc2092f Compare January 3, 2024 13:17

chore: prune dependencies

56a2f8f

roeap force-pushed the separating-aws-crate branch from bc2092f to 56a2f8f Compare January 3, 2024 13:24

rtyler added 2 commits January 3, 2024 07:07

Introduce a test to make sure tilde paths work properly

264adfa

This test fails on main but passes in this branch because the URL handling logic introduced properly encodes file URLs. No need for object_store updates here Fixes delta-io#1806

Temporarily skip Azure tests

72cf100

rtyler enabled auto-merge (rebase) January 3, 2024 18:57

roeap approved these changes Jan 3, 2024

View reviewed changes

rtyler merged commit 3aa6dc6 into delta-io:main Jan 3, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactoring AWS code out of the core crate #1995

chore: refactoring AWS code out of the core crate #1995

rtyler commented Dec 27, 2023 •

edited

Loading

ion-elgreco Dec 30, 2023

ion-elgreco Dec 30, 2023

ion-elgreco Dec 30, 2023

rtyler Dec 31, 2023

ion-elgreco Dec 30, 2023

ion-elgreco Dec 30, 2023

rtyler Dec 31, 2023

ion-elgreco Dec 30, 2023

rtyler Dec 31, 2023

ion-elgreco Dec 30, 2023

rtyler Dec 31, 2023

ion-elgreco Dec 30, 2023

ion-elgreco Dec 30, 2023

rtyler Dec 31, 2023

roeap commented Jan 1, 2024

roeap left a comment

roeap Jan 1, 2024

roeap Jan 1, 2024

roeap Jan 1, 2024

roeap Jan 1, 2024

roeap left a comment

roeap left a comment

chore: refactoring AWS code out of the core crate #1995

chore: refactoring AWS code out of the core crate #1995

Conversation

rtyler commented Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap commented Jan 1, 2024

roeap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap left a comment

Choose a reason for hiding this comment

roeap left a comment

Choose a reason for hiding this comment

rtyler commented Dec 27, 2023 •

edited

Loading