blackmwk commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3812021973
##########
crates/iceberg/src/spec/transform.rs:
##########
@@ -137,11 +139,40 @@ pub enum Transform {
impl Transform {
/// Returns a human-readable String representation of a transformed value.
+ ///
+ /// The temporal transforms store an ordinal count since the Unix epoch,
and
+ /// this method renders that count as a date so that partition paths and
+ /// snapshot summary keys match the Java reference implementation:
+ ///
+ /// | Transform | Format | Example |
+ /// |-----------|-----------------|-----------------|
+ /// | `Year` | `yyyy` | `2017` |
+ /// | `Month` | `yyyy-MM` | `2017-06` |
+ /// | `Day` | `yyyy-MM-dd` | `2017-06-15` |
+ /// | `Hour` | `yyyy-MM-dd-HH` | `2017-06-15-16` |
+ ///
+ /// `Void` renders as `null`, as does an absent value for any transform.
pub fn to_human_string(&self, field_type: &Type, value: Option<&Literal>)
-> String {
let Some(value) = value else {
return "null".to_string();
};
+ // Year, Month and Hour have an `int` result type holding an ordinal
count
+ // since the Unix epoch, so the datum path below would render the raw
count
+ // rather than a date. The Java reference implementation instead
overrides
+ // `toHumanString` on each of these transforms (`Years`, `Months`,
`Hours`)
+ // 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 {
Review Comment:
I prefer to move this under line 181 so that it's more consistent.
##########
crates/iceberg/src/spec/transform.rs:
##########
@@ -1119,4 +1192,81 @@ mod tests {
check_boundary(PredicateOperator::GreaterThanOrEq, datum.clone(),
datum);
}
}
+
+ #[test]
+ fn test_human_year() {
Review 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.
--
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]
