Fokko commented on code in PR #379:
URL: https://github.com/apache/iceberg-python/pull/379#discussion_r1479986435
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1332,10 +1332,22 @@ def serialize(self, value: Any) -> bytes:
return to_bytes(self.primitive_type, value)
def update_min(self, val: Any) -> None:
- self.current_min = val if self.current_min is None else min(val,
self.current_min)
+ if self.current_min is None:
+ self.current_min = val
+ elif val is None:
+ # keep current_min
+ return
+ else:
+ self.current_min = min(val, self.current_min)
def update_max(self, val: Any) -> None:
- self.current_max = val if self.current_max is None else max(val,
self.current_max)
+ if self.current_max is None:
+ self.current_max = val
+ elif val is None:
+ # keep current_max
+ return
+ else:
+ self.current_max = max(val, self.current_max)
Review Comment:
How about avoiding the `return`:
```suggestion
if self.current_max is None:
self.current_max = val
elif val is not None:
self.current_max = max(val, self.current_max)
```
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1332,10 +1332,22 @@ def serialize(self, value: Any) -> bytes:
return to_bytes(self.primitive_type, value)
def update_min(self, val: Any) -> None:
Review Comment:
```suggestion
def update_min(self, val: Optional[Any]) -> None:
```
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1332,10 +1332,22 @@ def serialize(self, value: Any) -> bytes:
return to_bytes(self.primitive_type, value)
def update_min(self, val: Any) -> None:
- self.current_min = val if self.current_min is None else min(val,
self.current_min)
+ if self.current_min is None:
+ self.current_min = val
+ elif val is None:
+ # keep current_min
+ return
+ else:
+ self.current_min = min(val, self.current_min)
Review Comment:
```suggestion
if self.current_min is None:
self.current_min = val
elif val is not None:
self.current_min = min(val, self.current_min)
```
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1332,10 +1332,22 @@ def serialize(self, value: Any) -> bytes:
return to_bytes(self.primitive_type, value)
def update_min(self, val: Any) -> None:
- self.current_min = val if self.current_min is None else min(val,
self.current_min)
+ if self.current_min is None:
+ self.current_min = val
+ elif val is None:
+ # keep current_min
+ pass
+ else:
+ self.current_min = min(val, self.current_min)
def update_max(self, val: Any) -> None:
Review Comment:
This type definition is actually wrong:
```suggestion
def update_max(self, val: Optional[Any]) -> None:
```
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
