You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Kurtiscwright (via GitHub)" <gi...@apache.org> on 2026/08/19 06:54:44 UTC
[PR] Fix partition transform align with java [iceberg-rust]
Kurtiscwright opened a new pull request, #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #3021
## What changes are included in this PR?
<!--
Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates.
-->
Moved transform to using from EPOCH time on Year, Month, and Hour (Day already aligns with Java).
## Are these changes tested?
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Added new unit tests to validate these changes
## AI Disclosure
<!--
https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions
-->
Use Claude to write unit tests and comments
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Fix partition transform align with java [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Fix partition transform align with java [iceberg-rust]
Posted by "Kurtiscwright (via GitHub)" <gi...@apache.org>.
Kurtiscwright commented on PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#issuecomment-5350716777
> Thanks @Kurtiscwright for this pr!
Thank you for the quick review, will get a revision out asap.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Fix partition transform align with java [iceberg-rust]
Posted by "Kurtiscwright (via GitHub)" <gi...@apache.org>.
Kurtiscwright commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3818293439
##########
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:
The doc test is a great call, thank you for reminding me. I will make doc tests for these private helpers.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(partition): Align DateTime Transform to Java's Implementation [iceberg-rust]
Posted by "Kurtiscwright (via GitHub)" <gi...@apache.org>.
Kurtiscwright commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3909509212
##########
crates/integration_tests/tests/partition_path_temporal_transforms.rs:
##########
Review Comment:
That is fair, I will remove it 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(partition): Align DateTime Transform to Java's Implementation [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Fix partition transform align with java [iceberg-rust]
Posted by "Kurtiscwright (via GitHub)" <gi...@apache.org>.
Kurtiscwright commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3818290590
##########
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:
That makes sense, I will move it in the next revision.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(partition): Align DateTime Transform to Java's Implementation [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3913972994
##########
crates/iceberg/src/spec/transform.rs:
##########
Review Comment:
Nice, see the new doctest running.
```
test crates/iceberg/src/spec/transform.rs - spec::transform::Transform::to_human_string (line 161) ... ok
```
https://github.com/apache/iceberg-rust/actions/runs/33420936834/job/99583209396?pr=3022#step:7:86
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(partition): Align DateTime Transform to Java's Implementation [iceberg-rust]
Posted by "Kurtiscwright (via GitHub)" <gi...@apache.org>.
Kurtiscwright commented on PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#issuecomment-5484131938
@blackmwk @kevinjqliu @dannycjones Thank you for your patience, I was traveling when the reviews came in. All comments should be addressed. I added new integration tests for Spark-Java Equivalence testing. Once this PR gets merged, I will look to expand those tests and add them to the Github Action CI.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(partition): Align DateTime Transform to Java's Implementation [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3852359680
##########
crates/iceberg/src/spec/transform.rs:
##########
@@ -1119,4 +1192,81 @@ mod tests {
check_boundary(PredicateOperator::GreaterThanOrEq, datum.clone(), datum);
}
}
+
+ #[test]
+ fn test_human_year() {
+ assert_eq!(Transform::human_year(-1970), "0000");
+ assert_eq!(Transform::human_year(-1), "1969");
+ assert_eq!(Transform::human_year(0), "1970");
+ assert_eq!(Transform::human_year(47), "2017");
+ }
+
+ #[test]
+ fn test_human_month() {
+ assert_eq!(Transform::human_month(-1970 * 12), "0000-01");
+ assert_eq!(Transform::human_month(-13), "1968-12");
+ assert_eq!(Transform::human_month(-12), "1969-01");
+ assert_eq!(Transform::human_month(-1), "1969-12");
+ assert_eq!(Transform::human_month(0), "1970-01");
+ assert_eq!(Transform::human_month(11), "1970-12");
+ assert_eq!(Transform::human_month(12), "1971-01");
+ assert_eq!(Transform::human_month(569), "2017-06");
+ }
+
+ #[test]
+ fn test_human_hour() {
+ assert_eq!(Transform::human_hour(-24), "1969-12-31-00");
+ assert_eq!(Transform::human_hour(-1), "1969-12-31-23");
+ assert_eq!(Transform::human_hour(0), "1970-01-01-00");
+ assert_eq!(Transform::human_hour(23), "1970-01-01-23");
+ assert_eq!(Transform::human_hour(24), "1970-01-02-00");
+ assert_eq!(Transform::human_hour(1000), "1970-02-11-16");
+ }
+
+ #[test]
+ fn test_to_human_string_dispatches_on_transform() {
+ let int = Type::Primitive(PrimitiveType::Int);
+ let date = Type::Primitive(PrimitiveType::Date);
+
+ 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::Hour.to_human_string(&int, Some(&Literal::int(1000))),
+ "1970-02-11-16"
+ );
+
+ // Day needs no dispatch arm: its result type is `date`, so the datum path
+ // already renders a calendar date. Asserted so that a later reader knows
+ // Day was considered rather than overlooked.
+ assert_eq!(
+ Transform::Day.to_human_string(&date, Some(&Literal::int(0))),
+ "1970-01-01"
+ );
Review Comment:
Is there any risk that an `int` gets passed in here for a `Day` transform?
Other implementations of Iceberg (including Rust previously) have used `int` to represent day rather than date.
I think my primary worry is for anyone who is running something copy-on-write which could take an int-encoded day transform and write it back out as int-encoded rather than date-encoded. The impact would be small though IMO as this is not what Iceberg filtering is based on.
##########
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 think the body of `test_to_human_string_dispatches_on_transform` would make a good doctest if cleaned up a bit. Basically, something that shows clearly how we transform a bunch of types and their transforms.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Fix partition transform align with java [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3828737765
##########
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:
This is still not resolved
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(partition): Align DateTime Transform to Java's Implementation [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3912643059
##########
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:
Thanks for explaination, I think you are right.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(partition): Align DateTime Transform to Java's Implementation [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3852603955
##########
crates/iceberg/src/spec/transform.rs:
##########
@@ -1119,4 +1192,81 @@ mod tests {
check_boundary(PredicateOperator::GreaterThanOrEq, datum.clone(), datum);
}
}
+
+ #[test]
+ fn test_human_year() {
+ assert_eq!(Transform::human_year(-1970), "0000");
+ assert_eq!(Transform::human_year(-1), "1969");
+ assert_eq!(Transform::human_year(0), "1970");
+ assert_eq!(Transform::human_year(47), "2017");
+ }
+
+ #[test]
+ fn test_human_month() {
+ assert_eq!(Transform::human_month(-1970 * 12), "0000-01");
+ assert_eq!(Transform::human_month(-13), "1968-12");
+ assert_eq!(Transform::human_month(-12), "1969-01");
+ assert_eq!(Transform::human_month(-1), "1969-12");
+ assert_eq!(Transform::human_month(0), "1970-01");
+ assert_eq!(Transform::human_month(11), "1970-12");
+ assert_eq!(Transform::human_month(12), "1971-01");
+ assert_eq!(Transform::human_month(569), "2017-06");
+ }
+
+ #[test]
+ fn test_human_hour() {
+ assert_eq!(Transform::human_hour(-24), "1969-12-31-00");
+ assert_eq!(Transform::human_hour(-1), "1969-12-31-23");
+ assert_eq!(Transform::human_hour(0), "1970-01-01-00");
+ assert_eq!(Transform::human_hour(23), "1970-01-01-23");
+ assert_eq!(Transform::human_hour(24), "1970-01-02-00");
+ assert_eq!(Transform::human_hour(1000), "1970-02-11-16");
+ }
+
+ #[test]
+ fn test_to_human_string_dispatches_on_transform() {
+ let int = Type::Primitive(PrimitiveType::Int);
+ let date = Type::Primitive(PrimitiveType::Date);
+
+ 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::Hour.to_human_string(&int, Some(&Literal::int(1000))),
+ "1970-02-11-16"
+ );
+
+ // Day needs no dispatch arm: its result type is `date`, so the datum path
+ // already renders a calendar date. Asserted so that a later reader knows
+ // Day was considered rather than overlooked.
+ assert_eq!(
+ Transform::Day.to_human_string(&date, Some(&Literal::int(0))),
+ "1970-01-01"
+ );
Review Comment:
I think the likelihood here of someone running into this is minimal. I don't expect people not to use `PartitionSpec::partition_type` function.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(partition): Align DateTime Transform to Java's Implementation [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY merged PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(partition): Align DateTime Transform to Java's Implementation [iceberg-rust]
Posted by "Kurtiscwright (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Fix partition transform align with java [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#issuecomment-5359197485
think its a good idea to include a regression test utilizing spark?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Fix partition transform align with java [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3022:
URL: https://github.com/apache/iceberg-rust/pull/3022#discussion_r3828736222
##########
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:
Actually I'm not say doc tests for private helpers, I mean add some on pub fn.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org