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.

Sign up for GitHub

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
Merged

Fix literal predicate equality check #94

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
304e13b
Fix literal predicate equality check
danielcweeks Oct 21, 2023
fc133a2
Fix the tests
Fokko Oct 22, 2023
a8d873e
Some more fixes
Fokko Oct 24, 2023
8223531
Merge pull request #1 from Fokko/fix/literal-predicate-quality
danielcweeks Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyiceberg/expressions/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 :)

return self.term == other.term and self.literal == other.literal
return False

Expand Down
4 changes: 2 additions & 2 deletions tests/expressions/test_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ def test_greater_than() -> None:


def test_greater_than_or_equal() -> None:
assert GreaterThanOrEqual("foo", 5) == parser.parse("foo <= 5")
assert GreaterThanOrEqual("foo", "a") == parser.parse("'a' >= foo")
assert GreaterThanOrEqual("foo", 5) == parser.parse("foo >= 5")
assert GreaterThanOrEqual("foo", "a") == parser.parse("'a' <= foo")


def test_equal_to() -> None:
Expand Down