szehon-ho commented on PR #16765: URL: https://github.com/apache/iceberg/pull/16765#issuecomment-4735876009
A couple of clarifying notes (not blocking — just flagging for discussion): **1. Write path isn't blocked, only the read path.** Now that `TypeToMessageType` emits `geometry`/`geography`, `ParquetSchemaUtil.convert` succeeds where it previously threw, so the failure point moves into the value path and the two sides behave asymmetrically: - Read: `BaseParquetReaders.primitive` runs the logical-type visitor and `.orElseThrow(...)`, so a geo column fails fast with `UnsupportedOperationException`. - Write: `BaseParquetWriter.primitive` runs its logical-type visitor, but when it returns empty it falls through to `case BINARY -> ParquetValueWriters.byteBuffers(desc)` rather than throwing. So a geo write no longer fails at writer setup — it silently degrades to the generic binary writer. Since the value path is an explicit follow-up, it'd be safer to also reject `GEOMETRY`/`GEOGRAPHY` on the write side here, so we don't produce files the reader then refuses to read. **2. Serialization effect of the `toString` change (default algorithm dropped).** For reference, here's what gets written into table metadata before/after this PR. CRS is unchanged (the constructor already collapses the default to the bare form); only the algorithm changes. CRS — no change: | User action | Before | After | |---|---|---| | Sets default explicitly (`OGC:CRS84`) | `"geography"` | `"geography"` | | Doesn't set it | `"geography"` | `"geography"` | Algorithm — changed: | User action | Before | After | |---|---|---| | Sets default explicitly (`spherical`) | `"geography(OGC:CRS84, spherical)"` | `"geography"` | | Doesn't set it | `"geography"` | `"geography"` | So explicitly setting `spherical` is no longer persisted — it now serializes identically to not setting it. Semantically fine (the spec defaults an unspecified algorithm to `spherical`), but re-serializing an existing table that stored `"...,spherical)"` will produce the shorter string (same type, different bytes). Worth calling out in the PR description. -- 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]
