You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/07/12 17:27:05 UTC
[GitHub] [iceberg] CircArgs opened a new pull request, #5258: More Python Expressions
CircArgs opened a new pull request, #5258:
URL: https://github.com/apache/iceberg/pull/5258
Adding EQ, LT, GT. Assuming the negated versions are unnecessary.
WIP - tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r929012461
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and ', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
+ return id(self) == id(other) or (
+ type(self) == type(other) and self.term == other.term and self.literals == other.literals
+ )
-@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
Review Comment:
didn't give me trouble.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928963428
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
Review Comment:
@Fokko Originally, this was not optional, but was made so to account for the Unary expressions for example `IsNaN`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r919444305
##########
python/pyiceberg/expressions/base.py:
##########
@@ -327,6 +347,54 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundIn[T]:
return BoundIn(bound_ref, tuple(lit.to(bound_ref.field.field_type) for lit in self.literals)) # type: ignore
+@dataclass(frozen=True)
+class BoundEQ(BoundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("EQ expressions do not support negation.")
+
+
+@dataclass(frozen=True)
+class EQ(UnboundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("EQ expressions do not support negation.")
+
+ def bind(self, schema: Schema, case_sensitive: bool) -> BoundEQ[T]:
+ bound_ref = self.term.bind(schema, case_sensitive)
+ return BoundEQ(bound_ref, self.literals[0].to(bound_ref.field.field_type)) # type: ignore
+
+
+@dataclass(frozen=True)
+class BoundLT(BoundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("LT expressions do not support negation.")
+
+
+@dataclass(frozen=True)
+class LT(UnboundPredicate[T]):
Review Comment:
This isn't a huge deal and I know the Java enum used these abbreviations like `LT`, but I feel like expanding these might be better for the python class names, i.e. `LessThan`.
In a larger expression, `LessThan(Reference("foo_num"), literal(10))` is easier to reason about when seen for the first time than `LT(Reference("foo_num"), literal(10))`.
##########
python/pyiceberg/expressions/base.py:
##########
@@ -172,13 +178,33 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
+ raise AttributeError(
+ f"{self.__class__} expects a number of literals between at least {self.literals_len_lb} and at most {self.literals_len_ub or float('inf')}"
+ )
@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
Review Comment:
Should this be `Optional[int]` since it has `default=1` for the field?
##########
python/pyiceberg/expressions/base.py:
##########
@@ -172,13 +178,33 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
+ raise AttributeError(
+ f"{self.__class__} expects a number of literals between at least {self.literals_len_lb} and at most {self.literals_len_ub or float('inf')}"
+ )
@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
Review Comment:
I think using a pipe character will work here for the typehint. -> `literals: Tuple[Literal[T], ...] | Literal[T]`
##########
python/pyiceberg/expressions/base.py:
##########
@@ -327,6 +347,54 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundIn[T]:
return BoundIn(bound_ref, tuple(lit.to(bound_ref.field.field_type) for lit in self.literals)) # type: ignore
+@dataclass(frozen=True)
+class BoundEQ(BoundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("EQ expressions do not support negation.")
+
+
+@dataclass(frozen=True)
+class EQ(UnboundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("EQ expressions do not support negation.")
Review Comment:
Initially I was thinking we wouldn't need `__invert__` since we could just negate when visiting...but I'm realizing we need to capture these inversions as the user defines the expression. Since `~EQ(Reference("foo"), literal("bar"))` would raise `TypeError: bad operand type for unary ~: EQ`, we actually might need a `NotEQ` class that can be used directly or returned by `EQ.__invert__`. Does that sound right?
##########
python/pyiceberg/expressions/base.py:
##########
@@ -172,13 +178,33 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
+ raise AttributeError(
+ f"{self.__class__} expects a number of literals between at least {self.literals_len_lb} and at most {self.literals_len_ub or float('inf')}"
+ )
@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
Review Comment:
This makes sense to me but `if isinstance(self.literals, Literal):` is more explicit here and seems more intuitive.
##########
python/pyiceberg/expressions/base.py:
##########
@@ -172,13 +178,33 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
+ raise AttributeError(
+ f"{self.__class__} expects a number of literals between at least {self.literals_len_lb} and at most {self.literals_len_ub or float('inf')}"
+ )
@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
Review Comment:
For the `literals_len_lb` and `literals_len_ub`, are there scenarios where we need this specificity or is it the case that there will be two kinds of predicates: those that take a single literal and those that take a set of literals of any size? If it's the latter, should we just handle that in the `__post_init__` methods for the concrete predicate classes?
```py
@dataclass(frozen=True)
class In(UnboundPredicate[T]):
def __post_init__(self):
if isinstance(self.literals, Literal):
object.__setattr__(self, "literals", set(self.literals,))
```
...and do the reverse for predicates that require a single literal:
```py
@dataclass(frozen=True)
class EQ(UnboundPredicate[T]):
def __post_init__(self):
if isinstance(self.literals, Set):
if len(self.literals) > 1:
raise TypeError("...")
object.__setattr__(self, "literals", self.literals[0])
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928739649
##########
python/pyiceberg/expressions/base.py:
##########
@@ -156,7 +158,7 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
Returns:
BoundReference: A reference bound to the specific field in the Iceberg schema
"""
- field = schema.find_field(name_or_id=self.name, case_sensitive=case_sensitive)
+ field = schema.find_field(name_or_id=self.name, case_sensitive=case_sensitive) # pylint: disable=W0621
Review Comment:
```suggestion
field = schema.find_field(name_or_id=self.name, case_sensitive=case_sensitive) # pylint: disable=redefined-outer-name
```
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and ', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
+ return id(self) == id(other) or (
+ type(self) == type(other) and self.term == other.term and self.literals == other.literals
+ )
-@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
- term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: UnboundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> UnboundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and ', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
Review Comment:
We could also leverage the Python dataclasses and have the eq method generated.
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
Review Comment:
```suggestion
if not self.literals or len(self.literals) != 1:
```
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
Review Comment:
For readability and type checking it is best to also define the variables in the class itself:
```python
class BoundPredicate(Bound[T], BooleanExpression):
_term: BoundTerm[T]
_literals: Tuple[Literal[T]]
```
This will also remove the ignore below:
```python
@property
def literals(self) -> tuple[Literal[T], ...]:
return self._literals # type: ignore
```
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
Review Comment:
I'm a bit confused by the signature of the `__init__`, that allows us to pass in a `None`, and also the default value is `None`, but we clearly require the literal to be set. Wouldn't it be better to also set this in the signature?
Instead of:
```python
def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
```
Go for something like:
```python
def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T]] | Literal[T]):
```
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and ', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
+ return id(self) == id(other) or (
+ type(self) == type(other) and self.term == other.term and self.literals == other.literals
+ )
-@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
Review Comment:
This one is missing the `bind()` method
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
Review Comment:
What do you think of just accumulating the positional arguments into a list of literals?
```suggestion
def __init__(self, term: BoundTerm[T], *literals: Literal[T]):
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r919586454
##########
python/pyiceberg/expressions/base.py:
##########
@@ -172,13 +178,33 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
+ raise AttributeError(
+ f"{self.__class__} expects a number of literals between at least {self.literals_len_lb} and at most {self.literals_len_ub or float('inf')}"
+ )
@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
Review Comment:
yeah maybe I was playing too much as I tend to
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r919585687
##########
python/pyiceberg/expressions/base.py:
##########
@@ -172,13 +178,33 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
+ raise AttributeError(
+ f"{self.__class__} expects a number of literals between at least {self.literals_len_lb} and at most {self.literals_len_ub or float('inf')}"
+ )
@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
Review Comment:
I figured `Optional` would imply it could be `None` (would be equivalent to `int | None` which I don't think makes sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r929221621
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,70 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ _term: BoundTerm[T]
+ _literals: tuple[Literal[T], ...]
+
+ def __init__(self, term: BoundTerm[T], *literals: Literal[T]):
+ self._term = term
+ self._literals = literals
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
Review Comment:
When I test this and `literals` is an empty list, I get `x[]` (where `x` is the term)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928999058
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
Review Comment:
A missed that one, thanks! Again, I would suggest changing this into:
```python
def _validate_literals(self): # pylint: disable=W0238
if self.literals is not None:
raise AttributeError("IsNaN is a unary predicate and takes no Literals.")
```
into:
```
def _validate_literals(self): # pylint: disable=W0238
if self.literals:
raise AttributeError("IsNaN is a unary predicate and takes no Literals.")
```
This way we raise the error on an empty list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r927869568
##########
python/pyiceberg/expressions/base.py:
##########
@@ -172,13 +178,33 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
+ raise AttributeError(
+ f"{self.__class__} expects a number of literals between at least {self.literals_len_lb} and at most {self.literals_len_ub or float('inf')}"
+ )
@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
Review Comment:
ditched this idea in favor of an obfuscated `_validate_literals` method that defaults to the common pattern of checking for having exactly one literal otherwise children override the validation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r927868382
##########
python/pyiceberg/expressions/base.py:
##########
@@ -327,6 +347,54 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundIn[T]:
return BoundIn(bound_ref, tuple(lit.to(bound_ref.field.field_type) for lit in self.literals)) # type: ignore
+@dataclass(frozen=True)
+class BoundEQ(BoundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("EQ expressions do not support negation.")
+
+
+@dataclass(frozen=True)
+class EQ(UnboundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("EQ expressions do not support negation.")
+
+ def bind(self, schema: Schema, case_sensitive: bool) -> BoundEQ[T]:
+ bound_ref = self.term.bind(schema, case_sensitive)
+ return BoundEQ(bound_ref, self.literals[0].to(bound_ref.field.field_type)) # type: ignore
+
+
+@dataclass(frozen=True)
+class BoundLT(BoundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("LT expressions do not support negation.")
+
+
+@dataclass(frozen=True)
+class LT(UnboundPredicate[T]):
Review Comment:
still open to this. currently changed to `Lt` and the like
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928968345
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and ', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
+ return id(self) == id(other) or (
+ type(self) == type(other) and self.term == other.term and self.literals == other.literals
+ )
-@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
Review Comment:
does `UnboundPredicate` need it? I think it's abstract still. It lacks `__invert__` too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#issuecomment-1194602870
> Thanks, @CircArgs! I think there are some changes that would help readability, but those are minor. I merged this so that other work can continue in parallel.
Thanks @rdblue. I'll open another PR soon with the changes hopefully
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928996271
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and ', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
+ return id(self) == id(other) or (
+ type(self) == type(other) and self.term == other.term and self.literals == other.literals
+ )
-@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
Review Comment:
Then we should mark it as an abstract class, or we can create stubs for the methods, and raise NotImplementedError: https://docs.python.org/3/library/exceptions.html#NotImplementedError. PyCharm is bugging me that the classes are missing :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928970742
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and ', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
+ return id(self) == id(other) or (
+ type(self) == type(other) and self.term == other.term and self.literals == other.literals
+ )
-@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
- term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: UnboundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> UnboundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and ', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
Review Comment:
I originally used dataclasses throughout a lot of this code, but had difficulties getting things to play nice with the validation of literals. I think the trouble comes from leveraging `__post_init__` in a similar way to how `__init__` is currently, but it seems child classes don't care about the parent's `__post_init__` hence why I switched to `__init__` and just work things with `super`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928961756
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
Review Comment:
I like it! I may be misremembering. I thought I had gotten pushback in a similar situation using `*args` but let me have another go
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928996862
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
Review Comment:
@Fokko Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r929007401
##########
python/pyiceberg/expressions/base.py:
##########
@@ -327,6 +347,54 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundIn[T]:
return BoundIn(bound_ref, tuple(lit.to(bound_ref.field.field_type) for lit in self.literals)) # type: ignore
+@dataclass(frozen=True)
+class BoundEQ(BoundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("EQ expressions do not support negation.")
+
+
+@dataclass(frozen=True)
+class EQ(UnboundPredicate[T]):
+ def __invert__(self):
+ raise TypeError("EQ expressions do not support negation.")
Review Comment:
now we have correct inverts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r929224219
##########
python/pyiceberg/expressions/base.py:
##########
@@ -282,49 +338,239 @@ def __repr__(self) -> str:
return f"Not({repr(self.child)})"
def __str__(self) -> str:
- return f"(not {self.child})"
+ return f"Not({str(self.child)})"
+@dataclass(frozen=True)
class AlwaysTrue(BooleanExpression, ABC, Singleton):
"""TRUE expression"""
- def __invert__(self) -> "AlwaysFalse":
+ def __invert__(self) -> AlwaysFalse:
return AlwaysFalse()
- def __repr__(self) -> str:
- return "AlwaysTrue()"
-
- def __str__(self) -> str:
- return "true"
-
+@dataclass(frozen=True)
class AlwaysFalse(BooleanExpression, ABC, Singleton):
"""FALSE expression"""
- def __invert__(self) -> "AlwaysTrue":
+ def __invert__(self) -> AlwaysTrue:
return AlwaysTrue()
- def __repr__(self) -> str:
- return "AlwaysFalse()"
- def __str__(self) -> str:
- return "false"
+class IsNull(UnboundPredicate[T]):
+ def __invert__(self) -> NotNull:
+ return NotNull(self.term)
+
+ def _validate_literals(self): # pylint: disable=W0238
+ if self.literals is not None:
+ raise AttributeError("Null is a unary predicate and takes no Literals.")
+
+ def bind(self, schema: Schema, case_sensitive: bool) -> BoundIsNull[T]:
+ bound_ref = self.term.bind(schema, case_sensitive)
Review Comment:
A better name would be `bound_term` since this is not necessarily a ref.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r929222844
##########
python/pyiceberg/expressions/base.py:
##########
@@ -209,14 +265,14 @@ def right(self) -> BooleanExpression:
def __eq__(self, other) -> bool:
return id(self) == id(other) or (isinstance(other, And) and self.left == other.left and self.right == other.right)
- def __invert__(self) -> "Or":
+ def __invert__(self) -> Or:
return Or(~self.left, ~self.right)
def __repr__(self) -> str:
return f"And({repr(self.left)}, {repr(self.right)})"
def __str__(self) -> str:
- return f"({self.left} and {self.right})"
+ return f"And({str(self.left)}, {str(self.right)})"
Review Comment:
I don't think this is correct. This is basically just `repr` output. I think this should match what Java produces, which is a readable infix string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928957892
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r929008834
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
Review Comment:
I did this while I was changing literals to an *args pattern :p
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#issuecomment-1194376304
@Fokko please have another look when you get a chance. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r929010032
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else (literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly 1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and ', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and ', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
+ return id(self) == id(other) or (
+ type(self) == type(other) and self.term == other.term and self.literals == other.literals
+ )
-@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
Review Comment:
interesting. I'm not getting any linting errors on it. I'll try though. Wondering if I'll get trouble about ambiguous metaclass stuff
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928991075
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] | Literal[T] | None = None):
Review Comment:
I think it would be nice for passing in the literal(s). We also use it in other places: https://github.com/apache/iceberg/blob/master/python/pyiceberg/schema.py#L63-L67 So I think we should be okay here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r929231501
##########
python/pyiceberg/expressions/base.py:
##########
@@ -282,49 +338,239 @@ def __repr__(self) -> str:
return f"Not({repr(self.child)})"
def __str__(self) -> str:
- return f"(not {self.child})"
+ return f"Not({str(self.child)})"
+@dataclass(frozen=True)
class AlwaysTrue(BooleanExpression, ABC, Singleton):
"""TRUE expression"""
- def __invert__(self) -> "AlwaysFalse":
+ def __invert__(self) -> AlwaysFalse:
return AlwaysFalse()
- def __repr__(self) -> str:
- return "AlwaysTrue()"
-
- def __str__(self) -> str:
- return "true"
-
+@dataclass(frozen=True)
class AlwaysFalse(BooleanExpression, ABC, Singleton):
"""FALSE expression"""
- def __invert__(self) -> "AlwaysTrue":
+ def __invert__(self) -> AlwaysTrue:
return AlwaysTrue()
- def __repr__(self) -> str:
- return "AlwaysFalse()"
- def __str__(self) -> str:
- return "false"
+class IsNull(UnboundPredicate[T]):
+ def __invert__(self) -> NotNull:
+ return NotNull(self.term)
+
+ def _validate_literals(self): # pylint: disable=W0238
+ if self.literals is not None:
+ raise AttributeError("Null is a unary predicate and takes no Literals.")
+
+ def bind(self, schema: Schema, case_sensitive: bool) -> BoundIsNull[T]:
+ bound_ref = self.term.bind(schema, case_sensitive)
+ return BoundIsNull(bound_ref)
+
+
+class BoundIsNull(BoundPredicate[T]):
+ def __invert__(self) -> BoundNotNull:
+ return BoundNotNull(self.term)
+
+ def _validate_literals(self): # pylint: disable=W0238
+ if self.literals:
+ raise AttributeError("Null is a unary predicate and takes no Literals.")
+
+
+class NotNull(UnboundPredicate[T]):
+ def __invert__(self) -> IsNull:
+ return IsNull(self.term)
+
+ def _validate_literals(self): # pylint: disable=W0238
+ if self.literals:
+ raise AttributeError("NotNull is a unary predicate and takes no Literals.")
+
+ def bind(self, schema: Schema, case_sensitive: bool) -> BoundNotNull[T]:
+ bound_ref = self.term.bind(schema, case_sensitive)
+ return BoundNotNull(bound_ref)
+
+
+class BoundNotNull(BoundPredicate[T]):
+ def __invert__(self) -> BoundIsNull:
+ return BoundIsNull(self.term)
+
+ def _validate_literals(self): # pylint: disable=W0238
+ if self.literals:
+ raise AttributeError("NotNull is a unary predicate and takes no Literals.")
+
+
+class IsNaN(UnboundPredicate[T]):
+ def __invert__(self) -> NotNaN:
+ return NotNaN(self.term)
+
+ def _validate_literals(self): # pylint: disable=W0238
+ if self.literals:
+ raise AttributeError("IsNaN is a unary predicate and takes no Literals.")
+
+ def bind(self, schema: Schema, case_sensitive: bool) -> BoundIsNaN[T]:
+ bound_ref = self.term.bind(schema, case_sensitive)
+ return BoundIsNaN(bound_ref)
+
+
+class BoundIsNaN(BoundPredicate[T]):
+ def __invert__(self) -> BoundNotNaN:
+ return BoundNotNaN(self.term)
+
+ def _validate_literals(self): # pylint: disable=W0238
+ if self.literals:
+ raise AttributeError("IsNaN is a unary predicate and takes no Literals.")
+
+
+class NotNaN(UnboundPredicate[T]):
+ def __invert__(self) -> IsNaN:
+ return IsNaN(self.term)
+
+ def _validate_literals(self): # pylint: disable=W0238
+ if self.literals:
+ raise AttributeError("NotNaN is a unary predicate and takes no Literals.")
+
+ def bind(self, schema: Schema, case_sensitive: bool) -> BoundNotNaN[T]:
+ bound_ref = self.term.bind(schema, case_sensitive)
+ return BoundNotNaN(bound_ref)
+
+
+class BoundNotNaN(BoundPredicate[T]):
+ def __invert__(self) -> BoundIsNaN:
+ return BoundIsNaN(self.term)
+
+ def _validate_literals(self): # pylint: disable=W0238
+ if self.literals:
+ raise AttributeError("NotNaN is a unary predicate and takes no Literals.")
-@dataclass(frozen=True)
class BoundIn(BoundPredicate[T]):
- def __invert__(self):
- raise TypeError("In expressions do not support negation.")
+ def _validate_literals(self): # pylint: disable=W0238
+ if not self.literals:
+ raise AttributeError("BoundIn must contain at least 1 literal.")
+
+ def __invert__(self) -> BoundNotIn[T]:
+ return BoundNotIn(self.term, *self.literals)
-@dataclass(frozen=True)
class In(UnboundPredicate[T]):
- def __invert__(self):
- raise TypeError("In expressions do not support negation.")
+ def _validate_literals(self): # pylint: disable=W0238
+ if not self.literals:
+ raise AttributeError("In must contain at least 1 literal.")
+
+ def __invert__(self) -> NotIn[T]:
+ return NotIn(self.term, *self.literals)
def bind(self, schema: Schema, case_sensitive: bool) -> BoundIn[T]:
bound_ref = self.term.bind(schema, case_sensitive)
- return BoundIn(bound_ref, tuple(lit.to(bound_ref.field.field_type) for lit in self.literals)) # type: ignore
+ return BoundIn(bound_ref, *tuple(lit.to(bound_ref.field.field_type) for lit in self.literals)) # type: ignore
+
+
+class BoundNotIn(BoundPredicate[T]):
+ def _validate_literals(self): # pylint: disable=W0238
+ if not self.literals:
+ raise AttributeError("BoundNotIn must contain at least 1 literal.")
+
+ def __invert__(self) -> BoundIn[T]:
+ return BoundIn(self.term, *self.literals)
+
+
+class NotIn(UnboundPredicate[T]):
+ def _validate_literals(self): # pylint: disable=W0238
+ if not self.literals:
+ raise AttributeError("NotIn must contain at least 1 literal.")
+
+ def __invert__(self) -> In[T]:
+ return In(self.term, *self.literals)
+
+ def bind(self, schema: Schema, case_sensitive: bool) -> BoundNotIn[T]:
+ bound_ref = self.term.bind(schema, case_sensitive)
+ return BoundNotIn(bound_ref, *tuple(lit.to(bound_ref.field.field_type) for lit in self.literals)) # type: ignore
+
+
+class BoundEq(BoundPredicate[T]):
+ def __invert__(self) -> BoundNotEq[T]:
+ return BoundNotEq(self.term, *self.literals)
+
+
+class Eq(UnboundPredicate[T]):
Review Comment:
Since we are using expression classes directly, rather than using factory methods, I think that these classes should use full names, like `Equals` and `LessThan` (or `BoundEqual` and `BoundLessThan`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#issuecomment-1194534450
Thanks, @CircArgs! I think there are some changes that would help readability, but those are minor. I merged this so that other work can continue in parallel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5258:
URL: https://github.com/apache/iceberg/pull/5258
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #5258: More Python Expressions
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r929006938
##########
python/pyiceberg/expressions/base.py:
##########
@@ -172,13 +178,33 @@ def bind(self, schema: Schema, case_sensitive: bool) -> BoundReference[T]:
@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
+ object.__setattr__(self, "literals", (self.literals,))
+ if not (self.literals_len_lb <= len(self.literals) <= (self.literals_len_ub or float("inf"))): # type: ignore
+ raise AttributeError(
+ f"{self.__class__} expects a number of literals between at least {self.literals_len_lb} and at most {self.literals_len_ub or float('inf')}"
+ )
@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ literals: Union[Tuple[Literal[T], ...], Literal[T]]
+ literals_len_lb: int = field(init=False, repr=False, hash=False, compare=False, default=1)
+ literals_len_ub: Optional[int] = field(init=False, repr=False, hash=False, compare=False, default=1)
+
+ def __post_init__(self):
+ if not isinstance(self.literals, tuple):
Review Comment:
no longer relevant
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org