Skip to content

feat: support timezone in SDK temporal partition mappers#67164

Open
Lee-W wants to merge 1 commit into
apache:mainfrom
astronomer:fix-sdk-temporal-mapper-timezone
Open

feat: support timezone in SDK temporal partition mappers#67164
Lee-W wants to merge 1 commit into
apache:mainfrom
astronomer:fix-sdk-temporal-mapper-timezone

Conversation

@Lee-W
Copy link
Copy Markdown
Member

@Lee-W Lee-W commented May 19, 2026

Why

The core _BaseTemporalMapper accepts a timezone kwarg, but the SDK class that Dag authors actually instantiate (from airflow.sdk import StartOfDayMapper) did not — and the encoder dispatch dropped _timezone even if it had.

What

  • Add timezone kwarg to SDK _BaseTemporalMapper.__init__, matching the core signature (stored verbatim; core's deserialize handles parsing).
  • Carry _timezone through the _Serializer dispatch handler for all six StartOf*Mapper classes.
  • Unify the StartOfHourMapper import in encoders.py so singledispatch registration matches the runtime type.

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: [Claude] following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {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.

@Lee-W Lee-W moved this to In Progress in AIP-76 Asset Partitioning May 19, 2026
@Lee-W Lee-W force-pushed the fix-sdk-temporal-mapper-timezone branch from ca0c304 to 2c4fbf3 Compare May 19, 2026 13:29
@Lee-W Lee-W changed the title feat(partition-mapper): support timezone in SDK temporal mappers feat: support timezone in SDK temporal partition mappers May 19, 2026
@Lee-W Lee-W marked this pull request as ready for review May 19, 2026 13:32
@Lee-W Lee-W moved this from In Progress to In Review in AIP-76 Asset Partitioning May 19, 2026
The core temporal mappers accept a `timezone` kwarg but the SDK
`_BaseTemporalMapper` did not, so Dag authors could not reach the
feature via `from airflow.sdk import StartOfDayMapper`. Complete the
SDK signature by adding the kwarg to the constructor and carrying
`_timezone` through the `_Serializer` dispatch handler so the core
class receives it on deserialize.
@Lee-W Lee-W force-pushed the fix-sdk-temporal-mapper-timezone branch from 2c4fbf3 to 408f7a0 Compare May 19, 2026 13:39

def __init__(
self,
*,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

input_format: str = "%Y-%m-%dT%H:%M:%S",
output_format: str | None = None,
) -> None:
self._timezone = timezone
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The Core _BaseTemporalMapper runs timezone through parse_timezone(...) before assigning, but the SDK side just stores it verbatim. So sdk_instance._timezone can be str | Timezone | FixedTimezone, while core_instance._timezone is always a Timezone | FixedTimezone. The encode path tolerates both (encode_timezone handles all three types), but it leaves a subtle type asymmetry on a _timezone attribute that lives on both sibling classes under the same name. Either mirror parse_timezone here, or add a brief comment noting the SDK keeps the raw value and normalization happens in Core.deserialize, so future readers don't trip on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

2 participants