laskoviymishka commented on code in PR #2125:
URL: https://github.com/apache/iceberg-go/pull/2125#discussion_r4241080898
##########
table/arrow_utils_test.go:
##########
@@ -3355,31 +3361,43 @@ func TestGeoTypeParquetRoundTrip(t *testing.T) {
name string
icebergType iceberg.Type
geoarrowMetaJSON string
+ parquetLogical schema.LogicalType
}{
{
name: "geometry_default_crs",
icebergType: iceberg.GeometryType{},
geoarrowMetaJSON:
`{"crs":"OGC:CRS84","crs_type":"authority_code"}`,
+ parquetLogical: schema.GeometryLogicalType{},
},
{
name: "geometry_srid_0",
icebergType: geomSRID0,
geoarrowMetaJSON: `{"crs":"0","crs_type":"srid"}`,
+ parquetLogical: schema.GeometryLogicalType{Crs:
"srid:0"},
+ },
+ {
+ name: "geography_default",
+ icebergType: iceberg.GeographyType{},
+ geoarrowMetaJSON:
`{"crs":"OGC:CRS84","crs_type":"authority_code","edges":"spherical"}`,
+ parquetLogical:
schema.GeographyLogicalType{Algorithm: schema.GeographyEdgeSpherical},
},
{
name: "geography_srid_0",
icebergType: geogSRID0,
geoarrowMetaJSON:
`{"crs":"0","crs_type":"srid","edges":"spherical"}`,
+ parquetLogical: schema.GeographyLogicalType{Crs:
"srid:0", Algorithm: schema.GeographyEdgeSpherical},
},
{
name: "geometry_srid_4326",
icebergType: geomSRID,
geoarrowMetaJSON: `{"crs":"4326","crs_type":"srid"}`,
+ parquetLogical: schema.GeometryLogicalType{Crs:
"srid:4326"},
},
{
name: "geometry_authority_code",
icebergType: geomEPSG3857,
geoarrowMetaJSON:
`{"crs":"EPSG:3857","crs_type":"authority_code"}`,
+ parquetLogical: schema.GeometryLogicalType{Crs:
"EPSG:3857"},
Review Comment:
non-blocking, follow-up: could we add a `geometry(EPSG:4326)` row? It
collapses to `OGC:CRS84` (no `crs`) on the wire while Java writes `EPSG:4326`
verbatim, and the axis order differs, so I'd like whatever we emit pinned
deliberately.
##########
table/internal/parquet_files.go:
##########
@@ -1188,25 +1196,31 @@ func normalizeWKBArrayReachable(ext
array.ExtensionArray, active []bool, mem mem
}
// accumulateGeoBounds extends the per-field bounding boxes with the WKB values
-// in this batch. Null rows are skipped; a malformed WKB value fails the write.
+// in this batch and tallies their null counts. Null rows are skipped; a
+// malformed WKB value fails the write.
+//
+// Every geoCols entry comes from a *geoarrow.WKBType field of the writer's
+// schema, so a batch whose column does not match is an error rather than a
+// skip: skipping would leave a partial null count that under-reports nulls.
func (w *ParquetFileWriter) accumulateGeoBounds(batch arrow.RecordBatch) error
{
for _, gc := range w.geoCols {
if gc.colIdx >= int(batch.NumCols()) {
- continue
+ return fmt.Errorf("geo field %d: column %d missing from
batch with %d columns",
+ gc.fieldID, gc.colIdx, batch.NumCols())
}
ext, ok := batch.Column(gc.colIdx).(array.ExtensionArray)
if !ok {
- continue
+ return fmt.Errorf("geo field %d: expected a WKB
extension array, got %s",
+ gc.fieldID, batch.Column(gc.colIdx).DataType())
}
storage, ok := ext.Storage().(wkbStorage)
if !ok {
- continue
+ return fmt.Errorf("geo field %d: unsupported WKB
storage type %s",
+ gc.fieldID, ext.Storage().DataType())
}
- acc, ok := w.geoAccs[gc.fieldID]
- if !ok {
- continue
- }
+ acc := w.geoAccs[gc.fieldID]
Review Comment:
non-blocking, fine as a follow-up: I'd keep the `ok` check here and return
an error on a miss, same as the guards above. It's safe today because `geoAccs`
is built from the same `geoCols`, but if either side ever gets filtered this
becomes a nil panic on the write path.
--
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]
