jhump commented on code in PR #423:
URL: https://github.com/apache/iceberg-go/pull/423#discussion_r2085734360
##########
transforms.go:
##########
@@ -720,7 +720,7 @@ func (t DayTransform) MarshalText() ([]byte, error) {
func (DayTransform) String() string { return "day" }
-func (DayTransform) ResultType(Type) Type { return PrimitiveTypes.Date }
+func (DayTransform) ResultType(Type) Type { return PrimitiveTypes.Int32 }
Review Comment:
Okay, so I've pushed a test. But, unfortunately, it did _not_ reproduce the
issue I originally encountered.
I stepped through everything in this test with a debugger to figure out what
was going on, since I was surprised it passed. It turns out that the avro
encoder will happily accept an `int32` value (and also accept an
`iceberg.Date`, since its underlying kind is `int32`) even for a field with the
"date" logical type. It is a little odd that it doesn't survive a round-trip:
the tests writes an `int32` but gets back an `iceberg.Date` when reading that
just-written file. But I didn't add an assertion to verify that all values
round-trip correctly because (sigh 😢) none of the int32 values do: while this
one gets deserialized as a `time.Time` (and then converted to `iceberg.Date` in
`dataFile.initializeMapData`), all of the others get deserialized as `int`
(instead of `int32`). ☹️
I did a little spelunking in my own codebase, to try to figure out what
error I saw when I originally encountered an issue. It turns out that my
application has extra checks before writing a manifest, which verify that the
partition data map in each data file has the expected types of values. The map
had an `int32`, but it was expecting a `time.Time` (what would have been loaded
directly from Avro, due to the "date" logical type in the schema) or an
`iceberg.Date` due to `DayTransform.ResultType()` returning `Date` instead of
`Int32`. So it was my own code that was returning an error, not code in this
library.
I still think this change makes sense, despite the fact that things work
without it. It is possible that integrating with something that tries to read
the manifest could fail since the type in the manifest doesn't match the
expected type per the Iceberg spec. 🤷
--
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]
