You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/09/23 20:19:32 UTC
[GitHub] [iceberg] Fokko opened a new pull request, #5845: Python: Manifest evaluator
Fokko opened a new pull request, #5845:
URL: https://github.com/apache/iceberg/pull/5845
Manifest evaluator to filter the files that need to be loaded.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985005902
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all([upper > val.value for val in literals]):
Review Comment:
I think this is also reversed.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r989543850
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +875,196 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
Review Comment:
It's okay to combine these, but it does seem a little strange to expose so much in the public `ManifestEvaluator`. I somewhat prefer using a separate `_ManifestEvalVisitor` class.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985005830
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
Review Comment:
Should this start with `_`?
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r989543235
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +875,196 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True):
Review Comment:
It is unlikely that this is going to be a `Schema` because it will be the partition type produced by #5929.
I think what we want instead is to pass the Schema and the PartitionSpec to produce the type that the expression should be bound to.
Confusing a data filter (bound to Schema) with a partition filter (bound to partition type) is something that we want to avoid and is why there are static factory methods in the Java code. This is okay, but we will need to be careful here.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985005639
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
Review Comment:
This seems backwards to me. If the lower bound is less than _any_ value, then this can't be used to eliminate the in predicate.
I think the problem is that the Java code uses `filter` and then checks for empty. It collects all values that are greater than or equal to the lower bound and then returns rows cannot match if there were any.
We'll need to make sure there's a test for this.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985002904
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
Review Comment:
What do you mean here?
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r993706408
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +877,205 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class _ManifestEvalVisitor(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, partition_struct_schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True):
+ bound_partition_filter = partition_filter.bind(partition_struct_schema, case_sensitive)
+ self.partition_filter = rewrite_not(bound_partition_filter)
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all(lower > val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all(upper < val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is True and field.contains_null is False and field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if self.partition_fields[pos].contains_null is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ # contains_null encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = self.partition_fields[pos].contains_null is True and self.partition_fields[pos].lower_bound is None
+
+ if all_null and type(term.ref().field.field_type) in {DoubleType, FloatType}:
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, contains_nan needs to be checked against.
+ all_null = self.partition_fields[pos].contains_nan is False
+
+ if all_null:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ # values are all null and literal cannot contain null
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if lower > literal.value:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notEq(col, X) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value >= upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value <= lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value < lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_true(self) -> bool:
+ return ROWS_MIGHT_MATCH
+
+ def visit_false(self) -> bool:
+ return ROWS_CANNOT_MATCH
+
+ def visit_not(self, child_result: bool) -> bool:
+ return not child_result
+
+ def visit_and(self, left_result: bool, right_result: bool) -> bool:
+ return left_result and right_result
+
+ def visit_or(self, left_result: bool, right_result: bool) -> bool:
+ return left_result or right_result
+
+
+def manifest_evaluator(
+ partition_spec: PartitionSpec, schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True
Review Comment:
The partition filter should be a `BooleanExpression`, not an `UnboundPredicate`
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985005902
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all([upper > val.value for val in literals]):
Review Comment:
I think this is also reversed: it should be "if the upper bound is less than all literal values, rows cannot match".
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r989733773
##########
python/tests/expressions/test_expressions_base.py:
##########
@@ -1368,6 +1381,841 @@ def test_bound_boolean_expression_visitor_raise_on_unbound_predicate():
assert "Not a bound predicate" in str(exc_info.value)
+def _to_byte_buffer(field_type: IcebergType, val: Any):
Review Comment:
Yes, I like that. Splitting out `base.py` is on my list, but didn't want to do that in a PR that introduces new functionality.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985005639
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
Review Comment:
This seems backwards to me. If the lower bound is less than _any_ value, then this can't be used to eliminate the in predicate.
I think the problem is that the Java code uses `filter` and then checks for empty. It collects all values that are greater than or equal to the lower bound and then returns rows cannot match if there were any.
--
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
[GitHub] [iceberg] rdblue merged pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5845:
URL: https://github.com/apache/iceberg/pull/5845
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r989955442
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +875,196 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
Review Comment:
Yes, I went a bit back and forth on this. I don't like having a setter that sets something internal (`partition_fields`). I now created a function that returns a Callable, that hides the visitor:
```python
def manifest_evaluator(
schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True
) -> Callable[[ManifestFile], bool]:
evaluator = _ManifestEvalVisitor(schema, partition_filter, case_sensitive)
return evaluator.eval
```
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r993706996
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +877,205 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class _ManifestEvalVisitor(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, partition_struct_schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True):
+ bound_partition_filter = partition_filter.bind(partition_struct_schema, case_sensitive)
+ self.partition_filter = rewrite_not(bound_partition_filter)
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all(lower > val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all(upper < val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is True and field.contains_null is False and field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if self.partition_fields[pos].contains_null is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ # contains_null encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = self.partition_fields[pos].contains_null is True and self.partition_fields[pos].lower_bound is None
+
+ if all_null and type(term.ref().field.field_type) in {DoubleType, FloatType}:
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, contains_nan needs to be checked against.
+ all_null = self.partition_fields[pos].contains_nan is False
+
+ if all_null:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ # values are all null and literal cannot contain null
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if lower > literal.value:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notEq(col, X) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value >= upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value <= lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value < lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_true(self) -> bool:
+ return ROWS_MIGHT_MATCH
+
+ def visit_false(self) -> bool:
+ return ROWS_CANNOT_MATCH
+
+ def visit_not(self, child_result: bool) -> bool:
+ return not child_result
+
+ def visit_and(self, left_result: bool, right_result: bool) -> bool:
+ return left_result and right_result
+
+ def visit_or(self, left_result: bool, right_result: bool) -> bool:
+ return left_result or right_result
+
+
+def manifest_evaluator(
+ partition_spec: PartitionSpec, schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True
+) -> Callable[[ManifestFile], bool]:
+ partition_schema = Schema(*partition_spec.partition_type(schema))
Review Comment:
I don't think it should be necessary to create a `Schema` from the `StructType`.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985003516
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
Review Comment:
Looks correct to me.
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r989732854
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +875,196 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True):
Review Comment:
Yes, this was a temp solution to test the binding 👍🏻 Will be replaced once #5929 has been merged
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r989955442
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +875,196 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
Review Comment:
Yes, I went a bit back and forth on this. I don't like having a setter that sets something very internal. I now created a function that returns a Callable, that hides the visitor:
```python
def manifest_evaluator(
schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True
) -> Callable[[ManifestFile], bool]:
evaluator = _ManifestEvalVisitor(schema, partition_filter, case_sensitive)
return evaluator.eval
```
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985003023
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all([upper > val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if self.partition_fields[pos].contains_nan is False:
+ return ROWS_CANNOT_MATCH
+
+ if all_values_are_null(field, term.ref().field.field_type):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is True and field.contains_null is False and field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if self.partition_fields[pos].contains_null is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if all_values_are_null(self.partition_fields[pos], term.ref().field.field_type):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ # values are all null and literal cannot contain null
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if lower > literal.value:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notEq(col, X) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value >= upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value <= lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value < lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_true(self) -> bool:
+ return True
Review Comment:
ROWS_MIGHT_MATCH?
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all([upper > val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if self.partition_fields[pos].contains_nan is False:
+ return ROWS_CANNOT_MATCH
+
+ if all_values_are_null(field, term.ref().field.field_type):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is True and field.contains_null is False and field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if self.partition_fields[pos].contains_null is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if all_values_are_null(self.partition_fields[pos], term.ref().field.field_type):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ # values are all null and literal cannot contain null
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if lower > literal.value:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notEq(col, X) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value >= upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value <= lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value < lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_true(self) -> bool:
+ return True
+
+ def visit_false(self) -> bool:
+ return False
Review Comment:
ROWS_CANNOT_MATCH?
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r993979867
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +897,204 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class _ManifestEvalVisitor(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, partition_struct_schema: Schema, partition_filter: BooleanExpression, case_sensitive: bool = True):
+ self.partition_filter = bind(partition_struct_schema, rewrite_not(partition_filter), case_sensitive)
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all(lower > val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all(upper < val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is True and field.contains_null is False and field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if self.partition_fields[pos].contains_null is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ # contains_null encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = self.partition_fields[pos].contains_null is True and self.partition_fields[pos].lower_bound is None
+
+ if all_null and type(term.ref().field.field_type) in {DoubleType, FloatType}:
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, contains_nan needs to be checked against.
+ all_null = self.partition_fields[pos].contains_nan is False
+
+ if all_null:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ # values are all null and literal cannot contain null
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if lower > literal.value:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notEq(col, X) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value >= upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value <= lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value < lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_true(self) -> bool:
+ return ROWS_MIGHT_MATCH
+
+ def visit_false(self) -> bool:
+ return ROWS_CANNOT_MATCH
+
+ def visit_not(self, child_result: bool) -> bool:
+ return not child_result
+
+ def visit_and(self, left_result: bool, right_result: bool) -> bool:
+ return left_result and right_result
+
+ def visit_or(self, left_result: bool, right_result: bool) -> bool:
+ return left_result or right_result
+
+
+def manifest_evaluator(
+ partition_spec: PartitionSpec, schema: Schema, partition_filter: BooleanExpression, case_sensitive: bool = True
+) -> Callable[[ManifestFile], bool]:
+ partition_schema = Schema(*partition_spec.partition_type(schema))
Review Comment:
Curious: How does exploding the partition type work? Some PEP 636 magic?
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r993823074
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +877,205 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class _ManifestEvalVisitor(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, partition_struct_schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True):
+ bound_partition_filter = partition_filter.bind(partition_struct_schema, case_sensitive)
Review Comment:
Ah I see, I've updated the code 👍🏻
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r990833562
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +876,203 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class _ManifestEvalVisitor(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True):
+ self.partition_filter = rewrite_not(partition_filter.bind(schema, case_sensitive))
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all(lower > val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all(upper < val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is True and field.contains_null is False and field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if self.partition_fields[pos].contains_null is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ # contains_null encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = self.partition_fields[pos].contains_null is True and self.partition_fields[pos].lower_bound is None
+
+ if all_null and type(term.ref().field.field_type) in {DoubleType, FloatType}:
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, contains_nan needs to be checked against.
+ all_null = self.partition_fields[pos].contains_nan is False
+
+ if all_null:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ # values are all null and literal cannot contain null
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if lower > literal.value:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notEq(col, X) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value >= upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value <= lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value < lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_true(self) -> bool:
+ return ROWS_MIGHT_MATCH
+
+ def visit_false(self) -> bool:
+ return ROWS_CANNOT_MATCH
+
+ def visit_not(self, child_result: bool) -> bool:
+ return not child_result
+
+ def visit_and(self, left_result: bool, right_result: bool) -> bool:
+ return left_result and right_result
+
+ def visit_or(self, left_result: bool, right_result: bool) -> bool:
+ return left_result or right_result
+
+
+def manifest_evaluator(
+ schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True
Review Comment:
Now that #5929 is in, can you fix this so that it accepts a `PartitionSpec` and uses the partition type?
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985498605
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
Review Comment:
It a good that I've added that comment. I don't really like setting a class variable over there. I've split it out into a separate class.
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all([upper > val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if self.partition_fields[pos].contains_nan is False:
+ return ROWS_CANNOT_MATCH
+
+ if all_values_are_null(field, term.ref().field.field_type):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is True and field.contains_null is False and field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if self.partition_fields[pos].contains_null is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if all_values_are_null(self.partition_fields[pos], term.ref().field.field_type):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ # values are all null and literal cannot contain null
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if lower > literal.value:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notEq(col, X) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value >= upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value <= lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value < lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_true(self) -> bool:
+ return True
Review Comment:
Nice, thanks!
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all([upper > val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if self.partition_fields[pos].contains_nan is False:
Review Comment:
It should, thanks!
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
Review Comment:
Good call
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
Review Comment:
Ahh, great catch! It is flipped indeed, thanks! A test has been added.
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
Review Comment:
I implemented this using the `all` method since it will stop the loop early when a `False` has been encountered.
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all([upper > val.value for val in literals]):
Review Comment:
At least it is consistently reversed, thanks! Updated and added a test 👍🏻
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r985003785
##########
python/pyiceberg/expressions/base.py:
##########
@@ -835,3 +843,204 @@ def _(expr: BoundLessThan, visitor: BoundBooleanExpressionVisitor[T]) -> T:
@visit_bound_predicate.register(BoundLessThanOrEqual)
def _(expr: BoundLessThanOrEqual, visitor: BoundBooleanExpressionVisitor[T]) -> T:
return visitor.visit_less_than_or_equal(term=expr.term, literal=expr.literal)
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def all_values_are_null(partition_field: PartitionFieldSummary, field_type: IcebergType) -> bool:
+ # containsNull encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = partition_field.contains_null is True and partition_field.lower_bound is None
+
+ if all_null and (field_type is DoubleType or field_type is FloatType):
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, containsNaN needs to be checked against.
+ return partition_field.contains_nan is False
+
+ return all_null
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class ManifestEvaluator(BoundBooleanExpressionVisitor[bool]):
+ partition_filter: BooleanExpression
+ partition_fields: list[PartitionFieldSummary]
+
+ def __init__(self, partition_filter: BooleanExpression):
+ self.partition_filter = partition_filter
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ # this is not what we want
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all([lower < val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all([upper > val.value for val in literals]):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if self.partition_fields[pos].contains_nan is False:
Review Comment:
Should this use `field` from above?
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r989544775
##########
python/tests/expressions/test_expressions_base.py:
##########
@@ -1368,6 +1381,841 @@ def test_bound_boolean_expression_visitor_raise_on_unbound_predicate():
assert "Not a bound predicate" in str(exc_info.value)
+def _to_byte_buffer(field_type: IcebergType, val: Any):
Review Comment:
`base` is getting a bit big. Can we refactor it soon?
* `__init__.py` with the unbound expression classes
* `visitors.py` with visitor methods and base classes
* `bound.py` with bound expression classes (and maybe the binding-related visitors?)
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r990841852
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +876,203 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class _ManifestEvalVisitor(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True):
+ self.partition_filter = rewrite_not(partition_filter.bind(schema, case_sensitive))
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all(lower > val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all(upper < val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is True and field.contains_null is False and field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if self.partition_fields[pos].contains_null is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ # contains_null encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = self.partition_fields[pos].contains_null is True and self.partition_fields[pos].lower_bound is None
+
+ if all_null and type(term.ref().field.field_type) in {DoubleType, FloatType}:
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, contains_nan needs to be checked against.
+ all_null = self.partition_fields[pos].contains_nan is False
+
+ if all_null:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ # values are all null and literal cannot contain null
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if lower > literal.value:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notEq(col, X) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value >= upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value <= lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value < lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_true(self) -> bool:
+ return ROWS_MIGHT_MATCH
+
+ def visit_false(self) -> bool:
+ return ROWS_CANNOT_MATCH
+
+ def visit_not(self, child_result: bool) -> bool:
+ return not child_result
+
+ def visit_and(self, left_result: bool, right_result: bool) -> bool:
+ return left_result and right_result
+
+ def visit_or(self, left_result: bool, right_result: bool) -> bool:
+ return left_result or right_result
+
+
+def manifest_evaluator(
+ schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True
Review Comment:
I've added `partition_spec` to the argument list. We also need the current `schema` as it is an argument of `partition_type`. I've removed the schema as a parameter from the Visitor itself. Tomorrow I'll pick up the `TableScan` that will wire everything together 👍🏻
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r993709920
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +877,205 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class _ManifestEvalVisitor(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, partition_struct_schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True):
+ bound_partition_filter = partition_filter.bind(partition_struct_schema, case_sensitive)
+ self.partition_filter = rewrite_not(bound_partition_filter)
+
+ def eval(self, manifest: ManifestFile) -> bool:
+ if partitions := manifest.partitions:
+ self.partition_fields = partitions
+ return visit(self.partition_filter, self)
+
+ # No partition information
+ return ROWS_MIGHT_MATCH
+
+ def visit_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ if len(literals) > IN_PREDICATE_LIMIT:
+ return ROWS_MIGHT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if all(lower > val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ if field.upper_bound is not None:
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+ if all(upper < val.value for val in literals):
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_in(self, term: BoundTerm, literals: set[Literal[Any]]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_nan(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.contains_nan is True and field.contains_null is False and field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_is_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ if self.partition_fields[pos].contains_null is False:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_null(self, term: BoundTerm) -> bool:
+ pos = term.ref().accessor.position
+
+ # contains_null encodes whether at least one partition value is null,
+ # lowerBound is null if all partition values are null
+ all_null = self.partition_fields[pos].contains_null is True and self.partition_fields[pos].lower_bound is None
+
+ if all_null and type(term.ref().field.field_type) in {DoubleType, FloatType}:
+ # floating point types may include NaN values, which we check separately.
+ # In case bounds don't include NaN value, contains_nan needs to be checked against.
+ all_null = self.partition_fields[pos].contains_nan is False
+
+ if all_null:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ # values are all null and literal cannot contain null
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if lower > literal.value:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_not_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ # because the bounds are not necessarily a min or max value, this cannot be answered using
+ # them. notEq(col, X) with (X, Y) doesn't guarantee that X is a value in col.
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value > upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_greater_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.upper_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
+
+ if literal.value >= upper:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value <= lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_less_than_or_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
+ pos = term.ref().accessor.position
+ field = self.partition_fields[pos]
+
+ if field.lower_bound is None:
+ return ROWS_CANNOT_MATCH
+
+ lower = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+
+ if literal.value < lower:
+ return ROWS_CANNOT_MATCH
+
+ return ROWS_MIGHT_MATCH
+
+ def visit_true(self) -> bool:
+ return ROWS_MIGHT_MATCH
+
+ def visit_false(self) -> bool:
+ return ROWS_CANNOT_MATCH
+
+ def visit_not(self, child_result: bool) -> bool:
+ return not child_result
+
+ def visit_and(self, left_result: bool, right_result: bool) -> bool:
+ return left_result and right_result
+
+ def visit_or(self, left_result: bool, right_result: bool) -> bool:
+ return left_result or right_result
+
+
+def manifest_evaluator(
+ partition_spec: PartitionSpec, schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True
+) -> Callable[[ManifestFile], bool]:
+ partition_schema = Schema(*partition_spec.partition_type(schema))
Review Comment:
Oh, I see. The binding uses `find` methods on `Schema` to avoid traversing types. This should be fine.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r993976843
##########
python/pyiceberg/expressions/base.py:
##########
@@ -654,17 +664,37 @@ def _(obj: Or, visitor: BooleanExpressionVisitor[T]) -> T:
return visitor.visit_or(left_result=left_result, right_result=right_result)
+def bind(schema: Schema, expression: BooleanExpression, case_sensitive: bool) -> BooleanExpression:
Review Comment:
Minor: we usually add a default for `case_sensitive`. I think it's a good idea to require it for internal methods, but this will probably be called from many different places so I'd default it here.
--
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
[GitHub] [iceberg] rdblue commented on pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#issuecomment-1276843981
Nice work, @Fokko! Thanks for getting this done.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5845: Python: Manifest evaluator
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5845:
URL: https://github.com/apache/iceberg/pull/5845#discussion_r993710340
##########
python/pyiceberg/expressions/base.py:
##########
@@ -867,3 +877,205 @@ def visit_unbound_predicate(self, predicate) -> BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
+
+
+ROWS_MIGHT_MATCH = True
+ROWS_CANNOT_MATCH = False
+IN_PREDICATE_LIMIT = 200
+
+
+def _from_byte_buffer(field_type: IcebergType, val: bytes):
+ if not isinstance(field_type, PrimitiveType):
+ raise ValueError(f"Expected a PrimitiveType, got: {type(field_type)}")
+ return from_bytes(field_type, val)
+
+
+class _ManifestEvalVisitor(BoundBooleanExpressionVisitor[bool]):
+ partition_fields: list[PartitionFieldSummary]
+ partition_filter: BooleanExpression
+
+ def __init__(self, partition_struct_schema: Schema, partition_filter: UnboundPredicate, case_sensitive: bool = True):
+ bound_partition_filter = partition_filter.bind(partition_struct_schema, case_sensitive)
Review Comment:
This shouldn't call `bind` on the expression. It should use the `BindVisitor` to bind to the partition type.
I think it just wasn't obvious how to bind an expression because there isn't a helper method to do the binding and create a bind visitor. Probably just need to add that.
--
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