geruh commented on code in PR #3293:
URL: https://github.com/apache/iceberg-python/pull/3293#discussion_r3171454168
##########
tests/expressions/test_evaluator.py:
##########
@@ -1463,3 +1471,87 @@ def test_strict_integer_not_in(strict_data_file_schema:
Schema, strict_data_file
should_read = _StrictMetricsEvaluator(strict_data_file_schema,
NotIn("no_nulls", {"abc", "def"})).eval(strict_data_file_1)
assert not should_read, "Should not match: no_nulls field does not have
bounds"
+
+
[email protected](
+ "file_type, evolved_type, lower_bound, upper_bound, op, lit, expected",
+ [
+ # Int -> Long
+ (IntegerType(), LongType(), 30, 79, GreaterThan, 100,
ROWS_CANNOT_MATCH),
+ (IntegerType(), LongType(), 30, 79, LessThan, 50, ROWS_MIGHT_MATCH),
+ # Float -> Double
+ (FloatType(), DoubleType(), 30.0, 79.0, GreaterThan, 100.0,
ROWS_CANNOT_MATCH),
+ (FloatType(), DoubleType(), 30.0, 79.0, LessThan, 50.0,
ROWS_MIGHT_MATCH),
+ ],
+)
+def test_inclusive_metrics_evaluator_with_type_promotion(
+ file_type: PrimitiveType,
+ evolved_type: PrimitiveType,
+ lower_bound: Any,
+ upper_bound: Any,
+ op: Any,
+ lit: Any,
+ expected: bool,
+) -> None:
+ # Schema defines 'col' with evolved state
Review Comment:
nitty nit: I feel like the comments here are unnecessary.
##########
pyiceberg/conversions.py:
##########
@@ -353,13 +352,24 @@ def _(_: PrimitiveType, b: bytes) -> int:
return _LONG_STRUCT.unpack(b)[0]
+@from_bytes.register(LongType)
+def _(_: PrimitiveType, b: bytes) -> int:
+ if len(b) == 4:
Review Comment:
nit: just curious here when i linked the comment earlier to the java code I
noticed they use `< 8` any reason for the `== 4` on these?
##########
tests/expressions/test_evaluator.py:
##########
@@ -1463,3 +1471,87 @@ def test_strict_integer_not_in(strict_data_file_schema:
Schema, strict_data_file
should_read = _StrictMetricsEvaluator(strict_data_file_schema,
NotIn("no_nulls", {"abc", "def"})).eval(strict_data_file_1)
assert not should_read, "Should not match: no_nulls field does not have
bounds"
+
+
[email protected](
+ "file_type, evolved_type, lower_bound, upper_bound, op, lit, expected",
+ [
+ # Int -> Long
+ (IntegerType(), LongType(), 30, 79, GreaterThan, 100,
ROWS_CANNOT_MATCH),
+ (IntegerType(), LongType(), 30, 79, LessThan, 50, ROWS_MIGHT_MATCH),
+ # Float -> Double
+ (FloatType(), DoubleType(), 30.0, 79.0, GreaterThan, 100.0,
ROWS_CANNOT_MATCH),
+ (FloatType(), DoubleType(), 30.0, 79.0, LessThan, 50.0,
ROWS_MIGHT_MATCH),
+ ],
+)
+def test_inclusive_metrics_evaluator_with_type_promotion(
Review Comment:
nit non blocking: lets name after teh assertion not narrative:
`test_inclusive_metrics_eval_bounds_after_promotion` same for below
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
