[GitHub] [iceberg] Fokko commented on a diff in pull request #6892: Python: `starts_with` and `not_starts_with` expressions

["Fokko (via GitHub)" <gi...@apache.org>]

Fokko commented on code in PR #6892:
URL: https://github.com/apache/iceberg/pull/6892#discussion_r1116079239


##########
python/tests/expressions/test_parser.py:
##########
@@ -149,3 +151,13 @@ def test_and_or_with_parens() -> None:
     assert Or(IsNull("x"), And(GreaterThanOrEqual("x", 5), Not(LessThan("x", 10)))) == parser.parse(
         "(x is null) or (5 <= x) and not(x < 10)"
     )
+
+
+def test_starts_with() -> None:
+    assert StartsWith("x", "data") == parser.parse("x starts_with 'data'")

Review Comment:
   I don't think `starts_with` is common in SQL. How about:
   ```suggestion
       assert StartsWith("x", "data") == parser.parse("x LIKE 'data*'")
   ```



##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -943,6 +987,12 @@ def visit_less_than(self, term: BoundTerm[L], literal: Literal[L]) -> List[Tuple
     def visit_less_than_or_equal(self, term: BoundTerm[L], literal: Literal[L]) -> List[Tuple[str, str, Any]]:
         return [(term.ref().field.name, "<=", self._cast_if_necessary(term.ref().field.field_type, literal.value))]
 
+    def visit_starts_with(self, term: BoundTerm[L], literal: Literal[L]) -> List[Tuple[str, str, Any]]:
+        return [(term.ref().field.name, "starts_with", self._cast_if_necessary(term.ref().field.field_type, literal.value))]

Review Comment:
   I don't think that this is a supported predicate: https://github.com/apache/arrow/blob/45918a90a6ca1cf3fd67c256a7d6a240249e555a/python/pyarrow/parquet/core.py#L126-L131
   
   We can just return an empty predicate for now:
   ```suggestion
           return []
   ```
   This visitor isn't used right now but might be in the future for Dask to implement page skipping. Keeping it empty will mean that we don't use it for skipping pages, but we have to filter on a row level afterward anyway.



##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -675,6 +701,24 @@ def visit_less_than_or_equal(self, term: BoundTerm[L], literal: Literal[L]) -> b
 
         return ROWS_MIGHT_MATCH
 
+    def visit_starts_with(self, term: BoundTerm[L], literal: Literal[L]) -> bool:

Review Comment:
   Same for the ManifestEvaluator, can you copy the tests from Java: https://github.com/apache/iceberg/blob/master/api/src/test/java/org/apache/iceberg/expressions/TestInclusiveManifestEvaluator.java#L538-L627



-- 
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