hzmsh commented on PR #4115:
URL: https://github.com/apache/iceberg-python/pull/4115#issuecomment-6251109953
Hi @Fokko, thank you very much for the review and for pointing me to the
spec. That makes sense, and I'm happy to change the PR to skip the field when
writing.
I tried it locally first and ran into one case I wasn't sure how to handle,
so I'd really appreciate your guidance. Skipping the field works for a plain
scan. But when the dropped field comes before another partition field, the
partition record read back from the manifest is one field short, so the fields
after it shift, and filtering on them fails.
<details><summary>Example</summary>
```python
data = pa.Table.from_pydict({"id": [1, 2, 3], "category": ["a", "b", "a"],
"region": ["x", "y", "y"]})
tbl = catalog.create_table("default.t", schema=data.schema)
with tbl.update_spec() as u:
u.add_identity("category")
u.add_identity("region")
tbl.append(data)
with tbl.update_spec() as u:
u.remove_field("category")
with tbl.update_schema() as u:
u.delete_column("category")
tbl.delete("id == 1")
tbl.scan().to_arrow() # works either way
tbl.scan(row_filter="region == 'y'").to_arrow() # with the field skipped:
IndexError in the partition evaluator
```
With this PR as it is (unknown written as `null`), both scans return ids 2
and 3.
</details>
If I understand correctly, reading the skipped field back as null would mean
giving manifest reads the spec's partition type, which they don't have today
(`fetch_manifest_entry` reads the partition struct from the file's own schema).
I'd be glad to add that to this PR.
In case it's useful: I originally went with `null` because the [Avro type
mapping in the spec](https://iceberg.apache.org/spec/#avro) lists `unknown` as
"`null` or omitted", and Java writes it as `null`
([TypeToSchema.java](https://github.com/apache/iceberg/blob/9120cfa789d4ea29acd22c95973774f6a813a212/core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java#L216-L217)).
I should have mentioned that in the description, sorry about that.
Would you prefer that I go ahead with skipping the field and update the
manifest reads, or keep the `null` form for now? I'll follow whichever you
think is best. Thanks again for your time!
--
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]
