Skip to content

API: Fix day partition transform result type #9345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tdcmeehan wants to merge 1 commit into from
Closed

API: Fix day partition transform result type #9345

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion format/spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ Partition field IDs must be reused if an existing partition spec contains an equ
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz`, `timestamp_ns`, `timestamptz_ns` | `int` |
| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz`, `timestamp_ns`, `timestamptz_ns` | `int` |
| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz`, `timestamp_ns`, `timestamptz_ns` | `int` |
| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz`, `timestamp_ns`, `timestamptz_ns` | `date` |
Copy link
Contributor

@Fokko Fokko Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A date is also an integer, as it represents the number of days since 1-1-1970. I think that int is correct here, similar to month, hour, and others.

Copy link
Author

@tdcmeehan tdcmeehan Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the underlying type of date is an int, shouldn't it should still be documented to be date, as it's expected that metadata tables produce YYYY-mm-dd formatted values for days (I gathered this from #279)? As it's written now, it's surprising to me to see non-integer values when I look at partition metadata for a column that has been transformed with day.

Copy link
Member

@RussellSpitzer RussellSpitzer Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Fokko, this is a description of the transform which takes a date or timestamp and produces an int. If you are implementing this transform and return a date the transform would be incorrect and incompatible with other Iceberg implementations. Now if you implement this function and display the output as a different type (like the metadata table) that's fine.

| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz`, `timestamp_ns`, `timestamptz_ns` | `int` |
| **`void`** | Always produces `null` | Any | Source type or `int` |

Expand Down