fix: pass storage options down when getting delta table #893

wjones127 · 2022-10-18T03:15:38Z

Description

We forgot to pass down storage options, so storage options were being ignored if you were passing a URI and not a DeltaTable.

This also adds a new S3 option: AWS_S3_ALLOW_UNSAFE_RENAME which allows using the default S3 backend without support for concurrent writers. This is an opt-in option and there is a helpful error message to provide guidance to users.

To test, I implemented integration tests that run against localstack.

Related Issue(s)

might be a fix for Python write_deltalake to S3 fails to write due to "invalid json" #883

Documentation

wjones127 · 2022-10-18T03:28:26Z

python/deltalake/writer.py

+            storage_options = (table._storage_options or {}).update(
+                storage_options or {}
            )



@roeap Does this seems like an acceptable simplification? This would mean the storage options provided will override any ones that were originally specified in the table.

I think this should work fine. Right now we have an additional issue though, when is comes to storage options. Not entirely sure, but in some cases environment variables set may take precedence over configuration ... I remember fixing this in some places, but fore S§ I am not 100% sure..

isn't environment variable overriding code configuration expected behavior?

Hmm, not sure. My personal expectation would be that my explicit config takes precedence over the environment. Looking at e.g. multiple targets it seems to be easier to manage. IIRC the python file systems behave like that.

I currently get:

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Generic { store: "S3", source: CreateMultipartRequest { source: Error { retries: 0, message: "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<Error><Code>SignatureDoesNotMatch</Code><Message>The request signature we calculated does not match the signature you provided

With these changes.

If you have configured the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables, they have higher precedence than the values you've stored in your credentials file. In my case however they are the same, so should not matter.

roeap

LGTM!

roeap · 2022-10-19T19:27:47Z

python/deltalake/writer.py

+            storage_options = (table._storage_options or {}).update(
+                storage_options or {}
            )



I think this should work fine. Right now we have an additional issue though, when is comes to storage options. Not entirely sure, but in some cases environment variables set may take precedence over configuration ... I remember fixing this in some places, but fore S§ I am not 100% sure..

Thelin90

Some insights from me, if valuable at all.

Thelin90 · 2022-10-21T18:11:53Z

python/deltalake/writer.py

@@ -153,7 +153,7 @@ def write_deltalake(
        else:
            # Non-existant local paths are only accepted as fully-qualified URIs
            table_uri = "file://" + str(Path(table_or_uri).absolute())
-        table = try_get_deltatable(table_or_uri)
+        table = try_get_deltatable(table_or_uri, storage_options)


I tried this myself by also adding:

def try_get_deltatable(table_uri: str, storage_options: Dict[str, str], version: Optional[int] = None) -> Optional[DeltaTable]: try: return DeltaTable(table_uri=table_uri, storage_options=storage_options, version=version) except PyDeltaTableError as err: # TODO: There has got to be a better way... if "Not a Delta table" in str(err): return None elif "cannot find" in str(err): return None elif "No such file or directory" in str(err): return None else: raise

This will return None when the table does not exist, instead of failing for creds, but potentially also taking the version into account later...

This will return None when the table does not exist, instead of failing for creds, but potentially also taking the version into account later...

I'm not sure what you are saying here. It's supposed to return None when the table does not exist. Are you saying that if it just fails to authenticate to the Object store it might also return None?

Sorry, to be clear.

I'm saying the logic seem correct and yes it should return None in this case.

Thelin90 · 2022-10-21T18:21:01Z

python/deltalake/writer.py

-        else:
-            filesystem = pa_fs.PyFileSystem(
-                DeltaStorageHandler(table_uri, storage_options)
+            storage_options = (table._storage_options or {}).update(


This is the same as I tried:

if filesystem is None: if table is not None: storage_options = (table._storage_options or {}).update( storage_options or {} ) filesystem = pa_fs.PyFileSystem(DeltaStorageHandler(table_uri, storage_options))

Thelin90 · 2022-10-21T18:22:53Z

python/deltalake/writer.py

+            storage_options = (table._storage_options or {}).update(
+                storage_options or {}
            )



I currently get:

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Generic { store: "S3", source: CreateMultipartRequest { source: Error { retries: 0, message: "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<Error><Code>SignatureDoesNotMatch</Code><Message>The request signature we calculated does not match the signature you provided

With these changes.

If you have configured the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables, they have higher precedence than the values you've stored in your credentials file. In my case however they are the same, so should not matter.

Thelin90 · 2022-10-22T09:13:50Z

python/tests/test_fs.py

@@ -29,3 +32,75 @@ def test_read_simple_table_from_remote(s3_localstack):
    table_path = "s3://deltars/simple"
    dt = DeltaTable(table_path)
    assert dt.to_pyarrow_table().equals(pa.table({"id": [5, 7, 9]}))
+
+
+@pytest.mark.s3


@wjones127 is this you adding an integration test for S3? Based on our other conversations, since it seems like the unit-test were just testing on your local filesystem writing new delta tables.

wjones127 · 2022-10-24T00:34:02Z

python/deltalake/writer.py

+    if filesystem is not None:
+        raise NotImplementedError("Filesystem support is not yet implemented.  #570")


I made a note of what we have to do here: #570 (comment)

wjones127 · 2022-11-03T02:30:44Z

cc @houqp @roeap either you have a moment to re-review? I added integration tests and made a few more meaningful changes.

fvaleye

Thanks @wjones127 👍

It's great to include integration tests; we will avoid regressions in our future changes.

LGTM, it's OK to include this PR in the v0.6.3 version of the Python binding!

Thelin90 · 2022-11-10T00:30:07Z

Does this now work based on previous issues? Is it released in the latest version?

# Description Adding Azure integration tests to the Python bindings. ~~Will need to rebase after we merge #893.~~ # Related Issue(s) - fixes #955 # Documentation

wjones127 commented Oct 18, 2022

View reviewed changes

wjones127 marked this pull request as ready for review October 18, 2022 03:48

wjones127 requested review from fvaleye, rtyler, roeap and houqp as code owners October 18, 2022 03:48

roeap previously approved these changes Oct 19, 2022

View reviewed changes

Thelin90 reviewed Oct 21, 2022

View reviewed changes

wjones127 added 3 commits October 21, 2022 15:58

fix: pass storage options down when getting delta table

6446699

chore: cleanup

3e00963

test: try to get integration tests running again

0d5cb2f

wjones127 dismissed roeap’s stale review via 0d5cb2f October 22, 2022 03:57

wjones127 force-pushed the fix/pass-storage branch from b4ba322 to 0d5cb2f Compare October 22, 2022 03:57

wjones127 requested review from xianwill and mosyp as code owners October 22, 2022 03:57

feat: add option to do unsafe rename

cdb13f4

Thelin90 reviewed Oct 22, 2022

View reviewed changes

wjones127 added 3 commits October 23, 2022 15:16

fix: merge storage_options dictionaries correctly

7f688b0

fix: use anonymous access in tests to avoid timeout

12213f9

fix: add option to test cases

c87e5a7

wjones127 requested a review from roeap October 24, 2022 00:06

fix: fix raise error that filesystem support is not yet implemented

bf2a880

wjones127 commented Oct 24, 2022

View reviewed changes

This was referenced Oct 26, 2022

Python library error reading from large Delta Table on S3 #882

Closed

PyDeltaTableError: Failed to read delta log object: Generic DeltaS3ObjectStore error: Atomic rename requires a LockClient for S3 backends. #911

Closed

wjones127 enabled auto-merge (squash) November 3, 2022 02:29

This was referenced Nov 3, 2022

test(python): add azure integration tests #912

Merged

Threading issues accessing ADLSGen2 table from Python #915

Closed

fvaleye approved these changes Nov 9, 2022

View reviewed changes

wjones127 merged commit 1a79774 into delta-io:main Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pass storage options down when getting delta table #893

fix: pass storage options down when getting delta table #893

wjones127 commented Oct 18, 2022 •

edited

Loading

wjones127 Oct 18, 2022

roeap Oct 19, 2022

houqp Oct 20, 2022

roeap Oct 20, 2022

Thelin90 Oct 21, 2022 •

edited

Loading

roeap left a comment

roeap Oct 19, 2022

Thelin90 left a comment

Thelin90 Oct 21, 2022 •

edited

Loading

wjones127 Oct 21, 2022

Thelin90 Oct 21, 2022

Thelin90 Oct 21, 2022

Thelin90 Oct 21, 2022 •

edited

Loading

Thelin90 Oct 22, 2022

wjones127 Oct 22, 2022

wjones127 Oct 24, 2022

wjones127 commented Nov 3, 2022

fvaleye left a comment

Thelin90 commented Nov 10, 2022

		if filesystem is not None:
		raise NotImplementedError("Filesystem support is not yet implemented. #570")

fix: pass storage options down when getting delta table #893

fix: pass storage options down when getting delta table #893

Conversation

wjones127 commented Oct 18, 2022 • edited Loading

Description

Related Issue(s)

Documentation

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

Thelin90 Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

roeap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thelin90 left a comment

Choose a reason for hiding this comment

Thelin90 Oct 21, 2022 • 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

Thelin90 Oct 21, 2022 • 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

wjones127 commented Nov 3, 2022

fvaleye left a comment

Choose a reason for hiding this comment

Thelin90 commented Nov 10, 2022

wjones127 commented Oct 18, 2022 •

edited

Loading

Thelin90 Oct 21, 2022 •

edited

Loading

Thelin90 Oct 21, 2022 •

edited

Loading

Thelin90 Oct 21, 2022 •

edited

Loading