feat: Update timezone data to 2025b #26981
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rdtr
requested review from
a team,
czentgr,
elharo and
unidevel
as code owners
Contributor
Reviewer's GuideUpdates Presto’s timezone data to tzdata 2025b by bumping Joda-Time and adding the America/Coyhaique zone, adjusts the checksum-based regression test for zone-index, and refreshes all GitHub Actions workflows to use Temurin JDK 17.0.16 due to an upstream JDK issue. Sequence diagram for timezone resolution with updated tzdata_2025bsequenceDiagram
actor User
participant PrestoClient
participant PrestoCoordinator
participant TimeZoneKey
participant JodaTimeTimeZoneData
User->>PrestoClient: Submit query (timestamp, America/Coyhaique)
PrestoClient->>PrestoCoordinator: Send query
PrestoCoordinator->>TimeZoneKey: lookupTimeZoneKey(America/Coyhaique)
TimeZoneKey->>JodaTimeTimeZoneData: resolveZoneId(America/Coyhaique)
JodaTimeTimeZoneData-->>TimeZoneKey: zoneId(2234, tzdata 2025b rules)
TimeZoneKey-->>PrestoCoordinator: TimeZoneKey(2234)
PrestoCoordinator->>PrestoCoordinator: Evaluate expressions with timezone
PrestoCoordinator-->>PrestoClient: Query results
PrestoClient-->>User: Display timestamps in America/Coyhaique
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
7 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Given how many workflows hardcode the Java patch version, consider centralizing the Java version (e.g., via a shared reusable workflow, workflow-level env, or matrix variable) so future patch bumps only need to be changed in one place.
- The
zone-index.propertiesand associated checksum assertion are guarded by a strong "should not (normally) be changed" comment; it might be helpful to update that comment (or add a short note nearby) to explicitly call out the Joda/tzdata upgrade procedure, including updatingzone-index.propertiesand the expected hash.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Given how many workflows hardcode the Java patch version, consider centralizing the Java version (e.g., via a shared reusable workflow, workflow-level env, or matrix variable) so future patch bumps only need to be changed in one place.
- The `zone-index.properties` and associated checksum assertion are guarded by a strong "should not (normally) be changed" comment; it might be helpful to update that comment (or add a short note nearby) to explicitly call out the Joda/tzdata upgrade procedure, including updating `zone-index.properties` and the expected hash.
## Individual Comments
### Comment 1
<location> `presto-common/src/test/java/com/facebook/presto/common/type/TestTimeZoneKey.java:218-219` </location>
<code_context>
hasher.putString(timeZoneKey.getId(), StandardCharsets.UTF_8);
}
// Zone file should not (normally) be changed, so let's make this more difficult
- assertEquals(hasher.hash().asLong(), 4825838578917475630L, "zone-index.properties file contents changed!");
+ assertEquals(hasher.hash().asLong(), 3765670086753811806L, "zone-index.properties file contents changed!");
</code_context>
<issue_to_address>
**suggestion (testing):** Consider making the checksum assertion failure more actionable for future tzdata updates
Since tzdata updates like this are expected periodically, this assertion may fail without clearly telling the updater what to do. Consider either:
- Expanding the assertion message to note that the expected checksum should be updated after tzdata/Joda-Time changes, or
- Adding a brief comment on how to recompute the checksum (algorithm or helper method), so future maintainers don’t have to rediscover the process.
```suggestion
// Zone file should not (normally) be changed, so let's make this more difficult.
// If this assertion fails after an intentional tzdata/Joda-Time update that changes
// zone-index.properties, recompute the expected checksum by running this test and
// updating the constant below with the new value from hasher.hash().asLong().
assertEquals(
hasher.hash().asLong(),
3765670086753811806L,
"zone-index.properties file contents changed. If this is due to an intentional tzdata/Joda-Time or zone-index.properties update, update the expected checksum to match hasher.hash().asLong().");
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
218
to
+219
| // Zone file should not (normally) be changed, so let's make this more difficult | ||
| assertEquals(hasher.hash().asLong(), 4825838578917475630L, "zone-index.properties file contents changed!"); | ||
| assertEquals(hasher.hash().asLong(), 3765670086753811806L, "zone-index.properties file contents changed!"); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider making the checksum assertion failure more actionable for future tzdata updates
Since tzdata updates like this are expected periodically, this assertion may fail without clearly telling the updater what to do. Consider either:
- Expanding the assertion message to note that the expected checksum should be updated after tzdata/Joda-Time changes, or
- Adding a brief comment on how to recompute the checksum (algorithm or helper method), so future maintainers don’t have to rediscover the process.
Suggested change
| // Zone file should not (normally) be changed, so let's make this more difficult | |
| assertEquals(hasher.hash().asLong(), 4825838578917475630L, "zone-index.properties file contents changed!"); | |
| assertEquals(hasher.hash().asLong(), 3765670086753811806L, "zone-index.properties file contents changed!"); | |
| // Zone file should not (normally) be changed, so let's make this more difficult. | |
| // If this assertion fails after an intentional tzdata/Joda-Time update that changes | |
| // zone-index.properties, recompute the expected checksum by running this test and | |
| // updating the constant below with the new value from hasher.hash().asLong(). | |
| assertEquals( | |
| hasher.hash().asLong(), | |
| 3765670086753811806L, | |
| "zone-index.properties file contents changed. If this is due to an intentional tzdata/Joda-Time or zone-index.properties update, update the expected checksum to match hasher.hash().asLong()."); |
rdtr
force-pushed
the
update-tzdata-2025b-v2
branch
from
7a0e228 to
19d5d9f
Compare
Contributor
|
@rdtr Seems some CI not running, can you rebase to try gain? |
rdtr
force-pushed
the
update-tzdata-2025b-v2
branch
from
19d5d9f to
3816cb8
Compare
Author
Contributor
|
@rdtr I guess this is a bug of github, since you upgraded JDK to 17.0.16, you probably can continue working on the original PR, and ignore the pending tests related to 17.0.15. Once reviewed and approved, you may ask admin like @tdcmeehan to merge this without full CI passed. |
Contributor
|
I'm actually a little worried this isn't transient, since it's happened for two PRs in a row. @rdtr can you try to quickly revert to 15 to see if the CI passes? If so, it would seem to be specific to the version upgrade. |
rdtr
force-pushed
the
update-tzdata-2025b-v2
branch
2 times, most recently
from
eff8774 to
54f7367
Compare
Author
|
@tdcmeehan |
Contributor
|
@rdtr could you try to restore the JDK changes, but try a different distribution, to see if it's localized to the distribution? |
rdtr
force-pushed
the
update-tzdata-2025b-v2
branch
2 times, most recently
from
7a44386 to
593c0d7
Compare
- Update Joda-Time from 2.13.1 to 2.14.0 (includes tzdata 2025b) - Add America/Coyhaique timezone (new zone for Chile's Aysén Region) - Update zone-index.properties checksum in test
rdtr
force-pushed
the
update-tzdata-2025b-v2
branch
3 times, most recently
from
8b0751b to
1e0810d
Compare
This reverts commit 54f7367.
rdtr
force-pushed
the
update-tzdata-2025b-v2
branch
from
1e0810d to
399cddf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a replacement for #26856. The CI was having persistent issues running tests on the original PR even after several rebases, so I am opening a fresh PR.
Description
This PR updates the timezone data to 2025b by upgrading Joda-Time from 2.13.1 to 2.14.0. The update adds the new
America/Coyhaiquetimezone (zone key 2234), which was introduced for Chile's Aysén Region. The zone-index.properties checksum in TestTimeZoneKey has been updated to reflect the new zone entry.Motivation and Context
Timezone databases are periodically updated to reflect changes in timezone rules around the world. The tzdata 2025b release includes the new America/Coyhaique zone for Chile's Aysén Region. Keeping Presto's timezone data current ensures accurate timestamp handling for users in affected regions.
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.