You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ebyhr (via GitHub)" <gi...@apache.org> on 2026/06/14 04:48:52 UTC
[PR] Fix NotStartsWith residual evaluation to return correct result [iceberg-python]
ebyhr opened a new pull request, #3503:
URL: https://github.com/apache/iceberg-python/pull/3503
# Rationale for this change
- Fixes #3496
## Are these changes tested?
Yes - the added test fails if we revert this PR's change.
## Are there any user-facing changes?
Yes - this PR fixes a correctness issue of `NotStartsWith`.
--
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
Re: [PR] Fix NotStartsWith residual evaluation to return correct result [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3503:
URL: https://github.com/apache/iceberg-python/pull/3503#issuecomment-4763246681
Thanks for the PR @ebyhr and thanks for the review @rambleraptor
--
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
Re: [PR] Fix NotStartsWith residual evaluation to return correct result [iceberg-python]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #3503:
URL: https://github.com/apache/iceberg-python/pull/3503#discussion_r3416162417
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1905,10 +1905,11 @@ def visit_starts_with(self, term: BoundTerm, literal: LiteralValue) -> BooleanEx
return AlwaysFalse()
def visit_not_starts_with(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
- if not self.visit_starts_with(term, literal):
- return AlwaysTrue()
- else:
+ starts_with_result = self.visit_starts_with(term, literal)
+ if isinstance(starts_with_result, AlwaysTrue):
Review Comment:
```suggestion
# Should return opposite of visit_starts_with
if isinstance(starts_with_result, AlwaysTrue):
```
This took me a few seconds to understand why we're returning the opposite. I think this helps with readability a lot.
##########
tests/expressions/test_residual_evaluator.py:
##########
@@ -249,3 +249,16 @@ def test_not_in_timestamp() -> None:
ts_day += 3 # type: ignore
residual = res_eval.residual_for(Record(ts_day))
assert residual == AlwaysTrue()
+
+
+def test_not_starts_with() -> None:
Review Comment:
While you're here, can you right the same test for `StartsWith`? It would be good to have a future sanity check that we're not making the same mistake twice.
--
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
Re: [PR] Fix NotStartsWith residual evaluation to return correct result [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu merged PR #3503:
URL: https://github.com/apache/iceberg-python/pull/3503
--
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
Re: [PR] Fix NotStartsWith residual evaluation to return correct result [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3503:
URL: https://github.com/apache/iceberg-python/pull/3503#issuecomment-4763193701
looks like linter is failing because i pushed a bad commit (https://github.com/apache/iceberg-python/commit/fe7c749229be6a439a13371348ab741bba6fc940)
going to use this PR to fix forward
--
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
Re: [PR] Fix NotStartsWith residual evaluation to return correct result [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #3503:
URL: https://github.com/apache/iceberg-python/pull/3503#discussion_r3448998093
##########
pyiceberg/expressions/visitors.py:
##########
@@ -1905,10 +1905,12 @@ def visit_starts_with(self, term: BoundTerm, literal: LiteralValue) -> BooleanEx
return AlwaysFalse()
def visit_not_starts_with(self, term: BoundTerm, literal: LiteralValue) -> BooleanExpression:
- if not self.visit_starts_with(term, literal):
- return AlwaysTrue()
- else:
+ starts_with_result = self.visit_starts_with(term, literal)
+ # Should return opposite of visit_starts_with
+ if isinstance(starts_with_result, AlwaysTrue):
return AlwaysFalse()
+ else:
+ return AlwaysTrue()
Review Comment:
```suggestion
return ~self.visit_starts_with(term, literal)
```
might be cleaner to rely on `AlwaysTrue`/`AlwaysFalse`'s `__invert__` function
--
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
Re: [PR] Fix NotStartsWith residual evaluation to return correct result [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3503:
URL: https://github.com/apache/iceberg-python/pull/3503#issuecomment-4763176953
While this PR can use expression negation via `__invert__,` I think it may be worth supporting Python truthiness for the constant expressions in a follow-up:
```
class AlwaysTrue:
def __bool__(self) -> bool:
return True
class AlwaysFalse:
def __bool__(self) -> bool:
return False
```
That would make patterns like this behave intuitively:
```
if not self.visit_starts_with(term, literal):
return AlwaysTrue()
```
When adding this, we should also be careful that Python truthiness is for control flow, not expression construction: `not expr` returns a Python `bool`, while `~expr` returns a negated BooleanExpression. It would probably be safest for non-constant `BooleanExpression.__bool__` to raise, with only `AlwaysTrue` and `AlwaysFalse` overriding it. The follow-up should also audit optional-expression checks like `expr or AlwaysTrue()` and REST residual/filter handling so explicit `AlwaysFalse()` values are not treated as 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