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

Table Requirements Validation #200

Merged
merged 5 commits into from
Dec 11, 2023
Merged

Conversation

HonahX
Copy link
Contributor

@HonahX HonahX commented Dec 10, 2023

This PR is the second part of #140

It implements a validate method for each table requirement. This method raises a CommitFailedException with an appropriate error message if the table's metadata does not meet the requirements.

@HonahX HonahX mentioned this pull request Dec 10, 2023
@@ -34,6 +34,10 @@ def __repr__(self) -> str:
"""Return the string representation of the SnapshotRefType class."""
return f"SnapshotRefType.{self.name}"

def __str__(self) -> str:
"""Return the string representation of the SnapshotRefType class."""
return self.value
Copy link
Contributor Author

@HonahX HonahX Dec 11, 2023

Choose a reason for hiding this comment

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

Seems we need this __str__. Otherwise,

>>> ref_type=SnapshotRefType.Branch
>>> f"Test: {ref_type}"

will show Test: SnapshotRefType.Branch in my local environment (Python 3.11) but Test: branch in CI's python 3.8. I think it's because Python 3.11 changes the default behavior of Enum's str: https://blog.pecar.me/python-enum

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Some small comments, but looks good 👍


class AssertCreate(TableRequirement):
"""The table must not already exist; used for create transactions."""

type: Literal["assert-create"] = Field(default="assert-create")

def validate(self, base_metadata: Optional[TableMetadata]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the signature of the parent class as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I updated the signature and also add None-checks for other requirements, which will raise exception when the current table metadata is None.

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this @HonahX!

@Fokko Fokko merged commit e92e10a into apache:main Dec 11, 2023
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.

2 participants