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 for each data file has the correct/expected types of values. The
map had an `int32` value, but because `DayTransform.ResultType()` returned
`Date` instead of `Int32` the checks 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`. 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 do
actually 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
quite 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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
