EnxDev commented on PR #45784:
URL: https://github.com/apache/superset/pull/45784#issuecomment-6466280449
## EnxDev's Review Agent: apache/superset#45784 · HEAD 1d768b4
lgtm. An empty comparison now takes the same left-join path it takes with
full range off, and the new test goes red without the guard.
Checked locally on `1d768b4`:
- `tests/unit_tests/common/test_time_shifts.py` passes 70/70. With the
`helpers.py:3688` line reverted, `test_join_offset_dfs_empty_offset_df[True]`
fails with the same `TypeError: unsupported operand type(s) for -:
'numpy.ndarray' and 'DateOffset'` reported in #45783, so the test really guards
the fix.
- Throwaway probes (not in the PR) all pass: a dimension join key (`["A",
"D"]` with an all-NaN placeholder), an empty offset next to a populated one
with full range on, in both iteration orders (the populated offset still
extends the axis to its extra row), and an empty offset under a Week grain.
### 🔵 Nits
- `superset/models/helpers.py:3680`: the comment above `use_outer_join`
covers the existing conditions but not the new one. The NaN check is there
because an empty offset result is swapped for a one-row all-NaN placeholder at
`helpers.py:3171-3177`, so `offset_df.empty` would be False. One line pointing
at that would keep someone from "simplifying" it to an `.empty` check later.
- `tests/unit_tests/common/test_time_shifts.py:627`: could we add the mixed
case, one populated offset plus one empty offset with full range on? It's the
realistic dashboard setup ("1 day ago" and "1 year ago" on a young dataset) and
the one where deciding `how` per offset actually matters.
### 🙌 Praise
- `superset/models/helpers.py:3688`: gating the join type instead of
hardening `_coalesce_offset_index` keeps the placeholder row out of the outer
join entirely, and the parametrized test pins both modes to the same output.
<!-- enxdev-review-agent:1d768b4 -->
_Reviewed by EnxDev's Review Agent, @EnxDev · HEAD 1d768b4._
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
