Vamsi-klu commented on PR #68150:
URL: https://github.com/apache/airflow/pull/68150#issuecomment-5140296309
@potiuk Addressed your two inline notes (and the manual DST validation ask).
Summary below.
## 1. Custom Window subclass compatibility (to_upstream_tz shim)
Rather than **kwargs / signature sniffing, kept the stable public contract
and added an extension point:
- Window.to_upstream(self, decoded) stays the abstract one-arg contract
(unchanged for custom subclasses).
- New non-abstract Window.to_upstream_tz(self, decoded, tz) defaults to
return self.to_upstream(decoded).
- DayWindow overrides to_upstream_tz for DST-aware local-hour enumeration.
- RollupMapper.to_upstream always calls self.window.to_upstream_tz(decoded,
self.upstream_mapper.tzinfo).
Existing custom windows that only implement to_upstream(self, decoded) keep
working. Calendar-sensitive windows opt in via to_upstream_tz.
## 2. Explicit tzinfo contract
- PartitionMapper.tzinfo property on the base class returns None.
- _BaseTemporalMapper continues to override with the configured timezone.
- Dropped getattr(self.upstream_mapper, "tzinfo", None); RollupMapper uses
self.upstream_mapper.tzinfo directly.
- Mirrored the property into the Task SDK base + temporal mapper so custom
SDK mappers can declare it the same way.
## 3. Core / SDK asymmetry (why core window.py has the runtime logic)
Confirmed by encode/decode path:
- encode_window maps both SDK and core Window classes onto
airflow.partition_mappers.window.* import paths (BUILTIN_WINDOWS).
- decode_window always imports the core class (is_core_window_import_path /
import_string).
- Scheduler-side rollup enumeration therefore always runs on core
DayWindow.to_upstream_tz after deserialization — never on an SDK Window
instance.
- Task SDK DayWindow is an authoring/serialization marker only (no
to_upstream implementation). Docstring documents to_upstream_tz as the
extension point; check-window-in-sync only compares the shared surface
(Direction, expected_decoded_type, init/serialize), not runtime methods.
So users never see a "24-key" SDK answer that the scheduler would disagree
with: the expected key set is computed only in the scheduler on the core Window.
## 4. Bonus: encode_upstream preserves aware offsets
make_aware forces fold=1, so the old encode_upstream path (aware → naive →
make_aware → strftime) collapsed both fall-back 01:00 instants to the same %z
key. encode_upstream now formats aware datetimes via
astimezone(...).strftime(...) so input_format with %z yields 25 distinct keys
on fall-back days.
## 5. Manual DST validation output
```
=== Manual DST validation (PR #68150) ===
NY spring 2024-03-10: count=23 (expect 23), T02 absent=True
NY fall 2024-11-03 no %z: count=24 (expect 24 distinct)
NY fall 2024-11-03 with %z: count=25 (expect 25 distinct)
T01 keys: ['2024-11-03T01-0400', '2024-11-03T01-0500']
NY normal 2024-06-10: count=24 (expect 24)
Sydney spring 2024-10-06: count=23 (expect 23), T02 absent=True
Sydney fall 2024-04-07 no %z: count=24 (expect 24)
Fixed +05:30 on 2024-03-10: count=24 (expect 24)
UTC explicit on 2024-03-10: count=24 (expect 24)
Custom 1-arg Window + RollupMapper: ['all', 'all-b']
tzinfo contract: FixedKeyMapper=None, temporal=America/New_York OK
All manual assertions passed.
```
Unit tests: pytest airflow-core/tests/unit/partition_mappers — 384 passed.
## Second maintainer
CODEOWNERS for partition_mappers is @dstandish @Lee-W — would either of you
(or whoever you prefer, @potiuk) take a look as second maintainer? Thanks!
--
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]
