twuebi commented on PR #1873:
URL: https://github.com/apache/iceberg-go/pull/1873#issuecomment-5450781491
I don't think `source-id: 0` is correct either, you're right that a spec
serialized with no `source-id` key won't read back in Java or PyIceberg.
`PartitionSpecParser.fromJson` reads it with `JsonUtil.getInt(SOURCE_ID,
element)`, which throws on a missing key.
But dropping the special-case so we write `source-id: 0` doesn't match Java
either, maybe it's even worse:
- **Java never writes `0` for a void field.** `toJson` writes
`field.sourceId()` unconditionally, with no void branch, and `PartitionField`
holds the original source id, a voided field serializes with its original
positive source id. This is what you noted yourself in the
`updates_test.go:1303` thread ("Java preserves the original positive source-id
rather than storing 0"). Rust does the same, void fields carry a positive
`source_id`. So `source-id: 0` isn't Java's output. It's our go-sentinel value
leaking into the file.
- **`0` isn't a valid schema field id.** With `source-id: 0` present,
Java/PyIceberg get past the `getInt` read but then reference a column that
doesn't exist, so it fails at bind instead of at read. That's not "readable" in
any useful sense.
- **Our own bound decoder rejects it.** `validatePartitionSourceID` returns
`partition source ID must be positive: 0` for a bound spec, and
`PartitionField.UnmarshalJSON` decodes as bound. So the file we'd write
couldn't be read back by iceberg-go either.
The root of it: a source-less void tombstone has already discarded the
original source id (that's what the `[]int{0}` sentinel is), so there's no
positive id to recover at marshal time. `MarshalJSON` is the wrong layer to fix
this, the current omit-in/omit-out round-trip at least preserves the historical
file exactly, whereas `source-id: 0` turns it into something no implementation,
ours included, can bind.
For real Java parity, the fix belongs upstream of serialization, preserve
the original positive source id through decode/bind rather than collapsing it
to `0`, or reject a source-less void field in the add-spec commit path. I'm
happy to work on a follow-up PR to fix this, wdyt?
--
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]
