Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python API - delta.appendOnly enforcement #590

Merged

Conversation

WarSame
Copy link
Contributor

@WarSame WarSame commented Apr 22, 2022

Description

Adding enforcement of delta.appendOnly to the Python API. If delta.appendOnly is passed in as a key to the configuration and the writing mode is not append then the write will fail with a ValueError.

Related Issue(s)

This deals with the first bullet point in #575 . The second bullet point about column invariants is not handled here, so this PR does not close the issue on its own.

Documentation

This configuration field is already covered in the usage.rst -> "Examining a Table" -> "Metadata" section.

@WarSame WarSame changed the title Feature/python append only enforcement Python API - delta.appendOnly enforcement Apr 22, 2022
@WarSame WarSame marked this pull request as ready for review April 22, 2022 03:42
Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this PR!

It looks like you've tested for the case where the configuration is passed while creating the table, but what about if the table already exists? That configuration should be saved with the table. To accomplish this, we should be able to read the existing table configuration from the metadata property:

def configuration(self) -> List[str]:
(And sidenote: I just noticed the typehint on that configuration property is wrong; it should be Dict[str, str], not List[str]. If you could correct that in this PR, it would be appreciated 👍 .)

@fvaleye
Copy link
Collaborator

fvaleye commented Apr 25, 2022

Thank you @WarSame 👍

LGTM, ready to merge after fixing the formatting issue in the CI.

@WarSame
Copy link
Contributor Author

WarSame commented Apr 30, 2022

Sorry for the delay. I've had Covid for the last week so this has taken the backburner.

I believe I've fixed the formatting issues, though it appears the workflows need approval to run. I did add code to account for an existing table's metadata, and updated the tests to match that. Finally, it was getting to be a few lines of code to check these conditions so I pulled it out into its own functions. Please let me know if you prefer it inline.

@wjones127
Copy link
Collaborator

Sorry for the delay. I've had Covid for the last week so this has taken the backburner.

Oh no! Well I hope you are feeling better now.

Finally, it was getting to be a few lines of code to check these conditions so I pulled it out into its own functions. Please let me know if you prefer it inline.

Yeah that looks good to me.

Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few suggestions for the test.

python/deltalake/writer.py Outdated Show resolved Hide resolved
python/tests/test_writer.py Outdated Show resolved Hide resolved
python/tests/test_writer.py Outdated Show resolved Hide resolved
python/tests/test_writer.py Show resolved Hide resolved
python/deltalake/writer.py Outdated Show resolved Hide resolved
@wjones127
Copy link
Collaborator

@WarSame Looks like you just need to sort the imports, and then this PR is good-to-go. 😄

@WarSame
Copy link
Contributor Author

WarSame commented May 4, 2022

@wjones127 I've done so and it should be good to go!

@WarSame WarSame requested a review from fvaleye May 4, 2022 01:33
@wjones127 wjones127 merged commit d890928 into delta-io:main May 4, 2022
fvaleye pushed a commit to fvaleye/delta-rs that referenced this pull request May 4, 2022
* Add appendOnly check and test

* Add .coverage to .gitignore

* Readd metadata test, fix test

* Whitespace

* Remove unneeded concat

* Shorten condition - is this less clear?

* Fix configuration type annotation

* Move code to func, test existing config

* Fix formatting

* Add error string match, remove config to rely on existing config

* Fix linting for function

* Rename function

* Add version, add mode to error

* Add mode/data_store_type itertools product

* Sort imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants