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

Fix literal predicate equality check #94

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

danielcweeks
Copy link
Contributor

The equality check was not checking to see if they were the same predicate so:

assert StartsWith("foo", "data") == parser.parse("foo NOT LIKE 'data'")

would evaluate StartsWith == NotStartsWith as true
(or any combination of LiteralPredicate)

@danielcweeks danielcweeks requested a review from Fokko October 21, 2023 22:16
@@ -701,7 +701,7 @@ def bind(self, schema: Schema, case_sensitive: bool = True) -> BoundLiteralPredi

def __eq__(self, other: Any) -> bool:
"""Return the equality of two instances of the LiteralPredicate class."""
if isinstance(other, LiteralPredicate):
if isinstance(other, self.__class__):
Copy link
Contributor

Choose a reason for hiding this comment

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

This fix is for the LiteralPredicate, but there are more:

  • Line 367 should also use self.__class__ for IsNull, IsNaN etc.
  • Can you add self.__class__ to the SetPredicate on line 534.
  • The __eq__ of NotIn on line 667 can go (In doesn't have one either).

Copy link
Contributor

Choose a reason for hiding this comment

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

Raised a PR danielcweeks#1 to get these fixed :)

@Fokko Fokko added this to the PyIceberg 0.5.1 release milestone Oct 24, 2023
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 @danielcweeks for fixing this 🙌

@Fokko Fokko merged commit 66be1eb into apache:main Oct 24, 2023
6 checks passed
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