laskoviymishka commented on code in PR #1218:
URL: https://github.com/apache/iceberg-go/pull/1218#discussion_r3430004451
##########
manifest.go:
##########
@@ -422,6 +422,32 @@ func getFieldIDMap(sc *avro.Schema) (map[string]int,
map[int]string, map[int]int
return result, logicalTypes, fixedSizes
}
+// applyDayTransformDates marks every day(...) partition field described by the
Review Comment:
The godoc explains the why well, but it frames the two-encoding acceptance
as de-facto cross-engine behavior. As of apache/iceberg#16446 it's no longer
just convention — the spec's transform table now lists `day` with result type
`date`, plus note [1]: "Readers must also accept `int` values for the `day`
transform, interpreting each integer as a `date` represented by the number of
days since 1970-01-01." That's exactly what this function implements.
I'd cite that note here (and in the test's rationale comment, which
currently says "other Iceberg implementations and engines accept both
encodings") so the normalization reads as spec-mandated rather than a
compatibility nicety — it also tells the next reader this is required behavior,
not something safe to drop.
##########
manifest.go:
##########
@@ -1858,8 +1890,11 @@ func (d *dataFile) convertAvroValueToIcebergType(v any,
fieldID int) any {
if val, ok := v.(time.Time); ok {
return Date(val.Truncate(24*time.Hour).Unix() /
int64((time.Hour * 24).Seconds()))
}
+ if val, ok := v.(int32); ok {
+ return Date(val)
+ }
- return Date(v.(int32))
+ return v
Review Comment:
The `int32` guard is the right addition. My one concern is the `return v` it
falls through to: this slot is for `atype.Date`, so a caller will do
`partition[fieldID].(iceberg.Date)` and panic far from here if `v` is ever
neither `time.Time` nor `int32`.
In practice this is unreachable today — `twmb/avro` decodes an int+date
logical type to `time.Time` and a plain int to `int32`, so there's no third
type to land here. But the old `Date(v.(int32))` panicked loudly at the
conversion site on anything unexpected, and `return v` trades that for a silent
wrong-typed value if a decoder change ever fires it.
I'd either keep the loud-failure behavior (a panic or a typed error) or add
a one-line comment stating why this is unreachable, so nobody later "fixes" it
by adding a guard that reintroduces #1200. 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]
