Write human-readable partition paths for temporal transforms#1111
Write human-readable partition paths for temporal transforms#1111raghav-reglobe wants to merge 1 commit into
Conversation
|
"Diverges from the spec convention" |
|
And in general, can you please refrain from adding more comments than code? |
dba1204 to
537f34c
Compare
Partitioned writes for the year/month/day/hour transforms named the on-disk hive directory after the raw integer ordinal (e.g. `<field>=634/`): the partition routing column is the `date_diff(...)` ordinal and DuckDB core stringifies that value directly into the directory name. Apache Iceberg's reference writers use `Transform.toHumanString` for the path segment, producing `<field>=2022-11/` (month), `=2022/` (year), `=2022-11-15/` (day) and `=2022-11-15-13/` (hour). Match that convention without changing partition assignment: - Add an `iceberg_partition_to_human(transform, ordinal)` scalar that formats a temporal ordinal into the human-readable segment, reusing the existing `IcebergTransform::PartitionValueToString`. The temporal partition routing expression is wrapped with it, so the routing value (and therefore the directory) is human-readable. Rows still group by the same `date_diff` ordinal -- only the string representation differs. - Restore the raw ordinal for the manifest on the write-back path via a new inverse `IcebergTransform::PartitionStringToValue` (in `IcebergInsertGlobalState::AddFiles`), so the manifest partition value is byte-identical to before. Files written by older code (`=634/`) and by this change (`=2022-11/`) resolve to the same logical partition; readers match on the manifest value, not the path, and a later `rewrite_data_files` normalizes the physical layout. - Fix `PartitionValueToString`'s `hour` case to Iceberg's `yyyy-MM-dd-HH` (it emitted a full timestamp string) and 4-digit-pad `year`, so the read-side log path agrees with the written segment. bucket, truncate and identity are unchanged -- they already write the correct human value. Adds a regression test asserting the human-readable directory for month, year, day and hour transforms while the data round-trips and partition pruning still resolves. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Raghvendra Singh <raghav@cashify.in>
537f34c to
285e2b3
Compare
|
You're right, "spec convention" was imprecise — the table spec doesn't mandate a path format. Files are tracked by full path in the manifest and the partition tuple is stored in the manifest entry, so the directory string is never authoritative. What I meant is the convention of the Apache Iceberg reference implementation (Java): |
|
@Tishj reworded the description per your note — anything else you'd like changed before this is good to go? One branching question: this targets |
I don't understand why we would want to make this change. |
|
I think changing the partition function so it outputs the human readable string, then reinterpreting the string back to the ordinal for the manifest files introduces complexity we would rather not have. I would suggest instead to first add functionality to core that allows a user to determine the file naming pattern per partition value, then you can have duckdb-iceberg use that to write the human readable dates |
|
@Tmonster that makes sense — threading the format through a core hook is cleaner than reinterpreting the string back to the ordinal, and it lets the extension partition on the raw value directly. Before I put a PR together I want to make sure the shape matches what you had in mind. From a quick look, the only place core turns a partition value into the On the iceberg side that then lets us partition on the raw ordinal and drop the Does that match what you were picturing, and would core be open to a hook like that on the copy operator? Happy to adjust the API (or put it somewhere other than |
Problem
Partitioned writes for the
year/month/day/hourtransforms name theon-disk hive directory after the raw integer ordinal, e.g.
.../<field>=634/...for
month(ts). The partition routing column is thedate_diff(...)ordinal andDuckDB core stringifies that value directly into the directory name.
The Apache Iceberg reference implementation (Java) instead formats the path segment
with
Transform.toHumanString(PartitionSpec.partitionToPath):month=634=2022-11year=52=2022day=N=2022-11-15hour=N=2022-11-15-13This is not a correctness problem for Iceberg readers — files are tracked by full
path in the manifest and the partition tuple is stored in the manifest entry, so the
directory string is never authoritative. But it diverges from the Apache Iceberg
reference implementation's convention, the human-readable form is easier to
understand when browsing storage, and a table written by both DuckDB and Spark/Java
otherwise ends up with mixed
=634/and=2022-11/directories for the same logicalpartition.
Fix
The directory string is built by DuckDB core from the partition routing column's
value, so the extension's only lever is the value/type of that column. This PR
changes only the string representation of the temporal routing value and
leaves partition assignment untouched:
iceberg_partition_to_human(transform, ordinal)scalar formats a temporalordinal into the human-readable segment, reusing the existing
IcebergTransform::PartitionValueToString.GetPartitionExpressionwraps thedate_diffordinal with it, so the routing value (hence the directory) ishuman-readable. Rows still group by the same ordinal.
IcebergInsertGlobalState::AddFiles, so a new inverseIcebergTransform::PartitionStringToValueconverts the human string back to theraw ordinal there. The manifest partition value is byte-identical to before.
PartitionValueToString'shourcase is fixed to Iceberg'syyyy-MM-dd-HH(itpreviously emitted a full timestamp string), and
yearis 4-digit padded, sothe read-side output matches the written segment.
bucket,truncateandidentityare unchanged — they already write the correcthuman value.
Backward compatibility
Because the manifest partition value is unchanged, files written by older DuckDB
(
=634/) and by this change (=2022-11/) land in the same logical partition —Iceberg matches on the manifest value, not the directory. No re-partitioning, and a
subsequent
rewrite_data_filesnormalizes the physical layout over time. (If theordinal were ever lost, the INSERT would fail the manifest's BIGINT cast, so the
round-trip is verified by construction.)
Test
test/.../insert/partitions/temporal/test_human_readable_partition_paths.testasserts the human-readable directory for all four temporal transforms (and that the
old
=634/form is gone) while the data round-trips and partition pruning stillresolves.