blackmwk commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3901858716
##########
crates/integration_tests/tests/partition_path_temporal_transforms.rs:
##########
Review Comment:
I don't think we should add this test, it unrelated to this issue, the
partition path is determined by location generate.
##########
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:
This is breaking. Here you cast it to primitive literal unconditionally, but
if `field_type` is non primitive, it will still return null.
--
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]
