paulcaron16k commented on issue #3758:
URL:
https://github.com/apache/iceberg-python/issues/3758#issuecomment-5253689906
Traced this to the root cause. Two corrections to my narrowing above, and
it turns out to be broader than `upsert`.
## Root cause
`_SnapshotProducer._build_delete_files_partition_predicate` (added in
#3011, commit `8a47d2b`) builds its predicate via
`Transaction._build_partition_predicate`, which emits
`EqualTo(Reference(<source column>), <partition value>)`. A `DataFile`
records its partition values **already transformed**, so comparing them against
the source column only holds for
identity transforms.
That helper's other caller, `dynamic_partition_overwrite`, rejects
non-identity transforms before calling it:
```python
for field in self.table_metadata.spec().fields:
if not isinstance(field.transform, IdentityTransform):
raise ValueError("For now dynamic overwrite does not support a
table with non-identity-transform field ...")
```
The pruning path added in #3011 has no such guard.
`inclusive_projection` then applies the transform a *second* time. Traced
on a `day(ts)` table:
```
predicate : EqualTo(term=Reference(name='ts'), literal=LongLiteral(20459))
projection : EqualTo(term=Reference(name='p'), literal=LongLiteral(0))
evaluator -> False partitions=[PartitionFieldSummary[...,
b'\xebO\x00\x00', ...]] # 0x4feb = 20459
```
20459 is the day ordinal; re-read as microseconds it becomes day 0. The
evaluator rejects the very manifest holding the file being replaced, so
`_existing_manifests` appends it whole beside
the rewritten file and `_deleted_entries` records nothing.
## Correction 1 — it is not specific to temporal transforms
My table above was an artifact of file layout, not of the transforms.
`truncate` and `bucket` partition on `k`, the join column, so each key landed
in its own data file; deleting one
dropped a whole file and never reached the partial-rewrite path. Forcing
two rows to share a partition:
| spec | rows sharing one file | result on 0.12.0 |
|---|---|---|
| `identity(ts)` | `a`, `b` same ts | correct |
| `day(ts)` | `a`, `b` same day | duplicates |
| `truncate(k,1)` | `aa`, `ab` | correct —
`truncate(truncate(x))==truncate(x)`, so the double transform is a no-op |
| `bucket(k,4)` | `k0`, `k1` (both bucket 2) | **raises** |
Only identity is genuinely correct. Truncate passes by idempotence, not by
design.
## Correction 2 — bucket raises rather than duplicating
```
TypeError: Cannot convert LongLiteral into string
```
from `_OverwriteFiles._deleted_entries` → `inclusive_projection` → `bind`,
where a bucket ordinal cannot bind to a string column.
## Correction 3 — `upsert` is not required
`Table.delete()` reaches the same path directly. On a `day(ts)` table
holding `idx` 1 and 2 in one file:
```python
tbl.delete(EqualTo("idx", 1))
tbl.scan().to_arrow()["idx"].to_pylist()
# 0.11.1 -> [2]
# 0.12.0 -> [2, 1, 2]
```
The deleted row survives *and* an untouched row is duplicated, because the
stale manifest and the rewritten file are both counted. `upsert` is just the
common way to reach
`_OverwriteFiles`, not the only one.
## Fix direction
`manifest_evaluator` binds its filter against
`Schema(*spec.partition_type(schema).fields)` — partition field names and
transform result types. That is exactly what a partition record
already holds, so the filter can be built over the partition fields
directly and the projection step drops out entirely. No transform is ever
reapplied, and pruning is preserved for every
spec rather than only identity ones.
--
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]
