laskoviymishka commented on code in PR #2221:
URL: https://github.com/apache/iceberg-go/pull/2221#discussion_r4329363078
##########
types.go:
##########
@@ -1177,6 +1177,20 @@ func PromoteType(fileType, readType Type) (Type, error) {
if _, ok := readType.(StringType); ok {
return readType, nil
}
+ // Deliberately no case for GeometryType/GeographyType here: a
plain
+ // binary column is not known to carry valid WKB, so promoting
it to a
+ // geo type is unsafe even though both are physically a
BYTE_ARRAY.
Review Comment:
nit: sitting after the `return`, this reads as a trailing note on the string
branch. If the geo case folds into `default` like @zeroshade suggested, I'd
move it above `case BinaryType:` as the one place the decision lives, and
anchor it on the spec (no geo rows in the promotion table), which is harder to
argue around later than the row-group stats rationale:
```go
// BinaryType never promotes to or from GeometryType/GeographyType: the spec
// lists no promotions for geo types, and plain binary isn't guaranteed to
// hold valid WKB. Geo types reach the Equals check in default, so a CRS or
// edge-algorithm change errors as well.
```
##########
table/arrow_utils_test.go:
##########
@@ -3496,11 +3496,11 @@ func TestGeoTypeParquetRoundTrip(t *testing.T) {
// TestGeoParquetReadWithoutStoredSchema reads a GEOMETRY file that has no
// stored ARROW:schema, as Java/Spark writers produce. geoarrow-go maps the
// Parquet GEOMETRY logical type to geoarrow.wkb on read, so the file projects
-// onto a matching geometry column. A binary table column currently rejects it,
-// both on read and in the FileToDataFile (AddFiles) schema check. Whether
-// geometry should project onto a binary column is still an open decision
-// (https://github.com/apache/iceberg-go/issues/2193); flip the binary cases if
-// that promotion is allowed.
+// onto a matching geometry column. A binary table column deliberately rejects
+// it, both on read and in the FileToDataFile (AddFiles) schema check: geometry
+// is intentionally not promotable to/from binary (see the GeometryType /
+// GeographyType cases in iceberg.PromoteType), so these binary subtests are
+// expected to keep failing.
Review Comment:
nit: the binary subtests pass by asserting the rejection, so "expected to
keep failing" makes them sound like known failures. The pointer to the
`GeometryType / GeographyType` cases also goes stale if that case folds into
`default`, and the #2193 link got dropped:
```suggestion
// onto a matching geometry column. A binary table column rejects it, both on
// read and in the FileToDataFile (AddFiles) schema check: geo types are
// deliberately not promotable to or from binary (see iceberg.PromoteType and
// https://github.com/apache/iceberg-go/issues/2193), and the binary subtests
// assert that rejection.
```
--
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]
