laskoviymishka commented on code in PR #1632: URL: https://github.com/apache/iceberg-go/pull/1632#discussion_r3712043690
########## table/geo_write_test.go: ########## @@ -0,0 +1,134 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table + +import ( + "strings" + "testing" + + "github.com/apache/arrow-go/v18/arrow" + "github.com/apache/arrow-go/v18/arrow/array" + "github.com/apache/arrow-go/v18/arrow/memory" + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/io" + tblutils "github.com/apache/iceberg-go/table/internal" + "github.com/geoarrow/geoarrow-go" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/twpayne/go-geom/encoding/wkb" + "github.com/twpayne/go-geom/encoding/wkt" +) + +// wktToWKB converts Well-Known Text into little-endian (NDR) Well-Known Binary, +// the on-disk encoding iceberg-go writes for geometry/geography columns. +func wktToWKB(t *testing.T, s string) geoarrow.WKBBytes { + t.Helper() + + g, err := wkt.Unmarshal(s) + require.NoError(t, err) + b, err := wkb.Marshal(g, wkb.NDR) + require.NoError(t, err) + + return geoarrow.WKBBytes(b) +} + +// TestWriteGeometryColumnPopulatesBounds writes a geometry and a geography +// column end-to-end through the data file writer and checks the manifest-level +// geo bounds that iceberg-go computes from the WKB values. arrow-go's Parquet +// writer does not emit native GeoStatistics, so iceberg-go derives the column +// bounds itself (see geoBoundsAccumulator) and threads them into the DataFile +// exactly as the manifest carries them. Geometry gets a planar XY bounding box; +// geography stays unbounded because a planar box over geodesic edges is unsafe. +// +// This is the public-path counterpart to internal.TestWriteDataFileGeoBounds: +// that test feeds hand-built StatsCols straight to WriteDataFile, whereas this +// one drives the full table writer (newDataFileWriter/writeFile/WriteTask over a +// MetadataBuilder), so it also covers geo stats-collector setup in the path a +// caller actually uses. +func TestWriteGeometryColumnPopulatesBounds(t *testing.T) { + t.Parallel() + + const ( + geomFieldID = 2 + geogFieldID = 3 + ) + + geogType, err := iceberg.GeographyTypeOf("OGC:CRS84", "spherical") + require.NoError(t, err) + + schema := iceberg.NewSchema(0, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int32, Required: false}, + iceberg.NestedField{ID: geomFieldID, Name: "geom", Type: iceberg.GeometryType{}, Required: false}, + iceberg.NestedField{ID: geogFieldID, Name: "geog", Type: geogType, Required: false}, + ) + + // Geometry/geography are v3 types. + mb, err := NewMetadataBuilder(3) + require.NoError(t, err) + require.NoError(t, mb.AddSchema(schema)) + require.NoError(t, mb.SetCurrentSchemaID(0)) + unpartitioned := *iceberg.UnpartitionedSpec + require.NoError(t, mb.AddPartitionSpec(&unpartitioned, true)) + require.NoError(t, mb.SetDefaultSpecID(0)) + + arrowSchema, err := SchemaToArrowSchema(schema, nil, true, false) + require.NoError(t, err) + + // Two geometry points spanning X in [0, 30] and Y in [-5, 10]. + geomLo := wktToWKB(t, "POINT (0 -5)") + geomHi := wktToWKB(t, "POINT (30 10)") + geog := wktToWKB(t, "POINT (12 4)") + + rec, _, err := array.RecordFromJSON(memory.DefaultAllocator, arrowSchema, strings.NewReader(`[ Review Comment: Follow-up, not for this PR: at some point I'd run this on a checked allocator rather than `memory.DefaultAllocator`. The suite tests (`WriteRecordsTestSuite`, `ClusteredWriterTestSuite`) use `memory.NewCheckedAllocator` and assert zero residual on teardown, and for a path wiring the geo-bounds accumulator a silent allocation escape is the kind of thing we'd want a test to catch. `memory.NewCheckedAllocator(memory.DefaultAllocator)` plus a `mem.AssertSize(t, 0)` once the writer's done matches `arrow_utils_internal_test.go`. ########## table/geo_write_test.go: ########## @@ -0,0 +1,134 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table + +import ( + "strings" + "testing" + + "github.com/apache/arrow-go/v18/arrow" + "github.com/apache/arrow-go/v18/arrow/array" + "github.com/apache/arrow-go/v18/arrow/memory" + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/io" + tblutils "github.com/apache/iceberg-go/table/internal" + "github.com/geoarrow/geoarrow-go" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/twpayne/go-geom/encoding/wkb" + "github.com/twpayne/go-geom/encoding/wkt" +) + +// wktToWKB converts Well-Known Text into little-endian (NDR) Well-Known Binary, +// the on-disk encoding iceberg-go writes for geometry/geography columns. +func wktToWKB(t *testing.T, s string) geoarrow.WKBBytes { + t.Helper() + + g, err := wkt.Unmarshal(s) + require.NoError(t, err) + b, err := wkb.Marshal(g, wkb.NDR) + require.NoError(t, err) + + return geoarrow.WKBBytes(b) +} + +// TestWriteGeometryColumnPopulatesBounds writes a geometry and a geography +// column end-to-end through the data file writer and checks the manifest-level +// geo bounds that iceberg-go computes from the WKB values. arrow-go's Parquet +// writer does not emit native GeoStatistics, so iceberg-go derives the column +// bounds itself (see geoBoundsAccumulator) and threads them into the DataFile +// exactly as the manifest carries them. Geometry gets a planar XY bounding box; +// geography stays unbounded because a planar box over geodesic edges is unsafe. +// +// This is the public-path counterpart to internal.TestWriteDataFileGeoBounds: +// that test feeds hand-built StatsCols straight to WriteDataFile, whereas this +// one drives the full table writer (newDataFileWriter/writeFile/WriteTask over a +// MetadataBuilder), so it also covers geo stats-collector setup in the path a +// caller actually uses. +func TestWriteGeometryColumnPopulatesBounds(t *testing.T) { + t.Parallel() + + const ( + geomFieldID = 2 + geogFieldID = 3 + ) + + geogType, err := iceberg.GeographyTypeOf("OGC:CRS84", "spherical") + require.NoError(t, err) + + schema := iceberg.NewSchema(0, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int32, Required: false}, + iceberg.NestedField{ID: geomFieldID, Name: "geom", Type: iceberg.GeometryType{}, Required: false}, + iceberg.NestedField{ID: geogFieldID, Name: "geog", Type: geogType, Required: false}, + ) + + // Geometry/geography are v3 types. + mb, err := NewMetadataBuilder(3) + require.NoError(t, err) + require.NoError(t, mb.AddSchema(schema)) + require.NoError(t, mb.SetCurrentSchemaID(0)) + unpartitioned := *iceberg.UnpartitionedSpec + require.NoError(t, mb.AddPartitionSpec(&unpartitioned, true)) + require.NoError(t, mb.SetDefaultSpecID(0)) + + arrowSchema, err := SchemaToArrowSchema(schema, nil, true, false) + require.NoError(t, err) + + // Two geometry points spanning X in [0, 30] and Y in [-5, 10]. + geomLo := wktToWKB(t, "POINT (0 -5)") + geomHi := wktToWKB(t, "POINT (30 10)") + geog := wktToWKB(t, "POINT (12 4)") + + rec, _, err := array.RecordFromJSON(memory.DefaultAllocator, arrowSchema, strings.NewReader(`[ + {"id": 1, "geom": "`+geomLo.String()+`", "geog": "`+geog.String()+`"}, Review Comment: Not blocking, just a note for whoever reads this next: the test quietly depends on geoarrow's JSON parser round-tripping `geomLo.String()` (lowercase hex) back into WKB. If that ever stopped, the accumulator would see no values and the `require.Contains` below would fail for a fairly confusing reason. It works today and `parquet_files_test.go` leans on the same `.String()` hex pattern, so a one-line comment pointing at that precedent would save someone the archaeology later. ########## table/geo_write_test.go: ########## @@ -0,0 +1,134 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table + +import ( + "strings" + "testing" + + "github.com/apache/arrow-go/v18/arrow" + "github.com/apache/arrow-go/v18/arrow/array" + "github.com/apache/arrow-go/v18/arrow/memory" + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/io" + tblutils "github.com/apache/iceberg-go/table/internal" + "github.com/geoarrow/geoarrow-go" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/twpayne/go-geom/encoding/wkb" + "github.com/twpayne/go-geom/encoding/wkt" +) + +// wktToWKB converts Well-Known Text into little-endian (NDR) Well-Known Binary, +// the on-disk encoding iceberg-go writes for geometry/geography columns. +func wktToWKB(t *testing.T, s string) geoarrow.WKBBytes { + t.Helper() + + g, err := wkt.Unmarshal(s) + require.NoError(t, err) + b, err := wkb.Marshal(g, wkb.NDR) + require.NoError(t, err) + + return geoarrow.WKBBytes(b) +} + +// TestWriteGeometryColumnPopulatesBounds writes a geometry and a geography +// column end-to-end through the data file writer and checks the manifest-level +// geo bounds that iceberg-go computes from the WKB values. arrow-go's Parquet +// writer does not emit native GeoStatistics, so iceberg-go derives the column +// bounds itself (see geoBoundsAccumulator) and threads them into the DataFile +// exactly as the manifest carries them. Geometry gets a planar XY bounding box; +// geography stays unbounded because a planar box over geodesic edges is unsafe. +// +// This is the public-path counterpart to internal.TestWriteDataFileGeoBounds: +// that test feeds hand-built StatsCols straight to WriteDataFile, whereas this +// one drives the full table writer (newDataFileWriter/writeFile/WriteTask over a +// MetadataBuilder), so it also covers geo stats-collector setup in the path a +// caller actually uses. +func TestWriteGeometryColumnPopulatesBounds(t *testing.T) { + t.Parallel() + + const ( + geomFieldID = 2 + geogFieldID = 3 + ) + + geogType, err := iceberg.GeographyTypeOf("OGC:CRS84", "spherical") + require.NoError(t, err) + + schema := iceberg.NewSchema(0, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int32, Required: false}, + iceberg.NestedField{ID: geomFieldID, Name: "geom", Type: iceberg.GeometryType{}, Required: false}, + iceberg.NestedField{ID: geogFieldID, Name: "geog", Type: geogType, Required: false}, + ) + + // Geometry/geography are v3 types. + mb, err := NewMetadataBuilder(3) + require.NoError(t, err) + require.NoError(t, mb.AddSchema(schema)) + require.NoError(t, mb.SetCurrentSchemaID(0)) + unpartitioned := *iceberg.UnpartitionedSpec + require.NoError(t, mb.AddPartitionSpec(&unpartitioned, true)) + require.NoError(t, mb.SetDefaultSpecID(0)) + + arrowSchema, err := SchemaToArrowSchema(schema, nil, true, false) + require.NoError(t, err) + + // Two geometry points spanning X in [0, 30] and Y in [-5, 10]. + geomLo := wktToWKB(t, "POINT (0 -5)") + geomHi := wktToWKB(t, "POINT (30 10)") + geog := wktToWKB(t, "POINT (12 4)") + + rec, _, err := array.RecordFromJSON(memory.DefaultAllocator, arrowSchema, strings.NewReader(`[ + {"id": 1, "geom": "`+geomLo.String()+`", "geog": "`+geog.String()+`"}, + {"id": 2, "geom": "`+geomHi.String()+`", "geog": null} + ]`)) + require.NoError(t, err) + defer rec.Release() + + writer, err := newDataFileWriter(t.TempDir(), &io.LocalFS{}, mb, iceberg.Properties{}) + require.NoError(t, err) + + df, err := writer.writeFile(t.Context(), nil, WriteTask{ + Uuid: uuid.New(), + ID: 0, + FileCount: 1, + Schema: schema, + Batches: []arrow.RecordBatch{rec}, + }) + require.NoError(t, err) + require.EqualValues(t, 2, df.Count()) + + // Geometry column carries a planar XY bounding box in the manifest bounds. + lower := df.LowerBoundValues() + upper := df.UpperBoundValues() + require.Contains(t, lower, geomFieldID, "geometry column must record a lower bound") + require.Contains(t, upper, geomFieldID, "geometry column must record an upper bound") + + minX, minY, maxX, maxY, ok := tblutils.GeoBoundsXY(lower[geomFieldID], upper[geomFieldID]) + require.True(t, ok, "geometry bounds must decode to a planar XY box") + assert.Equal(t, 0.0, minX) + assert.Equal(t, -5.0, minY) + assert.Equal(t, 30.0, maxX) + assert.Equal(t, 10.0, maxY) Review Comment: Follow-up idea: alongside the geo bounds, asserting that the `id` column still gets ordinary min/max would be cheap insurance. The geo-type guard in `DataFileStatsFromMeta` suppresses generic Parquet stats for geo columns, and a regression that over-suppressed an adjacent non-geo column would sail straight past this test. A single `require.Contains(t, lower, 1)` on the `id` field ID pins it down. ########## table/geo_write_test.go: ########## @@ -0,0 +1,134 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table + +import ( + "strings" + "testing" + + "github.com/apache/arrow-go/v18/arrow" + "github.com/apache/arrow-go/v18/arrow/array" + "github.com/apache/arrow-go/v18/arrow/memory" + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/io" + tblutils "github.com/apache/iceberg-go/table/internal" + "github.com/geoarrow/geoarrow-go" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/twpayne/go-geom/encoding/wkb" + "github.com/twpayne/go-geom/encoding/wkt" +) + +// wktToWKB converts Well-Known Text into little-endian (NDR) Well-Known Binary, +// the on-disk encoding iceberg-go writes for geometry/geography columns. +func wktToWKB(t *testing.T, s string) geoarrow.WKBBytes { + t.Helper() + + g, err := wkt.Unmarshal(s) + require.NoError(t, err) + b, err := wkb.Marshal(g, wkb.NDR) + require.NoError(t, err) + + return geoarrow.WKBBytes(b) +} + +// TestWriteGeometryColumnPopulatesBounds writes a geometry and a geography +// column end-to-end through the data file writer and checks the manifest-level +// geo bounds that iceberg-go computes from the WKB values. arrow-go's Parquet +// writer does not emit native GeoStatistics, so iceberg-go derives the column +// bounds itself (see geoBoundsAccumulator) and threads them into the DataFile +// exactly as the manifest carries them. Geometry gets a planar XY bounding box; +// geography stays unbounded because a planar box over geodesic edges is unsafe. +// +// This is the public-path counterpart to internal.TestWriteDataFileGeoBounds: +// that test feeds hand-built StatsCols straight to WriteDataFile, whereas this +// one drives the full table writer (newDataFileWriter/writeFile/WriteTask over a +// MetadataBuilder), so it also covers geo stats-collector setup in the path a +// caller actually uses. +func TestWriteGeometryColumnPopulatesBounds(t *testing.T) { + t.Parallel() + + const ( + geomFieldID = 2 + geogFieldID = 3 + ) + + geogType, err := iceberg.GeographyTypeOf("OGC:CRS84", "spherical") + require.NoError(t, err) + + schema := iceberg.NewSchema(0, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int32, Required: false}, + iceberg.NestedField{ID: geomFieldID, Name: "geom", Type: iceberg.GeometryType{}, Required: false}, + iceberg.NestedField{ID: geogFieldID, Name: "geog", Type: geogType, Required: false}, + ) + + // Geometry/geography are v3 types. + mb, err := NewMetadataBuilder(3) + require.NoError(t, err) + require.NoError(t, mb.AddSchema(schema)) + require.NoError(t, mb.SetCurrentSchemaID(0)) + unpartitioned := *iceberg.UnpartitionedSpec + require.NoError(t, mb.AddPartitionSpec(&unpartitioned, true)) + require.NoError(t, mb.SetDefaultSpecID(0)) + + arrowSchema, err := SchemaToArrowSchema(schema, nil, true, false) + require.NoError(t, err) + + // Two geometry points spanning X in [0, 30] and Y in [-5, 10]. + geomLo := wktToWKB(t, "POINT (0 -5)") + geomHi := wktToWKB(t, "POINT (30 10)") + geog := wktToWKB(t, "POINT (12 4)") + + rec, _, err := array.RecordFromJSON(memory.DefaultAllocator, arrowSchema, strings.NewReader(`[ + {"id": 1, "geom": "`+geomLo.String()+`", "geog": "`+geog.String()+`"}, + {"id": 2, "geom": "`+geomHi.String()+`", "geog": null} + ]`)) + require.NoError(t, err) + defer rec.Release() + + writer, err := newDataFileWriter(t.TempDir(), &io.LocalFS{}, mb, iceberg.Properties{}) + require.NoError(t, err) + + df, err := writer.writeFile(t.Context(), nil, WriteTask{ + Uuid: uuid.New(), + ID: 0, + FileCount: 1, + Schema: schema, + Batches: []arrow.RecordBatch{rec}, + }) + require.NoError(t, err) + require.EqualValues(t, 2, df.Count()) + + // Geometry column carries a planar XY bounding box in the manifest bounds. + lower := df.LowerBoundValues() + upper := df.UpperBoundValues() + require.Contains(t, lower, geomFieldID, "geometry column must record a lower bound") + require.Contains(t, upper, geomFieldID, "geometry column must record an upper bound") + + minX, minY, maxX, maxY, ok := tblutils.GeoBoundsXY(lower[geomFieldID], upper[geomFieldID]) + require.True(t, ok, "geometry bounds must decode to a planar XY box") + assert.Equal(t, 0.0, minX) + assert.Equal(t, -5.0, minY) + assert.Equal(t, 30.0, maxX) + assert.Equal(t, 10.0, maxY) + + // Geography stays unbounded: a planar box over geodesic edges is unsafe. + assert.NotContains(t, lower, geogFieldID, "geography column must not record bounds") Review Comment: Wording nit for a follow-up: the assertion itself is right, but "must not record bounds" reads as a spec mandate and it isn't one. The V3 spec permits geography bounds (including the xmin > xmax wrapping convention) — iceberg-go omits them as a deliberate conservative choice, which the accumulator's own comment spells out ("geography is left unbounded until geodesic/antimeridian-aware computation is added"). Phrasing it as "geography does not record bounds in the current implementation" keeps us from encoding a temporary limitation as an invariant, and makes it obvious this assertion should flip when that computation lands. -- 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]
