JingsongLi commented on PR #9497:
URL: https://github.com/apache/paimon/pull/9497#issuecomment-5739077642
I re-checked the current head (f590fff) against my earlier comments. Two of
the three [P1]s are addressed, one is only partially addressed.
Fixed:
- `partition-spec` is now the JSON array of fields, matching
`PartitionSpecParser.fromJsonFields` — good.
- `content` is now written per writer (`data` / `deletes`) through the
per-`Content` writer factories — good.
Still open (positive-ID mapping):
1. [P1] `withPositiveFieldIds` (IcebergCommitCallback.java:761) renumbers
only the top-level fields, but Paimon assigns IDs from a single global counter
for top-level *and* nested fields (`Schema.Builder#column` →
`ReassignFieldId`). For a table like `(a INT, s ROW<x INT, y INT>)` the Paimon
IDs are `a=0, s=1, x=2, y=3`, so the remapped header becomes `{a:1, s:2{x:2,
y:3}}`: duplicate IDs inside one Iceberg schema, and `s`'s nested IDs are now
unrelated to `s`'s new ID. ID-based lookups (`schema.findField`, partition-spec
source-ID resolution) then have no unambiguous answer. The remap has to be
recursive (or an offset applied consistently) rather than top-level only.
2. [P1] The remap is applied only to the manifest header. The table metadata
published by this same callback still uses 0-based IDs
(`schemaCache.get(schemaId)` / `IcebergSchema.create` at :576-578, plus
`getPartitionFields` on that schema), and `IcebergDataFileMeta` still keys the
metrics maps (`null_value_counts`, `lower_bounds`, `upper_bounds`) by the
0-based `field.id()`. So one table now carries two different ID spaces — header
`1..N` vs. metadata/metrics `0-based` — and any reader that resolves metric
keys or partition source IDs against the header schema will land on the wrong
field. The original ask was one consistent positive-ID mapping everywhere
Iceberg IDs are emitted (schema, partition source IDs, metrics maps).
Also, none of the three points got a regression test. The `content` /
`partition-spec` behavior can be locked down by opening the generated manifest
with Iceberg `ManifestFiles.read` without an external spec map, and the ID
mapping by asserting that a table with nested types produces unique, positive
IDs in the header.
The [P2] about already-affected tables still stands as well (legacy
manifests without headers are retained by `createMetadataWithBase`), but it is
not blocking.
--
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]
