-
Notifications
You must be signed in to change notification settings - Fork 338
Update StrictProjection tests #1422
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
Conversation
This aligns more closely with Java, and is also easier to read.
@@ -985,7 +985,7 @@ def _truncate_number_strict( | |||
elif isinstance(pred, BoundGreaterThanOrEqual): | |||
return GreaterThan(Reference(name), _transform_literal(transform, boundary.decrement())) # type: ignore | |||
elif isinstance(pred, BoundNotEqualTo): | |||
return EqualTo(Reference(name), _transform_literal(transform, boundary)) | |||
return NotEqualTo(Reference(name), _transform_literal(transform, boundary)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for putting this PR together @Fokko !
I have a nit comment as I found the expected_type
in the tests a bit confusing, but other than that LGTM
) -> None: | ||
result = transform.strict_project(name="name", pred=pred) | ||
|
||
assert type(result) is expected_type or AlwaysFalse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I found it a bit confusing that we were defining the expected_type as AlwaysFalse
when actually None
was being returned by strict_project
, but maybe the alternative of making expected_type
Optional
is more confusing 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let me fix that in a follow up PR 👍
_assert_projection_strict(BoundLessThan(term=bound_reference_date, literal=date), transform, LessThan, "1970-01") | ||
_assert_projection_strict(BoundLessThanOrEqual(term=bound_reference_date, literal=date), transform, LessThan, "1970-01") | ||
_assert_projection_strict(BoundGreaterThan(term=bound_reference_date, literal=date), transform, GreaterThan, "1970-01") | ||
_assert_projection_strict(BoundGreaterThanOrEqual(term=bound_reference_date, literal=date), transform, GreaterThan, "1969-12") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Fokko, I'm still confused as to why it's not be
_assert_projection_strict(BoundGreaterThanOrEqual(term=bound_reference_date, literal=date), transform, GreaterThan, "1970-01")
here.
#539 (comment), It looks like a bug that is not limited to the negative value.
Let's assume predicate date >= 1980-02-02
, strict projection for month partition will project it to month(date)>1980-01
, assume date is 1980-02-01
, then the strict projection predicate will be true month(1980-02-01) == 1980-02 > 1980-01
, but original predicate is false. 1980-02-01 < 1980-02-02
.
So I think we should not decrement here, date >= 1980-02-02
should be strict project to month(date) > 1980-02
🤔
iceberg-python/pyiceberg/transforms.py
Line 986 in b981780
return GreaterThan(Reference(name), _transform_literal(transform, boundary.decrement())) # type: ignore |
This aligns more closely with Java, and is also easier to read.
This aligns more closely with Java, and is also easier to read.