laskoviymishka commented on code in PR #1219: URL: https://github.com/apache/iceberg-go/pull/1219#discussion_r3457454323
########## manifest_day_partition_test.go: ########## @@ -0,0 +1,156 @@ +// 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 iceberg + +import ( + "bytes" + "testing" + + "github.com/apache/iceberg-go/internal" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/twmb/avro" + "github.com/twmb/avro/atype" + "github.com/twmb/avro/ocf" +) + +// dayPartitionFieldID is the partition field id used by the day-transform tests. +const dayPartitionFieldID = 1000 + +// writeDayPartitionManifest encodes a single-entry v2 manifest whose only +// partition field ("ts_day", produced by a day transform) is serialized using +// partFieldNode. By varying partFieldNode between a plain Avro int and an Avro +// int with the "date" logical type, the same day value can be written in either +// of the two encodings that Iceberg engines produce in the wild. +func writeDayPartitionManifest(t *testing.T, partFieldNode avro.SchemaNode, dayValue int32) ([]byte, ManifestFile) { + t.Helper() + + spec := NewPartitionSpecID(0, PartitionField{ + FieldID: dayPartitionFieldID, + SourceIDs: []int{1}, + Name: "ts_day", + Transform: DayTransform{}, + }) + + builder, err := NewDataFileBuilder( + spec, + EntryContentData, + "s3://bucket/namespace/table/data/00000-0-day.parquet", + ParquetFile, + map[int]any{dayPartitionFieldID: dayValue}, + map[int]string{}, + map[int]int{}, + 100, + 1024, + ) + require.NoError(t, err) + + snapshotID := int64(42) + seqNum := int64(1) + entry := NewManifestEntry(EntryStatusADDED, &snapshotID, &seqNum, &seqNum, builder.Build()) + + // Build the partition record schema (r102) by hand so the test controls the + // on-disk Avro encoding of the partition field, independent of how the + // writer would model a day transform's result type. + partNode := avro.SchemaNode{ + Type: atype.Record, + Name: "r102", + Fields: []avro.SchemaField{ + {Name: "ts_day", Type: partFieldNode, Props: internal.WithFieldID(dayPartitionFieldID)}, + }, + } + partSchema, err := partNode.Schema() + require.NoError(t, err) + + entrySchema, err := internal.NewManifestEntrySchema(partSchema, 2) + require.NoError(t, err) + + var buf bytes.Buffer + wr, err := ocf.NewWriter(&buf, entrySchema, + // WithSchema writes the explicit JSON schema, preserving the field-id + // props the manifest reader relies on to map partition values. + ocf.WithSchema(entrySchema.String()), + ocf.WithMetadata(map[string][]byte{ + "format-version": []byte("2"), + "content": []byte("data"), + })) + require.NoError(t, err) + require.NoError(t, wr.Encode(entry)) + require.NoError(t, wr.Close()) + + file := &manifestFile{ + Path: "s3://bucket/namespace/table/metadata/00000-day.avro", + SpecID: 0, + Content: ManifestContentData, + } + file.setVersion(2) + + return buf.Bytes(), file +} + +// TestDayPartitionReadsAvroIntAndDate verifies that iceberg-go can read a +// day-transform partition value regardless of whether it was written as a plain +// Avro int or as an Avro int carrying the "date" logical type. Other Iceberg +// implementations (Java, PyIceberg, iceberg-rust) and engines (Trino, Spark) +// accept both encodings, and so must iceberg-go; a manifest normalized to the +// date encoding by one writer must remain readable here. See apache/iceberg#16414. +// +// The two encodings share the same physical layout (a zig-zag int), but the +// Avro decoder hands back a different Go type for each: a plain int decodes to +// int32, while a date-logical int decodes to time.Time, which the manifest +// reader converts to a Date. Both cases must surface the same logical day value. +func TestDayPartitionReadsAvroIntAndDate(t *testing.T) { + const dayValue = int32(19000) // 2022-01-08 + + cases := []struct { + name string + partNode avro.SchemaNode + want any + }{ + { + name: "avro int (no logical type)", + partNode: internal.NullableNode(internal.IntNode), + want: dayValue, Review Comment: The `want` field asserts different Go types for the two encodings — `int32` for plain-int, `Date` for date-logical — but the normalization layer exists specifically to erase that difference (apache/iceberg#16446). Callers pattern-matching on `Partition()` values (predicate eval, projection, CDC) shouldn't have to handle both `int32` and `Date` for the same logical field, and a test that accepts both teaches the opposite. It also slips past the assertion: `assert.IsType` is non-fatal, and `assert.EqualValues(t, dayValue, got)` coerces `Date(19000)` and `int32(19000)` to the same number, so a regression to raw `int32` goes unreported even on the case that's supposed to catch it. I'd drop `want` and assert uniformly for both cases — `assert.IsType(t, Date(0), got)` plus `assert.Equal(t, Date(dayValue), got)` for typed equality. That's what's on `main` today. ########## manifest_day_partition_test.go: ########## @@ -0,0 +1,156 @@ +// 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 iceberg + +import ( + "bytes" + "testing" + + "github.com/apache/iceberg-go/internal" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/twmb/avro" + "github.com/twmb/avro/atype" + "github.com/twmb/avro/ocf" +) + +// dayPartitionFieldID is the partition field id used by the day-transform tests. +const dayPartitionFieldID = 1000 + +// writeDayPartitionManifest encodes a single-entry v2 manifest whose only +// partition field ("ts_day", produced by a day transform) is serialized using +// partFieldNode. By varying partFieldNode between a plain Avro int and an Avro +// int with the "date" logical type, the same day value can be written in either +// of the two encodings that Iceberg engines produce in the wild. +func writeDayPartitionManifest(t *testing.T, partFieldNode avro.SchemaNode, dayValue int32) ([]byte, ManifestFile) { + t.Helper() + + spec := NewPartitionSpecID(0, PartitionField{ + FieldID: dayPartitionFieldID, + SourceIDs: []int{1}, + Name: "ts_day", + Transform: DayTransform{}, + }) + + builder, err := NewDataFileBuilder( + spec, + EntryContentData, + "s3://bucket/namespace/table/data/00000-0-day.parquet", + ParquetFile, + map[int]any{dayPartitionFieldID: dayValue}, + map[int]string{}, + map[int]int{}, + 100, + 1024, + ) + require.NoError(t, err) + + snapshotID := int64(42) + seqNum := int64(1) + entry := NewManifestEntry(EntryStatusADDED, &snapshotID, &seqNum, &seqNum, builder.Build()) + + // Build the partition record schema (r102) by hand so the test controls the + // on-disk Avro encoding of the partition field, independent of how the + // writer would model a day transform's result type. + partNode := avro.SchemaNode{ + Type: atype.Record, + Name: "r102", + Fields: []avro.SchemaField{ + {Name: "ts_day", Type: partFieldNode, Props: internal.WithFieldID(dayPartitionFieldID)}, + }, + } + partSchema, err := partNode.Schema() + require.NoError(t, err) + + entrySchema, err := internal.NewManifestEntrySchema(partSchema, 2) + require.NoError(t, err) + + var buf bytes.Buffer + wr, err := ocf.NewWriter(&buf, entrySchema, + // WithSchema writes the explicit JSON schema, preserving the field-id + // props the manifest reader relies on to map partition values. + ocf.WithSchema(entrySchema.String()), + ocf.WithMetadata(map[string][]byte{ + "format-version": []byte("2"), + "content": []byte("data"), + })) + require.NoError(t, err) + require.NoError(t, wr.Encode(entry)) + require.NoError(t, wr.Close()) + + file := &manifestFile{ + Path: "s3://bucket/namespace/table/metadata/00000-day.avro", + SpecID: 0, + Content: ManifestContentData, + } + file.setVersion(2) + + return buf.Bytes(), file +} + +// TestDayPartitionReadsAvroIntAndDate verifies that iceberg-go can read a +// day-transform partition value regardless of whether it was written as a plain +// Avro int or as an Avro int carrying the "date" logical type. Other Iceberg +// implementations (Java, PyIceberg, iceberg-rust) and engines (Trino, Spark) +// accept both encodings, and so must iceberg-go; a manifest normalized to the +// date encoding by one writer must remain readable here. See apache/iceberg#16414. Review Comment: Small cross-reference inconsistency: the docstring cites #16414 (the issue) while the production code at manifest.go:436 cites #16446 (the clarification PR that mandates the normalization). Worth pointing both at #16446 so the test and the code it guards line up. Minor — fold it into the rebase. ########## manifest_day_partition_test.go: ########## @@ -0,0 +1,156 @@ +// 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 iceberg + +import ( + "bytes" + "testing" + + "github.com/apache/iceberg-go/internal" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/twmb/avro" + "github.com/twmb/avro/atype" + "github.com/twmb/avro/ocf" +) + +// dayPartitionFieldID is the partition field id used by the day-transform tests. +const dayPartitionFieldID = 1000 + +// writeDayPartitionManifest encodes a single-entry v2 manifest whose only +// partition field ("ts_day", produced by a day transform) is serialized using +// partFieldNode. By varying partFieldNode between a plain Avro int and an Avro +// int with the "date" logical type, the same day value can be written in either +// of the two encodings that Iceberg engines produce in the wild. +func writeDayPartitionManifest(t *testing.T, partFieldNode avro.SchemaNode, dayValue int32) ([]byte, ManifestFile) { + t.Helper() + + spec := NewPartitionSpecID(0, PartitionField{ + FieldID: dayPartitionFieldID, + SourceIDs: []int{1}, + Name: "ts_day", + Transform: DayTransform{}, + }) + + builder, err := NewDataFileBuilder( + spec, + EntryContentData, + "s3://bucket/namespace/table/data/00000-0-day.parquet", + ParquetFile, + map[int]any{dayPartitionFieldID: dayValue}, + map[int]string{}, + map[int]int{}, + 100, + 1024, + ) + require.NoError(t, err) + + snapshotID := int64(42) + seqNum := int64(1) + entry := NewManifestEntry(EntryStatusADDED, &snapshotID, &seqNum, &seqNum, builder.Build()) + + // Build the partition record schema (r102) by hand so the test controls the + // on-disk Avro encoding of the partition field, independent of how the + // writer would model a day transform's result type. + partNode := avro.SchemaNode{ + Type: atype.Record, + Name: "r102", + Fields: []avro.SchemaField{ + {Name: "ts_day", Type: partFieldNode, Props: internal.WithFieldID(dayPartitionFieldID)}, + }, + } + partSchema, err := partNode.Schema() + require.NoError(t, err) + + entrySchema, err := internal.NewManifestEntrySchema(partSchema, 2) + require.NoError(t, err) + + var buf bytes.Buffer + wr, err := ocf.NewWriter(&buf, entrySchema, + // WithSchema writes the explicit JSON schema, preserving the field-id + // props the manifest reader relies on to map partition values. + ocf.WithSchema(entrySchema.String()), + ocf.WithMetadata(map[string][]byte{ + "format-version": []byte("2"), + "content": []byte("data"), Review Comment: I think this test doesn't exercise what it's documenting. The OCF metadata here only carries `format-version` and `content`, no `partition-spec`. `applyDayTransformDates` (manifest.go:711) returns immediately when `partition-spec` is absent, so for the plain-int case the day-transform field is never forced to `date` — the reader just hands back the raw `int32`. The assertion below passes because normalization was skipped, not because it worked; this test would still pass if `applyDayTransformDates` were deleted entirely. That's also the scenario #1199 actually cares about: real manifests from older iceberg-go writers carry `partition-spec` but encode the field as plain int. To reach the path under test we need to write it here: ```go ocf.WithMetadata(map[string][]byte{ "format-version": []byte("2"), "content": []byte("data"), "partition-spec": specFieldsJSON, // marshaled PartitionField slice "partition-spec-id": []byte("0"), }) ``` This is exactly the change that landed on `main` in #1218 (commit `41131be`). -- 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]
