shoemoney commented on code in PR #2034:
URL: https://github.com/apache/iceberg-go/pull/2034#discussion_r4064379810
##########
schema_conversions_test.go:
##########
@@ -282,3 +283,76 @@ func TestNonDayInt32PartitionAvroIntEncoding(t *testing.T)
{
assert.IsType(t, int32(0), got, "plain Int32 partition must decode as
int32, not time.Time")
assert.Equal(t, int32(42), got)
}
+
+// TestNanosecondPartitionAvroSchema verifies that v3 nanosecond timestamp
+// types are accepted as partition types and round trip through Avro at
+// nanosecond precision. The read path in manifest.go already decodes
+// atype.TimestampNanos, so the write path has to emit it.
+func TestNanosecondPartitionAvroSchema(t *testing.T) {
+ cases := []struct {
+ name string
+ typ Type
+ }{
+ {"timestamp_ns", TimestampNsType{}},
+ {"timestamptz_ns", TimestampTzNsType{}},
+ }
+
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ partitionType := &StructType{FieldList: []NestedField{
+ {ID: 1000, Name: "ts_ns", Type: tc.typ,
Required: false},
+ }}
+
+ avroSchema, err :=
partitionTypeToAvroSchema(partitionType)
+ require.NoError(t, err)
+ require.NotNil(t, avroSchema)
+
+ // A value that is not a whole number of microseconds,
so a
+ // micros-based logical type would lose the trailing
123 ns.
+ want := time.Date(2024, time.March, 1, 12, 0, 0,
456789123, time.UTC)
+
+ encoded, err :=
avroSchema.Encode(map[string]any{"ts_ns": want})
+ require.NoError(t, err)
+
+ var decoded map[string]any
+ _, err = avroSchema.Decode(encoded, &decoded)
+ require.NoError(t, err)
+
+ got, ok := decoded["ts_ns"]
+ require.True(t, ok)
+ gotTime, isTime := got.(time.Time)
+ require.True(t, isTime, "nanosecond partition field
must decode as time.Time, got %T", got)
+ assert.Equal(t, want.UnixNano(),
gotTime.UTC().UnixNano(),
+ "nanosecond partition value must round trip
without precision loss")
+ })
+ }
+}
+
+// TestNewManifestWriterV3NanosecondPartition verifies that a v3 manifest can
+// be written for a table with an identity partition on a nanosecond timestamp
+// column. Before the nanosecond arms existed in partitionTypeToAvroSchema,
+// this failed with "unsupported partition type: timestamp_ns".
+func TestNewManifestWriterV3NanosecondPartition(t *testing.T) {
+ cases := []struct {
+ name string
+ typ Type
+ }{
+ {"timestamp_ns", TimestampNsType{}},
+ {"timestamptz_ns", TimestampTzNsType{}},
+ }
+
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ sc := NewSchema(0,
+ NestedField{ID: 1, Name: "ts", Type: tc.typ,
Required: true},
+ )
+ spec := NewPartitionSpecID(0,
+ PartitionField{FieldID: 1000, SourceIDs:
[]int{1}, Name: "ts_identity", Transform: IdentityTransform{}},
+ )
+
+ writer, err := NewManifestWriter(3, io.Discard, spec,
sc, 1)
+ require.NoError(t, err)
+ require.NotNil(t, writer)
Review Comment:
Fixed in 731e48b. Both nanosecond types now use TimestampNano partition
statistics. The regression adds positive and negative TimestampNano entries,
calls ToManifestFile, and checks the exact lower/upper bound bytes for both
types; it fails at finalization on the previous implementation and passes with
the fix. I also added both ns arms to the non-nullable schema test helper. Full
make test passes on both the prior head and this change (Go 1.26.1). Pinned
golangci-lint v2.12.2 reports only the same pre-existing gofmt finding in
table/snapshot_producers.go:1766 on both.
##########
internal/avro_schemas.go:
##########
@@ -122,6 +125,8 @@ var (
TimeSchema = mustSchema(TimeNode)
TimestampSchema = mustSchema(TimestampNode)
TimestampTzSchema = mustSchema(TimestampTzNode)
+ TimestampNsSchema = mustSchema(TimestampNsNode)
Review Comment:
Removed the unused TimestampNsSchema and TimestampTzNsSchema variables in
731e48b; the node definitions remain in use by partition schema conversion.
Both nullable and non-nullable nanosecond schema round-trip tests pass.
##########
internal/avro_schemas.go:
##########
@@ -101,7 +101,10 @@ var (
TimeNode = avro.SchemaNode{Type: atype.Long, LogicalType:
atype.TimeMicros}
TimestampNode = avro.SchemaNode{Type: atype.Long, LogicalType:
atype.TimestampMicros, Props: map[string]any{"adjust-to-utc": false}}
TimestampTzNode = avro.SchemaNode{Type: atype.Long, LogicalType:
atype.TimestampMicros, Props: map[string]any{"adjust-to-utc": true}}
- UUIDNode = avro.SchemaNode{Type: atype.Fixed, Name:
"uuid_fixed", Size: 16, LogicalType: atype.UUID}
+
+ TimestampNsNode = avro.SchemaNode{Type: atype.Long, LogicalType:
atype.TimestampNanos, Props: map[string]any{"adjust-to-utc": false}}
+ TimestampTzNsNode = avro.SchemaNode{Type: atype.Long, LogicalType:
atype.TimestampNanos, Props: map[string]any{"adjust-to-utc": true}}
+ UUIDNode = avro.SchemaNode{Type: atype.Fixed, Name:
"uuid_fixed", Size: 16, LogicalType: atype.UUID}
Review Comment:
Separated UUIDNode from the nanosecond node pair in 731e48b and formatted
the changed files. The ns pair now forms its own group.
--
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]
