Conversation
|
@kevinjqliu when you have a chance, could you please take a look at this pr and let me know if any update is needed? |
Fokko
left a comment
There was a problem hiding this comment.
Thanks for tackling this!
The description says the fixed 9-digit output "matches the Java DateTimeUtil.nanosToIsoTimestamp behavior… with no trailing-zero trimming", but Java actually does the opposite. It trims trailing zeros (and drops the fraction entirely for a whole second). nanosToIsoTimestamp formats via DateTimeFormatter.ISO_LOCAL_DATE_TIME, whose appendFraction(NANO_OF_SECOND, 0, 9, true) omits trailing zeros.
Identity transform on timestamp_ns / timestamptz_ns rendered the raw integer for to_human_string, producing non-ISO partition directory paths (e.g. ts=1740342104375612001) unlike the microsecond timestamp types which produce ISO-8601 strings. _int_to_human_string dispatched Date/Time/Timestamp/Timestamptz but had no registration for TimestampNanoType / TimestamptzNanoType, so those values fell through to str(value). Add to_human_timestamp_ns and to_human_timestamptz_ns helpers that render full 9 fractional-digit ISO-8601 strings (matching the Java Transforms behaviour), and register them for the nanosecond timestamp types. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved blocking issues were identified.
Pull request overview
Fixes ISO-8601 formatting for identity-partitioned nanosecond timestamps.
Changes:
- Adds nanosecond timestamp formatting helpers.
- Registers handlers for nanosecond timestamp types.
- Adds unit and partition-path coverage.
File summaries
| File | Description |
|---|---|
tests/utils/test_datetime.py |
Tests nanosecond formatting edge cases. |
tests/test_transforms.py |
Tests identity transform output. |
tests/table/test_partitioning.py |
Tests generated partition paths. |
pyiceberg/utils/datetime.py |
Adds nanosecond ISO formatting. |
pyiceberg/transforms.py |
Registers nanosecond type handlers. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Rationale for this change
An identity transform on a
timestamp_ns/timestamptz_nscolumn renders theraw integer value in its human string, so identity partition directory paths
come out as
ts=1740342104375612001instead of an ISO-8601 timestamp. Themicrosecond
timestamp/timestamptztypes render correctly (e.g.ts=2025-02-23T20:21:44.375612), so this is an inconsistency in the nanosecondtimestamp types added for the v3 spec.
The cause is in
_int_to_human_string(pyiceberg/transforms.py): it dispatchesDateType,TimeType,TimestampType, andTimestamptzType, but has noregistration for
TimestampNanoType/TimestamptzNanoType, so those valuesfall through to the base handler
str(value). BecauseIdentityTransform.to_human_stringandPartitionSpec.partition_to_path(
pyiceberg/partitioning.py) both route through_int_to_human_string, anidentity-partitioned nanosecond timestamp column produces a raw-integer
partition path.
This change adds
to_human_timestamp_ns/to_human_timestamptz_nshelpers inpyiceberg/utils/datetime.pyand registers them for the two nanosecondtimestamp types. Python's
datetimeonly supports microsecond precision, so thehelpers render the microsecond instant at a fixed 6 fractional digits and append
the remaining 3 sub-microsecond digits, yielding a full 9 fractional-digit
ISO-8601 string with no trailing-zero trimming. This matches the Java
DateTimeUtil.nanosToIsoTimestamp/nanosToIsoTimestamptzbehaviour used byTransforms.identity().toHumanString()(for example1510871468000001001 -> 2017-11-16T22:31:08.000001001, with the zone offset keptafter the fraction for the tz variant).
Are these changes tested?
Yes.
tests/utils/test_datetime.py: newtest_to_human_timestamp_nsandtest_to_human_timestamptz_nscover the epoch boundary and thesub-microsecond digit boundaries (0, 1, 999, 1000), a whole-second-plus-1ns
case (where the microsecond fraction is zero), and the Java reference value.
tests/test_transforms.py::test_identity_human_string: addstimestamp_nsand
timestamptz_nsrows alongside the existing type rows.tests/table/test_partitioning.py::test_partition_spec_to_path_timestamp: anend-to-end
PartitionSpec.partition_to_pathcheck for the microsecond andnanosecond timestamp/timestamptz types (the user-facing symptom).
The targeted subset passes (14 passed). Running the full
tests/test_transforms.py,tests/utils/test_datetime.py, andtests/table/test_partitioning.pyfilesshows only the pre-existing failures that require the optional
pyiceberg_coreextension; these new tests do not touch that path. Integration tests
(Docker + Spark) were not run in this environment; the behaviour is covered by
the unit tests end-to-end through
partition_to_path.Are there any user-facing changes?
Yes. Identity partition paths for
timestamp_nsandtimestamptz_nscolumns arenow ISO-8601 strings (matching the microsecond timestamp types and the Java
implementation) instead of raw integers.
IdentityTransform.to_human_stringforthese types changes correspondingly.