Adopt `ObjectStore` #761

roeap · 2022-08-22T07:14:00Z

Description

First of all I have to apologize, this is a very large PR and not all of the changes have been discussed. However trying to work around the intermediate state in migration turned out to be quite bothersome, so my hope was to "just get it done" and migrate to fully adopting ObjectStore.

As for our object stores ... The DeltaObjectStore wraps the other stores to hand some path translations etc. The local implementations for azure and gcs are completely gone. the local store uses our optimized implementation for replace_if_not_exists and the S3 store leverages the lock client much like before.

Also it seemed to me there were many different code path how we created tables or backends depending on if they were needed initialized or un-initialized. Since there are some nuances now, how the delta store needs to be configured based on the passed uri, i tried passing all of that through the builder. I also introduced a StorageUrl to help with url parsing conssitent with object_store.

ToDo:

update erros either here or in object store. In some places we have a ObjectStoreError::NotImplemented placeholder right now.
Set up integration tests for backends other than S3 - we can likely use the setup for object_store to a large extend.
Update documentation
remove patch for object_store in root Cargo.toml

Follow Ups

S3: migrate batch delete to object_store
Azure: implement batch delete in object_store
Contribute proxy settings in object_store
Isolate rusoto dependencies

Even if we decide to not merge this, I think there should be some valuable work in here to support the transition ...

Update

More and more tests are passing and are taking shape. Since I assume this is hard to review, let me try and summarize what has been done.

adopting object_store meant to remove the gcs and blob backends entirely while retaining the rename_if_not_exists of the local and s3 store. As such, these stores just proxy object_store, but for this functionality. Aside from cleanup, I reworked the error handling, removing StorageError entirely and having dedicated errors in local an s3 but only used within the mod.

All of this fragmented creating tables a bit too much, which is why I collected all creation / uri relevant code in a new builder module, and moved/updated the DeltaTableBuilder there as well. Subsequently all code path using various means of initializing tables, are migrated to using the builder. - the recommended open_* function remain of course, and were already using the builder.

Of course testing all of this must be a priority. Building on @Blajda work around a TestContext and what I learned from object_store around integration testing, I created a new module test_utils inside the crate, where we define an IntegrationContext, that can be instantiated be storage integration. Basically it handles creating and destrying resources like buckets / tempdir unsing cli tools mostly, and has some helpers to load test data into the service. This also means we are always working against a copy of the data, which avoids some of the post-test cleanup code we have floating around.

For testing DeltaObjectStore + backends, i ported the test suite from object_store, since we now expose the same api.
All S3 tests are (will be) migrated to using the IntegrationContext and applied to all integrations (haven't done GCS yet, if the cli is too foreign, I may leave that for a followup)
removed some now redundant tests

The rest is (should) mostly be adopting code to changed types / APIs and some more general houskeeping.

Related Issue(s)

closes #610
closes #690 - the DeltaTableBuilder allows constucting a DeltaObjectStore, which in turn exposes the "raw" object store
closes #609 - supported in object_store
closes #608 - supported in object_store
closes #568 - responsibility moved upstream
closes #546 - no longer a prerequisite
related #514 - at least partially closed since object store has a default retry policy. need to confirm for requests in this crate
closes #497 - added integration tests via azurite
closes #489 - this has been addressed before, and if not, it would be now
closes #721 - moving to officially supported formats

not #397 - while fake-gcs, can run, we need further options exposed in obnject_store to make it work.

Documentation

roeap

Finally we have all tests passing :).

The change is still quite massive, but I tried to use the opportunity to do some general housekeeping around the tests, and run tests for at leat one more integration - azure. All integration tests are now in files prefixed with integration_. Some of this I originally wanted to have in the crate, but there were conflicts with environment setups etc ...

If there is anything I can to to make reviews easier, let me know..

roeap · 2022-08-28T18:45:42Z

python/deltalake/writer.py

@@ -138,10 +139,10 @@ def write_deltalake(

    if isinstance(table_or_uri, str):
        table = try_get_deltatable(table_or_uri)
-        table_uri = table_or_uri
+        table_uri = str(Path(table_or_uri).absolute())


Enforcing an absolute path here was necessary to make the python 3.7 tests work, the reason that somehow encodings got messed up a bit with the leading .. This is a bit concerning, since it seems something is off with path handling, and as far as I understand a difference might be that in pyarrow 9 we do not need additional calls to the fs to get the file size.

My thinking though was that since tests are passing, and I assume we will be making a pass through the python file system implementation rather soon, that we can live with that for now.

Yeah I will take a look at this later as I work more on filesystems.

roeap · 2022-08-28T18:47:25Z

python/tests/conftest.py

@@ -18,7 +18,7 @@ def s3cred() -> None:

 @pytest.fixture()
 def s3_localstack(monkeypatch):
-    monkeypatch.setenv("AWS_REGION", "us-east-2")
+    monkeypatch.setenv("AWS_REGION", "us-east-1")


using us-east-1 to avoid passing a region constrint in CLI for setting up tests. However this is redundant now, since we switch to s3 for s3api cli invocations.

roeap · 2022-08-28T18:49:22Z

rust/src/builder.rs

+/// Storage option keys to use when creating [crate::storage::s3::S3StorageOptions].
+/// The same key should be used whether passing a key in the hashmap or setting it as an environment variable.
+/// Provided keys may include configuration for the S3 backend and also the optional DynamoDb lock used for atomic rename.
+pub mod s3_storage_options {


not sure if we still process all these options, we likely need another pass through the builder anyhow, when we add more explicit support for some delta options.

roeap · 2022-08-28T18:50:17Z

rust/src/builder.rs

+        builder = builder.with_service_account_path(account);
+    }
+
+    // TODO (roeap) We need either the option to insecure requests, or allow http connections


we need some changes in upstream object_store to work with fake-gcs

I thought they already use fake-gcs upstream? https://github.com/apache/arrow-rs/blob/62eeaa5ebd59ac611b8d17f2fc26373fc30af53f/.github/workflows/object_store.yml#L67-L70

Or is it that we need to expose the option on our side?

The upstream tests rely on passing in a custom client via the builder, but that option is not exposed publicly. I think all we need to du is expose that option, then we canb properly set up the backend here for integration tests.

roeap · 2022-08-28T18:56:12Z

rust/src/storage/file.rs

+        match ret {
+            Err(e) if e.0 == libc::EEXIST => Err(LocalFileSystemError::AlreadyExists {
+                path: to.into(),
+                source: Box::new(e),
+            }),
+            Err(e) if e.0 == libc::ENOENT => Err(LocalFileSystemError::NotFound {
+                path: to.into(),
+                source: Box::new(e),
+            }),
+            Err(e) if e.0 == libc::EINVAL => Err(LocalFileSystemError::InvalidArgument {
+                path: to.into(),
+                source: e,
+            }),
+            Err(e) => Err(LocalFileSystemError::Generic {
+                store: STORE_NAME,
+                source: Box::new(e),
+            }),
+            Ok(_) => Ok(()),
+        }
+    }


the error handling should be a bit flatter and more explicit now, A closer look if the mapping is correct should be good though :)

roeap · 2022-08-28T18:57:52Z

rust/src/storage/mod.rs

-                    Err(UriError::InvalidScheme(String::from(parts[0])))
-                }
+    /// Deletes object by `paths`.
+    pub async fn delete_batch(&self, paths: &[Path]) -> ObjectStoreResult<()> {


we have a naive batch delete function here, but we lost the optimized implementation for s3. Rather then retaining that here, I was thinking it's better to contribute this upstream.

yeah, we should contribute this upstream.

apache/arrow-rs#2615

roeap · 2022-08-28T19:01:40Z

rust/src/storage/s3.rs

+
+/// Error raised by storage lock client
+#[derive(thiserror::Error, Debug)]
+enum S3LockError {


The basic idea of using private error types internally, but exposing one global error type to consumers resonates with me. This is more or less adopted from object store. Eventually we may want to apply this throughout delta-rs, with the aim to only expose DetaTableError publicly.

roeap · 2022-08-28T19:03:32Z

rust/tests/integration_object_store.rs

@@ -0,0 +1,438 @@
+#![cfg(feature = "integration_test")]


this contains more or less the entire test suite from object_store.

roeap · 2022-08-28T19:05:22Z

rust/tests/repair_s3_rename_test.rs

+    async fn rename(&self, from: &Path, to: &Path) -> ObjectStoreResult<()> {
+        if let Some(ref path) = self.pause_before_copy_path {
+            if path == to {
+                pause(&self.pause_until_true);
            }
        }
+        self.copy(from, to).await?;
+        if let Some(ref path) = self.pause_before_delete_path {
+            if path == from {
+                pause(&self.pause_until_true);
+            }
+        }
+        self.delete(from).await
+    }


This should capture the logic from the previous dispatcher implementation? @houqp

Looks like it from a quick glance, cc @mosyp since he wrote all the dispatcher tests :)

houqp · 2022-08-29T05:45:55Z

rust/tests/repair_s3_rename_test.rs

+    // let lock_client = dynamodb_lock::DynamoDbLockClient::new(
+    //     rusoto_dynamodb::DynamoDbClient::new(s3_common::region()),
+    //     dynamodb_lock::DynamoDbOptions::default(),
+    // );


can be deleted?

roeap · 2022-08-30T14:21:33Z

@houqp - was a little bit worried about the merge after the parquet2 PR, but as it turns, when you clean up things, things get cleaner :D. So it was very straight-forward to integrate the parquet2 changes!

wjones127

This is great work @roeap! I requested a few minor changes.

wjones127 · 2022-08-30T15:39:01Z

rust/src/storage/mod.rs

-                            return Err(UriError::MissingObjectKey);
-                        }
-                    };
+impl DeltaObjectStore {


So IIUC, the main reason we are wrapping rather than just using the ObjectStore trait as-is is for the base_path / relative URL computation?

Exactly! Since all the paths from the log are relative, we always have to do that conversion, so I thought it would be easiest to just have a base path inside the object store. There are some convenience functions defined, but without the base path, these would certainly not warrant wrapping the store.

wjones127 · 2022-08-30T15:42:41Z

rust/src/storage/s3.rs

@@ -0,0 +1,687 @@
+//! AWS S3 storage backend. It only supports a single writer and is not multi-writer safe.


Isn't it multi-writer safe if the lock is enabled (and all writers use the shared locking mechanism)?

Absolutely it is - in fact a significant part of the development effort was spend on migrating the tests for just that fact :D

rust/src/storage/s3.rs

wjones127 · 2022-08-30T15:53:16Z

rust/src/storage/s3.rs

+    ///
+    /// Options are described in [s3_storage_options].
+    pub fn try_new(
+        storage: Arc<DynObjectStore>,


So usage looks like this, right?

use object_store::aws::AmazonS3Builder; use deltalake::storage::s3::{S3StorageBackend, S3StorageOptions}; let inner = AmazonS3Builder::new().with_bucket_name("my-bucket").build()?; let store = S3StorageBackend::try_new(Arc::new(inner), S3StorageOptions::default())?;

If so, maybe provide the above as an example.

wjones127 · 2022-08-30T17:40:18Z

rust/src/delta.rs

    /// get a shared reference to the delta object store
    pub fn object_store(&self) -> Arc<DeltaObjectStore> {
        self.storage.clone()
    }

+    /// The


Suggested change

/// The

/// The URI of the root of the table.

wjones127 · 2022-08-30T17:43:13Z

python/deltalake/writer.py

@@ -138,10 +139,10 @@ def write_deltalake(

    if isinstance(table_or_uri, str):
        table = try_get_deltatable(table_or_uri)
-        table_uri = table_or_uri
+        table_uri = str(Path(table_or_uri).absolute())


Yeah I will take a look at this later as I work more on filesystems.

wjones127 · 2022-08-30T17:48:52Z

rust/src/builder.rs

+        builder = builder.with_service_account_path(account);
+    }
+
+    // TODO (roeap) We need either the option to insecure requests, or allow http connections


I thought they already use fake-gcs upstream? https://github.com/apache/arrow-rs/blob/62eeaa5ebd59ac611b8d17f2fc26373fc30af53f/.github/workflows/object_store.yml#L67-L70

Or is it that we need to expose the option on our side?

wjones127 · 2022-08-30T18:03:19Z

rust/src/storage/mod.rs

-                    Err(UriError::InvalidScheme(String::from(parts[0])))
-                }
+    /// Deletes object by `paths`.
+    pub async fn delete_batch(&self, paths: &[Path]) -> ObjectStoreResult<()> {


apache/arrow-rs#2615

Co-authored-by: Will Jones <willjones127@gmail.com>

roeap · 2022-08-30T21:00:39Z

@wjones127 - sorry I have to ping you again, made an error transferring the doc test. It's fixed now :).

roeap added 3 commits August 20, 2022 16:15

feat: update datafusion and object store

f8cba3b

feat: adopt ObjectStore

4d5374b

Merge branch 'main' into object-store

6285669

roeap requested review from mosyp, wjones127, fvaleye, houqp and xianwill August 22, 2022 07:15

roeap added 6 commits August 22, 2022 10:09

chore: more cleanup

1b67000

Merge branch 'main' into object-store

946a474

fix: resolve conflict

c7c3c1f

chore: clippy

e4663b3

fix: comment

833150f

chore: add integration test feature

937e2d2

roeap mentioned this pull request Aug 22, 2022

Bump version of the Python binding to 0.6.0 #762

Merged

roeap added 14 commits August 22, 2022 18:00

refactor: object store as ground truth for table uri

a754523

refactor: make creating builder infallible

cb2b462

fix: fix python build

6477004

fix: datafusion tests

3defda4

fix: table uri invocations

fbe25f6

fix: uri normalize test

58dde1e

chore: flatten directories

47225ea

fix: macro imports

342ae17

fix: imports

0a3ea7f

fix: allow configuring non https connections

4a3b7aa

feat: draft integration set scaffolding

c393c90

refactor: clean up storage errors

dccb508

refactor: remove StorageError

01c4bf6

test: port tests from object_store crate

83b8445

roeap mentioned this pull request Aug 23, 2022

Self-resolving errors in reading from ADLS Gen2 on 0.5.8 #765

Closed

roeap added 9 commits August 27, 2022 18:07

fix: datafusion test

382e9c0

fix: integration tests

226358d

chore: clippy

f1b33ec

fix: checkpoint integration tests

bb93a04

fix: environemtn during integration tests

314c321

ci: add macos 10.15 back to test matrix

5ff8eec

fix: write non empty test fiules to work with azure listing

89a3e90

fix: use correct storage backend in azure comit tests

eeeef28

fix: py3.7 tests

535d8b5

roeap commented Aug 28, 2022

View reviewed changes

houqp reviewed Aug 29, 2022

View reviewed changes

roeap added 4 commits August 30, 2022 12:19

Merge branch 'main' into object-store

c9b1d1c

fix: parquet2 merge errors

9663e70

fix: add missing rustls feature gates

efdca25

fix: add missing dev dependency

aae013b

wjones127 reviewed Aug 30, 2022

View reviewed changes

roeap and others added 2 commits August 30, 2022 22:10

chore: pr comments

63a74ca

Apply suggestions from code review

0d655d8

Co-authored-by: Will Jones <willjones127@gmail.com>

roeap requested review from houqp and wjones127 August 30, 2022 20:17

wjones127 previously approved these changes Aug 30, 2022

View reviewed changes

fix: doctest

2b6f94d

roeap dismissed wjones127’s stale review via 2b6f94d August 30, 2022 20:49

wjones127 approved these changes Aug 30, 2022

View reviewed changes

roeap merged commit 17999d2 into delta-io:main Aug 30, 2022

roeap deleted the object-store branch August 30, 2022 21:09

roeap mentioned this pull request Aug 30, 2022

feat: update arrow/datafusion #755

Closed

MrPowers mentioned this pull request Oct 4, 2022

Roadmap 2022 H2 (discussion) delta-io/delta#1307

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt `ObjectStore` #761

Adopt `ObjectStore` #761

roeap commented Aug 22, 2022 •

edited

Loading

roeap left a comment

roeap Aug 28, 2022

wjones127 Aug 30, 2022

roeap Aug 28, 2022

roeap Aug 28, 2022

roeap Aug 28, 2022

wjones127 Aug 30, 2022

roeap Aug 30, 2022

roeap Aug 28, 2022

roeap Aug 28, 2022

houqp Aug 29, 2022

wjones127 Aug 30, 2022

roeap Aug 28, 2022

roeap Aug 28, 2022

roeap Aug 28, 2022

houqp Aug 29, 2022

houqp Aug 29, 2022

roeap commented Aug 30, 2022

wjones127 left a comment

wjones127 Aug 30, 2022

roeap Aug 30, 2022

wjones127 Aug 30, 2022

roeap Aug 30, 2022

wjones127 Aug 30, 2022

wjones127 Aug 30, 2022

wjones127 Aug 30, 2022

wjones127 Aug 30, 2022

wjones127 Aug 30, 2022

roeap commented Aug 30, 2022

		@@ -0,0 +1,687 @@
		//! AWS S3 storage backend. It only supports a single writer and is not multi-writer safe.

Adopt ObjectStore #761

Adopt ObjectStore #761

Conversation

roeap commented Aug 22, 2022 • edited Loading

Description

Update

Related Issue(s)

Documentation

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

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 Aug 30, 2022

wjones127 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

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 Aug 30, 2022

Adopt `ObjectStore` #761

Adopt `ObjectStore` #761

roeap commented Aug 22, 2022 •

edited

Loading