kaxil commented on code in PR #67164:
URL: https://github.com/apache/airflow/pull/67164#discussion_r3268757592
##########
task-sdk/src/airflow/sdk/definitions/partition_mappers/temporal.py:
##########
@@ -16,17 +16,27 @@
# under the License.
from __future__ import annotations
+from typing import TYPE_CHECKING
+
from airflow.sdk.definitions.partition_mappers.base import PartitionMapper
+if TYPE_CHECKING:
+ from pendulum import FixedTimezone, Timezone
+
class _BaseTemporalMapper(PartitionMapper):
+ """Base class for Temporal Partition Mappers."""
+
default_output_format: str
def __init__(
self,
+ *,
Review 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`.
##########
task-sdk/src/airflow/sdk/definitions/partition_mappers/temporal.py:
##########
@@ -16,17 +16,27 @@
# under the License.
from __future__ import annotations
+from typing import TYPE_CHECKING
+
from airflow.sdk.definitions.partition_mappers.base import PartitionMapper
+if TYPE_CHECKING:
+ from pendulum import FixedTimezone, Timezone
+
class _BaseTemporalMapper(PartitionMapper):
+ """Base class for Temporal Partition Mappers."""
+
default_output_format: str
def __init__(
self,
+ *,
+ timezone: str | Timezone | FixedTimezone = "UTC",
input_format: str = "%Y-%m-%dT%H:%M:%S",
output_format: str | None = None,
) -> None:
+ self._timezone = timezone
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
