rangareddy commented on PR #850:
URL: https://github.com/apache/incubator-xtable/pull/850#issuecomment-5190038652
Rebuilt as a single commit `719ad4a` (the `scala.Array(namespace)` fix
folded in — `java.util._` is imported, so a bare `Array` resolves against
`java.lang.reflect.Array` under Ammonite). Squash verified content-preserving:
`git diff` against the merge base has the same checksum before and after.
**I need to retract a claim in the description.** It said every cell had
been executed end-to-end on pristine seed data. That is not true on the current
revision, and it was wrong rather than merely outdated, so I have corrected the
description rather than quietly editing around it.
The round-2 append cell fails **deterministically** on a freshly reset
`demo/data`:
```
ERROR TableFormatSync: Failed to sync table changes
java.lang.IllegalArgumentException: Cannot add field CustomerKey as an
identifier field: not a required field
at org.apache.iceberg.Schema.validateIdentifierField(Schema.java:159)
at org.apache.iceberg.SchemaUpdate.applyChanges(SchemaUpdate.java:554)
at
org.apache.xtable.iceberg.IcebergSchemaSync.sync(IcebergSchemaSync.java:60)
```
| step | `CustomerKey` |
|---|---|
| tracked seed commit `20231105172136406.commit` | `"string"` → **required**
|
| initial sync → Iceberg metadata v1 | `identifier-field-ids=[2]`,
`required=true` |
| round-2 append commit | `["null","string"]` → **optional** |
| incremental sync | fails as above |
The round-2 CSV is read with `header=true` and no schema, so Spark types
every column as a nullable string, flipping `CustomerKey` from required to
optional. `IcebergSchemaExtractor.toIceberg` then correctly declines to set
identifier fields for a nullable record key — that is the guard added for #366
— so XTable's target schema has none. But the **table** still carries
`identifier-field-ids=[2]` from the initial sync, and `SchemaUpdate.apply()`
re-validates those against the new schema.
`grep -rn 'setIdentifierFields' xtable-core/src/main/java` returns nothing:
`IcebergSchemaSync` never reconciles identifier fields, so once a record key
goes required → optional the Iceberg target is permanently wedged — every
subsequent sync fails the same way.
This is a **distinct gap from #261 / #366**, which fixed table *creation*
with an already-nullable key rather than the transition on an existing table.
It reproduces outside the demo, so it is an XTable bug that the demo merely
surfaces — deterministically, because the seed's required key plus a
schema-less round-2 read guarantees the transition, and `stop_demo.sh
--reset-data` restores exactly that state.
Why it looked green before: a `demo/data` directory carried over from a
previous run already has `CustomerKey` nullable, so the initial sync never sets
an identifier field and the round-2 cell passes. Order-dependent, which is why
neither of us caught it.
Two things I would like your view on before this merges:
1. **How should `IcebergSchemaSync` handle it** — clear the identifier
fields when the latest schema has none, refuse the nullability change with a
clear error, or keep the record key required on the Iceberg side? That is a
semantics decision, so I would rather not pick it unilaterally. Happy to file
it as an issue with this evidence and take it to `dev@` if you prefer.
2. **Whether this PR should merge before that is fixed.** The round-2 cell
is part of the notebook, so as it stands a new user following the demo hits the
failure. Everything else in the notebook passes; the hardening changes are
independent of it.
For the record, `inferSchema=true` on the round-2 read is not a workaround:
the existing schema is all required strings, so inference additionally gives
`CustomerKey: int` and `YearlyIncome: double`, adding a string → int change
that Iceberg rejects outright.
--
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]
