Quantum0uasar commented on PR #59691:
URL: https://github.com/apache/airflow/pull/59691#issuecomment-4557207016
Thanks for tackling this edge case — placeholder-to-expanded index
transition bugs are notoriously hard to reproduce in dynamic task mapping. I
reviewed the logic and have a few observations:
## On the core helper
The guard condition:
```python
return (
-1 in task_to_map_indexes[task.task_id] and
-1 not in task_to_map_indexes[relative.task_id] and
0 in task_to_map_indexes[relative.task_id]
)
```
This reads cleanly, but I’d flag one subtle case: what if the upstream
expands to more than one instance (e.g., `map_index` in `{0, 1, 2, ...}`)? The
function returns a boolean for translating `-1 → 0`, but for a fan-out case
where downstream `map_index=-1` should resolve to **all** expanded instances,
not just `0`, is that handled elsewhere? Worth a comment in the code or a
targeted test for fan-out upstreams.
## On DB query cost
The reviewer note about unnecessary DB queries is important. If
`task_to_map_indexes` is built by querying per-task rather than via a single
bulk query, this will fire N queries for N upstream tasks in complex DAGs.
Consider pre-fetching once:
```python
all_indexes = session.execute(
select(TI.task_id, TI.map_index)
.where(TI.dag_id == dag_id, TI.run_id == run_id)
).all()
task_to_map_indexes = defaultdict(set)
for task_id, map_index in all_indexes:
task_to_map_indexes[task_id].add(map_index)
```
This avoids the query amplification problem especially for wide DAGs.
## Test coverage gap
The existing regression tests cover:
- Placeholder already expanded (upstream `map_index=-1` replaced by `0`)
- Downstream in both unexpanded and expanded states
One gap: a test where the upstream has **not yet** been expanded
(placeholder still active). The fix should be a no-op in that case, but it’s
easy to accidentally break this when changing the condition.
Also agree with the reviewer’s request for an `ALL_SUCCESS` trigger rule
test — trigger rule handling is another layer where index resolution can
silently produce wrong results.
## Minor
- The helper name `resolve_placeholder_map_index` is descriptive; consider
adding a one-line docstring explaining the `-1 → 0` contract so future
maintainers don’t have to re-derive it.
- Is there a linked issue number for this fix? Would help close the feedback
loop (e.g., `Closes #59289` in the PR description if that’s the upstream issue).
Overall the fix is targeted and correct for the primary case. The main risk
area is the fan-out upstream scenario and DB query cost at scale. Happy to look
at the full diff in `taskinstance.py` if you want a more detailed line-level
review.
--
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]
