Kurtiscwright commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3909550205
##########
crates/iceberg/src/spec/transform.rs:
##########
@@ -137,24 +142,138 @@ 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.
+ ///
+ /// # Example
+ ///
+ /// ```
+ /// use iceberg::spec::{Literal, PrimitiveType, Transform, Type};
+ ///
+ /// let int = Type::Primitive(PrimitiveType::Int);
+ /// let date = Type::Primitive(PrimitiveType::Date);
+ ///
+ /// // A stored value carries no logical type of its own. For transforms
that do
+ /// // not format it themselves, the declared field type decides how it
renders.
+ /// let stored = Literal::int(17332);
+ /// assert_eq!(
+ /// Transform::Identity.to_human_string(&int, Some(&stored)),
+ /// "17332"
+ /// );
+ /// assert_eq!(
+ /// Transform::Identity.to_human_string(&date, Some(&stored)),
+ /// "2017-06-15"
+ /// );
+ ///
+ /// // The temporal transforms format their own ordinal and ignore the
declared
+ /// // type. All four ordinals below are the same instant,
2017-06-15T16:00:00Z,
+ /// // counted at four granularities.
+ /// assert_eq!(
+ /// Transform::Year.to_human_string(&int, Some(&Literal::int(47))),
+ /// "2017"
+ /// );
+ /// assert_eq!(
+ /// Transform::Month.to_human_string(&int, Some(&Literal::int(569))),
+ /// "2017-06"
+ /// );
+ /// assert_eq!(
+ /// Transform::Day.to_human_string(&int, Some(&Literal::int(17332))),
+ /// "2017-06-15"
+ /// );
+ /// assert_eq!(
+ /// Transform::Hour.to_human_string(&int, Some(&Literal::int(415984))),
+ /// "2017-06-15-16"
+ /// );
+ ///
+ /// // `Void` and an absent value render as `null`.
+ /// assert_eq!(
+ /// Transform::Void.to_human_string(&int, Some(&Literal::int(47))),
+ /// "null"
+ /// );
+ /// assert_eq!(Transform::Year.to_human_string(&int, None), "null");
+ /// ```
pub fn to_human_string(&self, field_type: &Type, value: Option<&Literal>)
-> String {
- let Some(value) = value else {
+ let Some(value) = value.and_then(Literal::as_primitive_literal) else {
Review Comment:
I don't believe it does. My understanding is that `.and_then()` is
equivalent to `.map()`, but instead of returning nested results
`Option<Option<T>>` it returns a flattened result `Option<T>`. My thinking is
that this is syntactic sugar that could be expanded to look like:
``` rust
let value = match value {
None => return "null".to_string(),
Some(literal) => match literal.as_primitive_literal() {
Some(primitive) => primitive,
None => return "null".to_string(),
},
};
```
with the in-line `else` supplying the None branches.
I referenced this
https://doc.rust-lang.org/rust-by-example/error/option_unwrap/and_then.html
If you disagree or feel it increases the maintenance burden, I am happy to
change the code in a follow up commit.
--
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]
