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

Partition column comparison is an assertion rather than if block with raise exception #2242

Closed
joe-sharman opened this issue Mar 4, 2024 · 3 comments · Fixed by #2286
Closed
Labels
bug Something isn't working

Comments

@joe-sharman
Copy link
Contributor

joe-sharman commented Mar 4, 2024

Environment

Delta-rs version:
0.16.0
Binding:
python
Environment:
MacOS


Bug

What happened:
An assertion is done between partition columns from incoming data and target table. If this fails an AssertionError is raised.

What you expected to happen:
This should be an if block, and raise an exception if the two values aren't the same. ValueError would be fine.

How to reproduce it:
Try to write a table with the wrong partition columns specified in the incoming data.

More details:
This exception should also be changed, maybe something like FileExistsError

raise AssertionError("DeltaTable already exists.")

Edit:
assert statements are ignored when running in optimise mode, so this could cause issues for a user running in optimised mode

@joe-sharman joe-sharman added the bug Something isn't working label Mar 4, 2024
@ion-elgreco
Copy link
Collaborator

@joe-sharman do you want to make a contribution to address this?

@joe-sharman
Copy link
Contributor Author

I'll put something together :)

joe-sharman added a commit to joe-sharman/delta-rs that referenced this issue Mar 13, 2024
# Description

- Replaces assert and AssertionError with built-in exceptions
- Amended tests to reference new exception types

# Related Issue(s)
Closes delta-io#2242
@joe-sharman
Copy link
Contributor Author

Hey @ion-elgreco I've had a go making the change. I've tried to follow the commit conventions in the repo and have used built-in exceptions rather than custom as that seems to be the convention here.

ion-elgreco pushed a commit that referenced this issue Mar 13, 2024
…2286)

# Description

- Replaces assert and AssertionError with built-in exceptions
- Amended tests to reference new exception types
- Following conventions in file of using built-in exceptions rather than
custom exceptions

# Related Issue(s)
Closes #2242

# Documentation

<!---
Share links to useful documentation
--->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants