szehon-ho commented on PR #16765: URL: https://github.com/apache/iceberg/pull/16765#issuecomment-4735525629
A couple of things worth addressing before this goes in: **1. Writes aren't blocked, only reads are — asymmetric half-wiring.** Now that `TypeToMessageType` emits `geometry`/`geography`, `ParquetSchemaUtil.convert` succeeds where it used to throw, so the failure point moves into the value path and the two sides behave differently: - Read: `BaseParquetReaders.primitive` runs the logical-type visitor and `.orElseThrow(...)`, so a geo column throws `UnsupportedOperationException` — fails fast (good). - Write: `BaseParquetWriter.primitive` runs its logical-type visitor, but when it returns empty it **falls through** to `case BINARY -> ParquetValueWriters.byteBuffers(desc)` instead of throwing. So a geo write no longer fails at schema/writer setup; it silently degrades to the generic binary writer (raw bytes, or a `ClassCastException` depending on the value). Since the value read/write path is an explicit follow-up, can we also block geo on the write side here (reject `GEOMETRY`/`GEOGRAPHY` in the writer) so we don't produce files the reader then refuses to read? As-is the type is half-wired: schema converts, reads throw, writes fall through. **2. The `GeographyType` equality/serialization change is an API-level behavior change bundled into the Parquet work.** `equals`/`hashCode`/`toString` now compare the resolved getters, so an explicit `spherical` is treated as equal to an omitted one. Two effects worth surfacing: - It's correct per spec (an unspecified `A` is `spherical`) and is arguably a latent-bug fix, but it changes `GeographyType` semantics for *all* engines/catalogs, not just the Parquet path. - Serialization side effect: `toString` now drops the default algorithm, so `geography(srid:4326, spherical)` serializes into table metadata as `"geography(srid:4326)"`. Re-serializing an existing table that stored `"...,spherical)"` produces a shorter (semantically equivalent) string — a visible metadata diff. This was originally split out as #16766. Given the cross-engine blast radius, it may be cleaner to land the equality change on its own with its own tests/justification rather than as a dependency of the schema mapping — and at minimum the PR description should call out the serialization change. -- 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]
