Support timezone in SDK temporal partition mappers#67164
Conversation
ca0c304 to
2c4fbf3
Compare
2c4fbf3 to
408f7a0
Compare
|
|
||
| def __init__( | ||
| self, | ||
| *, |
There was a problem hiding this comment.
Adding *, here makes the SDK constructor keyword-only. That matches the Core class shape, which is the right destination, but it's a breaking signature change against the already-released task-sdk/1.2.1, where _BaseTemporalMapper.__init__ accepts input_format positionally — so StartOfDayMapper("%Y-%m-%dT%H:%M:%S") is valid public API today and would break on upgrade. Worth calling out in the PR description (and a newsfragment) since this is the public entry point Dag authors import via from airflow.sdk import StartOfDayMapper.
There was a problem hiding this comment.
I just added it newsfragment and description. Should we include it in 3.3.0 or 3.2.3? interface inconsistency can probably(?) be considered as a bug.
uranusjr
left a comment
There was a problem hiding this comment.
One nit on timezone input handling in a thread above
Add a keyword-only `timezone` kwarg to the SDK `_BaseTemporalMapper`, carry `_timezone` through `_Serializer` dispatch, and resolve string values via `parse_timezone()` at construction. Breaking change vs `task-sdk/1.2.1`; see `67164.significant.rst`.
408f7a0 to
8da1ce1
Compare
Why
The Core
_BaseTemporalMapperaccepts atimezonekwarg, but the SDK class Dag authors instantiate (from airflow.sdk import StartOfDayMapper) did not — and the_Serializerdispatch dropped_timezoneeven if it had. SoStartOfDayMapper(timezone="Asia/Taipei")silently fell back to UTC after serialization.What
timezonekwarg to the SDK_BaseTemporalMapper.__init__, matching the Core signature.timezonevalues viaparse_timezone()at construction so unknown names raisependulum.tz.exceptions.InvalidTimezoneeagerly, and_timezonematches Core's type contract (Timezone | FixedTimezone) instead of staying as astr._timezonethrough the_Serializerdispatch handler for all sixStartOf*Mapperclasses so the Core class receives it on deserialize.StartOfHourMapperimport inencoders.pyso thesingledispatchregistration matches the runtime type used by Dag authors.significantnewsfragment documenting the keyword-only constructor as a breaking change againsttask-sdk/1.2.1.Breaking changes
The SDK
_BaseTemporalMapperconstructor is now keyword-only. Callers that relied onStartOfDayMapper("%Y-%m-%dT%H:%M:%S")(valid intask-sdk1.2.1) must switch toStartOfDayMapper(input_format="%Y-%m-%dT%H:%M:%S"). Seeairflow-core/newsfragments/67164.significant.rst.Was generative AI tooling used to co-author this PR?
Generated-by: [Claude] following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.