potiuk commented on code in PR #68150:
URL: https://github.com/apache/airflow/pull/68150#discussion_r3681597701
##########
airflow-core/src/airflow/partition_mappers/base.py:
##########
@@ -173,9 +173,13 @@ def to_downstream(self, key: str) -> str | Iterable[str]:
def to_upstream(self, downstream_key: str) -> frozenset[str]:
"""Return the complete set of upstream partition keys required for
*downstream_key*."""
decoded = self.upstream_mapper.decode_downstream(downstream_key)
+ # Hand the upstream mapper's timezone to the window so calendar
windows (DayWindow) can
+ # enumerate the real local hours across DST. Non-temporal mappers have
no tzinfo -> None,
+ # in which case the window falls back to its fixed-count (UTC)
behaviour.
+ tz = getattr(self.upstream_mapper, "tzinfo", None)
return frozenset(
self.upstream_mapper.encode_upstream(expected_upstream)
- for expected_upstream in self.window.to_upstream(decoded)
+ for expected_upstream in self.window.to_upstream(decoded, tz)
Review Comment:
`Window` is exported from `airflow.sdk` (it's in `__all__`), so Dag authors
can define their own subclasses. A user-defined `to_upstream(self,
decoded_downstream)` will raise `TypeError` here, because the call passes `tz`
positionally — the default on the abstract signature doesn't protect an
override that doesn't declare the parameter.
All in-tree windows are updated, so this only affects out-of-tree
subclasses, which are probably rare for a feature this new. But it's a
public-API break and the newsfragment doesn't mention it.
Cheapest fix is to pass it as a keyword and let windows that don't care
absorb it via `**kwargs`; alternatively, keep the positional call and add a
line to the newsfragment telling custom-window authors to add the parameter.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/partition_mappers/base.py:
##########
@@ -173,9 +173,13 @@ def to_downstream(self, key: str) -> str | Iterable[str]:
def to_upstream(self, downstream_key: str) -> frozenset[str]:
"""Return the complete set of upstream partition keys required for
*downstream_key*."""
decoded = self.upstream_mapper.decode_downstream(downstream_key)
+ # Hand the upstream mapper's timezone to the window so calendar
windows (DayWindow) can
+ # enumerate the real local hours across DST. Non-temporal mappers have
no tzinfo -> None,
+ # in which case the window falls back to its fixed-count (UTC)
behaviour.
+ tz = getattr(self.upstream_mapper, "tzinfo", None)
Review Comment:
Duck-typing via `getattr` works, but it means a mapper that grows a `tzinfo`
attribute for unrelated reasons silently starts influencing window enumeration,
and a temporal mapper that *should* expose one but doesn't degrades silently to
fixed-count behaviour rather than failing.
Since you've already added the `tzinfo` property to the temporal mapper,
declaring it on the mapper base (returning `None` by default) would make the
contract explicit and let mypy check it. Minor — the comment above already
explains the intent well.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
