fix(transforms): DayTransform

[jhump]

Its transform function returns an int32, but its ResultType method indicated it's a date. Per the spec, the transform function is correct: the result is an int32 number of days since epoch.

[zeroshade]

The date type is represented exactly as an int32 number of days since the epoch though. The reason for using the date type as the result is so that the transformation to a string happens properly etc.

[jhump]

Hmm, ok. The spec states that the result type of the day transform is int. And, per the spec for representing these in Avro, that the resulting Avro schema type (for the partition type in a manifest file) should just be int, without the date logical type.

so that the transformation to a string happens properly

Where does it need to be transformed to a string? The implementations of DayTransform.Transformer, DayTransform.Apply, and DayTransform.ToHumanStr all return/require int32 values, not Date. So this feels very inconsistent.

I ran into an issue with this when using Transformer to compute a partition value and storing that in a manifest entry: it was an int32 result, and encoding to Avro was unhappy because the computed partition schema wanted a Date instead. (At least I think that's what happened; I had to make this change in a fork to get day transforms working, but my recollection of these specifics could be incorrect.) I suppose an alternate fix in my own code base would be to special-case this transform and explicitly convert the result value to Date? But that seems problematic, that it requires special-casing that way to correctly build a manifest.

[zeroshade]

Where does it need to be transformed to a string? The implementations of DayTransform.Transformer, DayTransform.Apply, and DayTransform.ToHumanStr all return/require int32 values, not Date. So this feels very inconsistent.

That's a good point, you're right. I forgot about the ToHumanStr function which handles the only case where we'd need to do that conversion to string. So you're probably right, we can safely shift the result type to be int32.

[zeroshade]

Can you create a test case that hits the problem you found and is solved by this change?

[jhump]

Can you create a test case that hits the problem you found and is solved by this change?

Yep, will do.

[kevinjqliu]

https://iceberg.apache.org/spec/#partition-transforms

day transform result type should be int

[kevinjqliu]

this was originally Date probably because of pyiceberg
https://github.com/apache/iceberg-python/blob/c84017dc6fd17da0c077504ba5d8de587563ec9c/pyiceberg/transforms.py#L645-L651

[zeroshade]

That's probably where I got it from 😄

Then we're all good here once @jhump adds a test

[jhump]

I can probably get to adding a test early next week. Sorry for the delays. I got side-tracked with some other urgent stuff that popped up this week.

[kevinjqliu]

no need to apologize :P
that's just how OSS contributions go

[zeroshade]

Yup yup! No rush nor need for apologizing

[jhump]

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. 🤷

[jhump]

Hmmm, looks like a semantic merge conflict with #427...

[jhump]

@zeroshade, I finally had a second to look at this. It was a very simple/tiny fix, but I just didn't have a chance to even look at it before now. Tests and lint are green for me locally now.

[zeroshade]

Thanks!