laskoviymishka commented on code in PR #2034:
URL: https://github.com/apache/iceberg-go/pull/2034#discussion_r4064205951
##########
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:
this stops at construction, so it can't catch the actual failure.
`NewManifestWriter` defers the partition-stats error into `partitionStatsErr`
and only surfaces it from `ToManifestFile`, and `newPartitionFieldStat`
(`manifest.go:1544`) has no ns arm, so `ToManifestFile` still errors for
`timestamp_ns` and `timestamptz_ns`.
Could we extend one case to `Add` a data-file entry with a `TimestampNano`
partition value and then call `ToManifestFile`? As written this passes even
with the write path broken.
##########
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:
the blank line pulls `UUIDNode` into the same group as the two ns nodes,
which reads like UUID is related to timestamps. I'd either keep all five in one
group or give the ns pair its own. Right now the grouping looks accidental.
##########
internal/avro_schemas.go:
##########
@@ -122,6 +125,8 @@ var (
TimeSchema = mustSchema(TimeNode)
TimestampSchema = mustSchema(TimestampNode)
TimestampTzSchema = mustSchema(TimestampTzNode)
+ TimestampNsSchema = mustSchema(TimestampNsNode)
Review Comment:
`TimestampNsSchema` and `TimestampTzNsSchema` don't have any callers.
`partitionTypeToAvroSchema` uses the node vars above, not these. The sibling
`TimestampSchema`/`TimestampTzSchema` are unused too so it follows the existing
pattern, but I'd drop the pair until there's a real use-site. wdyt?
--
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]
