codeant-ai-for-open-source[bot] commented on code in PR #42054:
URL: https://github.com/apache/superset/pull/42054#discussion_r3598310413
##########
superset/models/helpers.py:
##########
@@ -2312,8 +2349,110 @@ def _determine_join_keys(
return self._process_date_range_offset(offset_df, join_keys)
else:
+ return self._align_offset_without_time_grain(
+ df, offset_df, offset, join_keys, x_axis_label
+ )
+
+ def _align_offset_without_time_grain(
+ self,
+ df: pd.DataFrame,
+ offset_df: pd.DataFrame,
+ offset: str,
+ join_keys: list[str],
+ x_axis_label: str | None = None,
+ ) -> tuple[pd.DataFrame, list[str]]:
+ """
+ Determine join keys for a relative offset when no time grain is set.
+
+ Without a time grain there is no truncated join column, but the two
+ series can still be aligned exactly: shifting the main series'
+ timestamps by the offset delta lands them on the offset series' raw
+ timestamps (normalize_time_delta returns a negative delta for "... ago"
+ offsets). Timestamps without an exact counterpart in the offset series
+ produce nulls, mirroring the grain-based join, and null timestamps in
+ the two series join to each other (both stringify to "NaT"). When
+ there is no temporal join key the original join keys are used as-is.
+
+ The shift is computed on wall-clock time, with any timezone dropped for
+ the duration of the alignment. The offset query's own time range was
+ shifted the same way -- ``get_past_or_future`` reads naive timestamps
+ -- so the rows it returns carry the source wall clock, and matching on
+ it is what aligns the two series. Re-localizing the result would only
+ reintroduce the DST edge cases that wall-clock arithmetic sidesteps:
+ shifting onto a skipped or repeated local hour raises out of pandas.
+ Both sides are normalized identically, so the two readings of a
+ repeated hour still align with each other.
+
+ Month, quarter, and year offsets shift via ``DateOffset``, which clamps
+ to a valid calendar day (e.g. Mar 29, 30, and 31 all shift back one
+ month to Feb 28), so on daily/irregular data several end-of-month rows
+ can align to the same offset timestamp. This mirrors the inherent
+ ambiguity of "the same day N months ago" without a time grain to
+ truncate against.
+ """
+ # Prefer the query's temporal x-axis when it is a join key; otherwise
+ # use the first datetime join key.
+ candidate_keys = sorted(join_keys, key=lambda key: key != x_axis_label)
+ temporal_join_key = next(
+ (
+ key
+ for key in candidate_keys
+ if key in df.columns
+ and key in offset_df.columns
+ and pd.api.types.is_datetime64_any_dtype(df[key])
Review Comment:
**Suggestion:** The temporal-key detection is too strict: it only accepts
`datetime64` dtypes from the main DataFrame. Superset can carry temporal
columns as `object` series of `datetime.date`/`datetime` values (already
treated as temporal elsewhere), and this guard will miss them, fall back to raw
join keys, and silently produce empty/misaligned comparison series for
grain-less offsets. Use the same temporal detection logic used elsewhere (e.g.
`dataframe_utils.is_datetime_series`) so object-backed temporal columns are
aligned instead of bypassed. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Grain-less time comparison misaligns temporal comparison series.
- ⚠️ Some charts show empty comparison results.
- ⚠️ Object-backed date axes not aligned for offsets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute a time-series chart query through `ExploreMixin.get_query_result`
(`superset/models/helpers.py:1739-1818`) where `query_object.time_offsets`
is non-empty,
`time_grain` is `None`, and the temporal axis column (e.g. `"ds"`) is
included in
`join_keys` with dtype `object` holding `datetime.date`/`datetime` values (a
pattern
explicitly treated as temporal by `dataframe_utils.is_datetime_series` in
`superset/common/utils/dataframe_utils.py:72-81`).
2. In `processing_time_offsets` (`superset/models/helpers.py:1839-2143`),
the base
DataFrame `df` is normalized via `normalize_df`, then `join_keys` are built
from
non-metric columns and `join_offset_dfs` is called with `time_grain=None`
and those
`join_keys` when `offset_dfs` has relative offsets populated.
3. Inside `join_offset_dfs` (`superset/models/helpers.py:2492-2570`), the
code dispatches
to `_determine_join_keys`, which in the `else` (grain-less, non-date-range)
branch calls
`_align_offset_without_time_grain` (`superset/models/helpers.py:2356-2475`)
with `df`,
`offset_df`, `offset`, `join_keys`, and `x_axis_label` set to the query’s
temporal axis
label.
4. `_align_offset_without_time_grain` computes `candidate_keys` and then
`temporal_join_key` using the guard `key in df.columns and key in
offset_df.columns and
pd.api.types.is_datetime64_any_dtype(df[key])`
(`superset/models/helpers.py:2398-2405`).
For an object-typed temporal column containing `datetime.date`/`datetime`
values,
`is_datetime64_any_dtype(df[key])` returns `False`, so `temporal_join_key`
is `None` and
the function returns `offset_df, join_keys` without adding a shifted
temporal join column.
The subsequent join in `_perform_join`
(`superset/models/helpers.py:177-205`) operates
only on the raw non-temporal join keys, causing the grain-less
time-comparison series to
be empty or misaligned instead of aligned on the temporal axis.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b47d8329ae0c4d7f8c843d17958f48fc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b47d8329ae0c4d7f8c843d17958f48fc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 2402:2402
**Comment:**
*Logic Error: The temporal-key detection is too strict: it only accepts
`datetime64` dtypes from the main DataFrame. Superset can carry temporal
columns as `object` series of `datetime.date`/`datetime` values (already
treated as temporal elsewhere), and this guard will miss them, fall back to raw
join keys, and silently produce empty/misaligned comparison series for
grain-less offsets. Use the same temporal detection logic used elsewhere (e.g.
`dataframe_utils.is_datetime_series`) so object-backed temporal columns are
aligned instead of bypassed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42054&comment_hash=30d4e86e41cb7f4ccf82dbe6805321650e847870d6689e3b14051151382722b4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42054&comment_hash=30d4e86e41cb7f4ccf82dbe6805321650e847870d6689e3b14051151382722b4&reaction=dislike'>👎</a>
--
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]
