[FEAT] Delta lake allow unsafe rename for local writes #2824
Conversation
CodSpeed Performance ReportMerging #2824 will degrade performances by 34.2%Comparing Summary
Benchmarks breakdown
|
| @@ -825,14 +827,19 @@ def write_deltalake( | |||
| raise ValueError(f"Expected table to be a path or a DeltaTable, received: {type(table)}") | |||
|
|
|||
| # see: https://delta-io.github.io/delta-rs/usage/writing/writing-to-s3-with-locking-provider/ | |||
| scheme = urlparse(table_uri).scheme | |||
| scheme = get_protocol_from_path(table_uri) | |||
| if scheme == "s3" or scheme == "s3a": | |||
| if dynamo_table_name is not None: | |||
| storage_options["AWS_S3_LOCKING_PROVIDER"] = "dynamodb" | |||
| storage_options["DELTA_DYNAMO_TABLE_NAME"] = dynamo_table_name | |||
| else: | |||
| storage_options["AWS_S3_ALLOW_UNSAFE_RENAME"] = "true" | |||
There was a problem hiding this comment.
shouldn't we also enable allow_unsafe_rename to set AWS_S3_ALLOW_UNSAFE_RENAME here?
Another approach is that we can set ALLOW_UNSAFE_RENAME in storage_options (for all backends) so we don't care what scheme we're in.
There was a problem hiding this comment.
I'm shying away from allowing users to both not set the dynamo table name and set allow_unsafe_rename to false because that would change the default behavior of DataFrame.write_deltalake. In other words, with your suggestion someone would need to manually add allow_unsafe_rename=True to revert back to the behavior they used to see if they didn't specify a dynamo table name.
What do you think?
There was a problem hiding this comment.
Yeah thats fine then. Let's just go with that
I don't know a good way to test this since we believe this issue only arises when you mount a fabric table onto the local filesystem. However it is a pretty change that should not affect any existing behavior. I will verify that it works with our users once it is released.