laskoviymishka commented on code in PR #2164: URL: https://github.com/apache/iceberg-go/pull/2164#discussion_r4232826079
########## table/pos_delete_metrics_test.go: ########## @@ -0,0 +1,115 @@ +// 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_test + +import ( + "context" + "path/filepath" + "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" + iceio "github.com/apache/iceberg-go/io" + "github.com/apache/iceberg-go/table" + "github.com/stretchr/testify/require" +) + +// TestPositionDeleteFilesKeepFullFilePathBounds pins #2141: v2 position delete +// files keep exact file_path bounds and pos bounds whatever the table metrics +// settings are, as Java's MetricsConfig.forPositionDelete does. The table +// here asks for no file_path or pos metrics and data file paths are far +// longer than the default truncate(16). +func TestPositionDeleteFilesKeepFullFilePathBounds(t *testing.T) { + identity := iceberg.NewPartitionSpec(iceberg.PartitionField{ + SourceIDs: []int{2}, FieldID: 1000, Transform: iceberg.IdentityTransform{}, Name: "data", + }) + + specs := []struct { + name string + spec *iceberg.PartitionSpec + }{ + {"unpartitioned", iceberg.UnpartitionedSpec}, + {"partitioned", &identity}, + } + + filePathField, ok := iceberg.PositionalDeleteSchema.FindFieldByName("file_path") + require.True(t, ok) + posField, ok := iceberg.PositionalDeleteSchema.FindFieldByName("pos") + require.True(t, ok) + + for _, tc := range specs { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + + location := filepath.ToSlash(t.TempDir()) + schema := iceberg.NewSchema(0, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true}, + iceberg.NestedField{ID: 2, Name: "data", Type: iceberg.PrimitiveTypes.String, Required: true}, + ) + meta, err := table.NewMetadata(schema, tc.spec, table.UnsortedSortOrder, location, + iceberg.Properties{ + table.PropertyFormatVersion: "2", + table.WriteDeleteModeKey: table.WriteModeMergeOnRead, + table.MetricsModeColumnConfPrefix + ".file_path": "none", Review Comment: The none/none config here proves the override fires, but it doesn't reproduce #2141 itself. The original bug was the default `truncate(16)` clipping long paths, and since `positionDeleteMetricsProps` ignores table props entirely, both none and truncate pass for the same reason. I'd add a sub-case with no metrics props set (or the default) so a path longer than 16 chars actually exercises the truncate branch that regressed. The `require.Greater(len(dataPath), 16)` guard already assumes that scenario. ########## table/writer.go: ########## @@ -239,6 +239,26 @@ type dataFileWriter interface { writeFile(ctx context.Context, partitionValues map[int]any, task WriteTask) (iceberg.DataFile, error) } +// positionDeleteMetricsProps mirrors Java's MetricsConfig.forPositionDelete: +// position delete files keep full file_path and pos bounds whatever the table +// metrics settings are, so readers and conflict checks can resolve the data +// file a delete file targets from equal file_path bounds. +var positionDeleteMetricsProps = iceberg.Properties{ + DefaultWriteMetricsModeKey: string(tblutils.MetricModeNone), Review Comment: The comment frames this as bounds, but `default none` also zeroes value/null counts and column sizes for anything that isn't `file_path` or `pos`. Harmless on today's two-column `PositionalDeleteSchema` (and Java drops them too), but worth a line so whoever adds a `row` column later knows the drop is intentional. While we're here: it's a shared package-level map handed straight to `computeStatsPlan`, so a "must not be mutated" note would stop a future caller from corrupting it for every writer. ########## table/pos_delete_metrics_test.go: ########## @@ -0,0 +1,115 @@ +// 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_test + +import ( + "context" + "path/filepath" + "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" + iceio "github.com/apache/iceberg-go/io" + "github.com/apache/iceberg-go/table" + "github.com/stretchr/testify/require" +) + +// TestPositionDeleteFilesKeepFullFilePathBounds pins #2141: v2 position delete +// files keep exact file_path bounds and pos bounds whatever the table metrics +// settings are, as Java's MetricsConfig.forPositionDelete does. The table +// here asks for no file_path or pos metrics and data file paths are far +// longer than the default truncate(16). +func TestPositionDeleteFilesKeepFullFilePathBounds(t *testing.T) { + identity := iceberg.NewPartitionSpec(iceberg.PartitionField{ + SourceIDs: []int{2}, FieldID: 1000, Transform: iceberg.IdentityTransform{}, Name: "data", + }) + + specs := []struct { + name string + spec *iceberg.PartitionSpec + }{ + {"unpartitioned", iceberg.UnpartitionedSpec}, + {"partitioned", &identity}, + } + + filePathField, ok := iceberg.PositionalDeleteSchema.FindFieldByName("file_path") + require.True(t, ok) + posField, ok := iceberg.PositionalDeleteSchema.FindFieldByName("pos") + require.True(t, ok) + + for _, tc := range specs { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + + location := filepath.ToSlash(t.TempDir()) + schema := iceberg.NewSchema(0, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true}, + iceberg.NestedField{ID: 2, Name: "data", Type: iceberg.PrimitiveTypes.String, Required: true}, + ) + meta, err := table.NewMetadata(schema, tc.spec, table.UnsortedSortOrder, location, + iceberg.Properties{ + table.PropertyFormatVersion: "2", + table.WriteDeleteModeKey: table.WriteModeMergeOnRead, + table.MetricsModeColumnConfPrefix + ".file_path": "none", + table.MetricsModeColumnConfPrefix + ".pos": "none", + }) + require.NoError(t, err) + + metaLoc := location + "/metadata/v1.metadata.json" + fsF := func(context.Context) (iceio.IO, error) { return iceio.LocalFS{}, nil } + cat := &concurrentTestCatalog{metadata: meta, location: metaLoc, fsF: fsF} + tbl := table.New(table.Identifier{"db", "pos_delete_metrics"}, meta, metaLoc, fsF, cat) + + arrowSchema := arrow.NewSchema([]arrow.Field{ + {Name: "id", Type: arrow.PrimitiveTypes.Int64, Nullable: false}, + {Name: "data", Type: arrow.BinaryTypes.String, Nullable: false}, + }, nil) + data, err := array.TableFromJSON(memory.DefaultAllocator, arrowSchema, []string{ + `[{"id":1,"data":"a"},{"id":2,"data":"a"},{"id":3,"data":"b"},{"id":4,"data":"b"}]`, + }) + require.NoError(t, err) + defer data.Release() + + tbl, err = tbl.Append(ctx, array.NewTableReader(data, -1), nil) Review Comment: `array.NewTableReader(data, -1)` is created inline and never released, so its record batch leaks even though `data` has a deferred Release. I'd pull it into a local with its own defer: ```go rdr := array.NewTableReader(data, -1) defer rdr.Release() tbl, err = tbl.Append(ctx, rdr, nil) ``` Won't fail on the Go allocator, but worth matching how the rest of the package handles readers. ########## table/writer.go: ########## @@ -239,6 +239,26 @@ type dataFileWriter interface { writeFile(ctx context.Context, partitionValues map[int]any, task WriteTask) (iceberg.DataFile, error) } +// positionDeleteMetricsProps mirrors Java's MetricsConfig.forPositionDelete: +// position delete files keep full file_path and pos bounds whatever the table +// metrics settings are, so readers and conflict checks can resolve the data +// file a delete file targets from equal file_path bounds. +var positionDeleteMetricsProps = iceberg.Properties{ + DefaultWriteMetricsModeKey: string(tblutils.MetricModeNone), + MetricsModeColumnConfPrefix + ".file_path": string(tblutils.MetricModeFull), + MetricsModeColumnConfPrefix + ".pos": string(tblutils.MetricModeFull), +} + +// metricsPropsFor returns the properties that drive column metrics for files +// of the given content type. +func metricsPropsFor(content iceberg.ManifestEntryContent, props iceberg.Properties) iceberg.Properties { + if content == iceberg.EntryContentPosDeletes { + return positionDeleteMetricsProps + } + + return props Review Comment: The pos-delete branch is well covered, but nothing pins that equality deletes and data files still fall through to the table props here, so a regression that returned `positionDeleteMetricsProps` for every content type would sail through CI. A small table-driven test over `metricsPropsFor` (pos content gets the fixed props, equality and data get `props` back) would lock the passthrough. -- 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]
