kevinjqliu commented on code in PR #1644:
URL: https://github.com/apache/iceberg-python/pull/1644#discussion_r2173391227
##########
tests/integration/test_reads.py:
##########
@@ -1024,3 +1025,31 @@ def test_scan_with_datetime(catalog: Catalog) -> None:
df = table.scan(row_filter=LessThan("datetime", yesterday)).to_pandas()
assert len(df) == 0
+
+
[email protected]
+# TODO: For Hive we require writing V3
Review Comment:
we cant write v3 tables using the hive catalog yet?
##########
pyiceberg/expressions/visitors.py:
##########
@@ -893,15 +893,28 @@ def visit_unbound_predicate(self, predicate:
UnboundPredicate[L]) -> BooleanExpr
raise TypeError(f"Expected Bound Predicate, got: {predicate.term}")
def visit_bound_predicate(self, predicate: BoundPredicate[L]) ->
BooleanExpression:
- file_column_name =
self.file_schema.find_column_name(predicate.term.ref().field.field_id)
+ field = predicate.term.ref().field
+ file_column_name = self.file_schema.find_column_name(field.field_id)
if file_column_name is None:
# In the case of schema evolution, the column might not be present
- # in the file schema when reading older data
- if isinstance(predicate, BoundIsNull):
Review Comment:
this checks against `BoundIsNull`, but the same `BoundIsNull` check will
raise ValueError in the new implementation
##########
tests/io/test_pyarrow.py:
##########
@@ -2398,6 +2398,17 @@ def test_identity_partition_on_multi_columns() -> None:
) == arrow_table.sort_by([("born_year", "ascending"), ("n_legs",
"ascending"), ("animal", "ascending")])
+def test_initial_value() -> None:
+ # Have some fake data, otherwise it will generate a table without records
+ data = pa.record_batch([pa.nulls(10, pa.int32())], ["some_field"])
+ result = _to_requested_schema(
+ Schema(NestedField(1, "so-true", BooleanType(), required=True,
initial_default=True)), Schema(), data
Review Comment:
nit: `initial_default=True` here is a bit confusing, esp `required=True` is
right next to it.
`required=True` sets the `required` boolean option to True
`initial_default=True` sets the **value of** `initial_default` to True
##########
tests/integration/test_reads.py:
##########
@@ -1024,3 +1025,31 @@ def test_scan_with_datetime(catalog: Catalog) -> None:
df = table.scan(row_filter=LessThan("datetime", yesterday)).to_pandas()
assert len(df) == 0
+
+
[email protected]
+# TODO: For Hive we require writing V3
+# @pytest.mark.parametrize("catalog",
[pytest.lazy_fixture("session_catalog_hive"),
pytest.lazy_fixture("session_catalog")])
[email protected]("catalog", [pytest.lazy_fixture("session_catalog")])
+def test_initial_default(catalog: Catalog, spark: SparkSession) -> None:
+ identifier = "default.test_initial_default"
+ try:
+ catalog.drop_table(identifier)
+ except NoSuchTableError:
+ pass
+
+ one_column = pa.table([pa.nulls(10, pa.int32())], ["some_field"])
Review Comment:
nit:
```suggestion
one_column = pa.table([pa.nulls(10, pa.int32())], names=["some_field"])
```
##########
pyiceberg/expressions/visitors.py:
##########
@@ -893,15 +893,28 @@ def visit_unbound_predicate(self, predicate:
UnboundPredicate[L]) -> BooleanExpr
raise TypeError(f"Expected Bound Predicate, got: {predicate.term}")
def visit_bound_predicate(self, predicate: BoundPredicate[L]) ->
BooleanExpression:
- file_column_name =
self.file_schema.find_column_name(predicate.term.ref().field.field_id)
+ field = predicate.term.ref().field
+ file_column_name = self.file_schema.find_column_name(field.field_id)
if file_column_name is None:
# In the case of schema evolution, the column might not be present
- # in the file schema when reading older data
- if isinstance(predicate, BoundIsNull):
- return AlwaysTrue()
+ # we can use the default value as a constant and evaluate it
against
+ # the predicate
+ pred: BooleanExpression
+ if isinstance(predicate, BoundUnaryPredicate):
+ pred = predicate.as_unbound(field.name)
+ elif isinstance(predicate, BoundLiteralPredicate):
+ pred = predicate.as_unbound(field.name, predicate.literal)
+ elif isinstance(predicate, BoundSetPredicate):
+ pred = predicate.as_unbound(field.name, predicate.literals)
else:
- return AlwaysFalse()
+ raise ValueError(f"Unsupported predicate: {predicate}")
+
+ return (
+ AlwaysTrue()
+ if expression_evaluator(Schema(field), pred,
case_sensitive=self.case_sensitive)(Record(field.initial_default))
+ else AlwaysFalse()
+ )
if isinstance(predicate, BoundUnaryPredicate):
Review Comment:
is this line L919 to L926 duplicate?
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1814,9 +1814,12 @@ def struct(
array = self._cast_if_needed(field, field_array)
field_arrays.append(array)
fields.append(self._construct_field(field, array.type))
- elif field.optional:
+ elif field.optional or field.initial_default is not None:
arrow_type = schema_to_pyarrow(field.field_type,
include_field_ids=self._include_field_ids)
- field_arrays.append(pa.nulls(len(struct_array),
type=arrow_type))
+ if field.initial_default is None:
+ field_arrays.append(pa.nulls(len(struct_array),
type=arrow_type))
+ else:
+ field_arrays.append(pa.repeat(field.initial_default,
len(struct_array)))
fields.append(self._construct_field(field, arrow_type))
else:
raise ResolveError(f"Field is required, and could not be found
in the file: {field}")
Review Comment:
wdyt about restructuring this logic a bit to be more readable?
Optional field might have initial-default set. If set, use initial-default.
If not set, use null
```
When an optional field is added, the defaults may be null and should be
explicitly set
If initial-default is not set for an optional field, then the default value
is null for compatibility with older spec versions.
```
Required field will always have initial-default set. Should use
initial-default if `field_array` is None
```
When a required field is added, both defaults must be set to a non-null value
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
