huan233usc commented on PR #16851: URL: https://github.com/apache/iceberg/pull/16851#issuecomment-4796019827
Thanks @szehon-ho! Pushed 8d4a02067 with the two test follow-ups; left the type-mapping code as-is for now and want to explain why: - **Default-CRS round-trip test**: added a `Types.GeometryType.crs84()` round-trip to `testGeospatialTypeConversion` to lock in the `null` <-> `OGC:CRS84` normalization. - **Negative prune test**: added `testPruneGeospatialTypeWithIncompatibleRequestedType`, which requests a `BinaryType` for a geometry column and asserts the `Cannot project ... to incompatible type` precondition fires. - **Pass Spark's algorithm through (`SparkTypeToType`)**: I tried this, but on this branch `GeographyType.equals` still compares the raw nullable fields, so an explicit `SPHERICAL` is not equal to an omitted algorithm and it breaks the round-trip assertions. That resolved-value normalization is exactly what #16765 adds — once it lands, both directions can pass the algorithm through symmetrically and fail loudly on an unknown Spark algorithm. Keeping the documented drop-to-default here until then. - **Explicit algorithm/CRS translation layer (`TypeToSparkType`)**: the `!= SPHERICAL` guard already rejects loudly and self-documents the only supported value. I'd rather not duplicate Spark's accepted-CRS allow-list (`OGC:CRS84` only for geography today) in the Iceberg layer since it would drift across Spark versions and Spark already raises a CRS validation error. Happy to revisit alongside the metrics/value follow-up if you feel strongly. -- 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]
