Fix partition transform align with java - #3022
Conversation
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @Kurtiscwright for this pr!
| // to format the ordinal. Day needs no arm: its result type is `date`, so | ||
| // the datum already renders as a calendar date. Any literal that is not an | ||
| // `int` falls through to the existing behaviour. | ||
| if let Literal::Primitive(PrimitiveLiteral::Int(ordinal)) = value { |
There was a problem hiding this comment.
I prefer to move this under line 181 so that it's more consistent.
There was a problem hiding this comment.
That makes sense, I will move it in the next revision.
| } | ||
|
|
||
| #[test] | ||
| fn test_human_year() { |
There was a problem hiding this comment.
I'm not a big fan of adding tests for private fn, I prefer to test against pub fun, e.g. to_human_string. Ideally, we could add doc to to_human_string which includes runnable codes to demonstrate the result.
There was a problem hiding this comment.
The doc test is a great call, thank you for reminding me. I will make doc tests for these private helpers.
Thank you for the quick review, will get a revision out asap. |
3b4055e to
60e466d
Compare
|
think its a good idea to include a regression test utilizing spark? |
3 participants
Which issue does this PR close?
What changes are included in this PR?
Moved transform to using from EPOCH time on Year, Month, and Hour (Day already aligns with Java).
Are these changes tested?
Added new unit tests to validate these changes
AI Disclosure
Use Claude to write unit tests and comments