bito-code-review[bot] commented on code in PR #43936:
URL: https://github.com/apache/superset/pull/43936#discussion_r4010008878
##########
superset/models/helpers.py:
##########
@@ -3447,30 +3479,98 @@ def _coalesce_offset_index(
right-hand column, expressed in the offset's own time range (e.g.
"yesterday
15:00"). Shifting it forward by the offset places it on the main
series'
axis (e.g. "today 15:00") so the comparison line spans the full period.
+
+ Under a Week grain, ``resolved_week_offset`` is the same whole-week
shift
+ used to build the join column (see ``_resolve_week_grain_offset``);
reusing
+ it here instead of the raw calendar offset keeps this reconstructed
axis
+ value aligned to the same weekday the join matched on.
"""
x_axis = join_keys[0]
offset_x_axis = f"{x_axis}{R_SUFFIX}"
if x_axis not in df.columns or offset_x_axis not in df.columns:
return df
- # normalize_time_delta returns a negative delta for "... ago" offsets,
so
- # subtracting it shifts the historical timestamp forward onto the main
axis.
- try:
- forward_shift = DateOffset(**normalize_time_delta(offset))
- except (ValueError, TimeDeltaAmbiguousError):
- return df
+ if resolved_week_offset is not None:
+ forward_shift = resolved_week_offset
+ else:
+ # normalize_time_delta returns a negative delta for "... ago"
+ # offsets, so subtracting it shifts the historical timestamp
+ # forward onto the main axis.
+ try:
+ forward_shift = DateOffset(**normalize_time_delta(offset))
+ except (ValueError, TimeDeltaAmbiguousError):
+ return df
shifted = df[offset_x_axis] - forward_shift
df[x_axis] = df[x_axis].fillna(shifted)
return df
+ @staticmethod
+ def _resolve_week_grain_offset(
+ df: pd.DataFrame,
+ time_grain: str | None,
+ time_offset: str | None,
+ ) -> DateOffset | None:
+ """
+ Resolve a relative time offset applied under a Week grain to a single
+ whole-week ``DateOffset`` shared by every row of ``df``.
+
+ A calendar month/quarter/year is not a whole number of weeks, so
+ applying the raw calendar shift independently to each row rounds to a
+ different number of weeks depending on how many leap days or
+ month-length differences happen to fall inside that particular row's
+ span. Two main-series rows exactly one grain apart can then round to
+ *different* whole-week counts, colliding onto the same shifted date
+ (or skipping one). Resolving the shift once, from a single reference
+ date, and reusing that constant for every row keeps rows exactly as
+ many whole weeks apart as they started -- matching the offset
+ series' own real week-start dates, which are always aligned to the
+ grain's weekday.
+
+ Returns ``None`` when the offset does not apply (no offset, a date
+ range, or a non-Week grain), in which case callers fall back to the
+ original per-call calendar-offset behavior.
+ """
+ if (
+ not time_grain
+ or time_grain not in WEEK_GRAINS
+ or not time_offset
+ or ExploreMixin.is_valid_date_range_static(time_offset)
+ or df.empty
+ ):
+ return None
+
+ reference_column = df.iloc[:, 0]
+ reference_values = reference_column[
+ reference_column.apply(lambda value: hasattr(value, "strftime"))
+ ]
+ if reference_values.empty:
+ return None
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate and addresses the identified issue. By adding
`pd.notna(value)` to the predicate, the code correctly filters out `NaT` values
that would otherwise pass the `hasattr(value, 'strftime')` check, preventing
the subsequent `ValueError` when performing calculations on an all-null
temporal axis.
**superset/models/helpers.py**
```
reference_column = df.iloc[:, 0]
reference_values = reference_column[
reference_column.apply(lambda value: hasattr(value, "strftime")
and pd.notna(value))
]
if reference_values.empty:
return None
```
##########
superset/models/helpers.py:
##########
@@ -3447,30 +3479,110 @@ def _coalesce_offset_index(
right-hand column, expressed in the offset's own time range (e.g.
"yesterday
15:00"). Shifting it forward by the offset places it on the main
series'
axis (e.g. "today 15:00") so the comparison line spans the full period.
+
+ Under a Week grain, ``resolved_week_offset`` is the same whole-week
shift
+ used to build the join column (see ``_resolve_week_grain_offset``);
reusing
+ it here instead of the raw calendar offset keeps this reconstructed
axis
+ value aligned to the same weekday the join matched on.
"""
x_axis = join_keys[0]
offset_x_axis = f"{x_axis}{R_SUFFIX}"
if x_axis not in df.columns or offset_x_axis not in df.columns:
return df
- # normalize_time_delta returns a negative delta for "... ago" offsets,
so
- # subtracting it shifts the historical timestamp forward onto the main
axis.
- try:
- forward_shift = DateOffset(**normalize_time_delta(offset))
- except (ValueError, TimeDeltaAmbiguousError):
- return df
+ if resolved_week_offset is not None:
+ forward_shift = resolved_week_offset
+ else:
+ # normalize_time_delta returns a negative delta for "... ago"
+ # offsets, so subtracting it shifts the historical timestamp
+ # forward onto the main axis.
+ try:
+ forward_shift = DateOffset(**normalize_time_delta(offset))
+ except (ValueError, TimeDeltaAmbiguousError):
+ return df
shifted = df[offset_x_axis] - forward_shift
df[x_axis] = df[x_axis].fillna(shifted)
return df
+ @staticmethod
+ def _resolve_week_grain_offset(
+ df: pd.DataFrame,
+ time_grain: str | None,
+ time_offset: str | None,
+ ) -> DateOffset | None:
+ """
+ Resolve a relative time offset applied under a Week grain to a single
+ whole-week ``DateOffset`` shared by every row of ``df``.
+
+ A calendar month/quarter/year is not a whole number of weeks, so
+ applying the raw calendar shift independently to each row rounds to a
+ different number of weeks depending on how many leap days or
+ month-length differences happen to fall inside that particular row's
+ span. Two main-series rows exactly one grain apart can then round to
+ *different* whole-week counts, colliding onto the same shifted date
+ (or skipping one). Resolving the shift once, from a single reference
+ date, and reusing that constant for every row keeps rows exactly as
+ many whole weeks apart as they started -- matching the offset
+ series' own real week-start dates, which are always aligned to the
+ grain's weekday.
+
+ Returns ``None`` when the offset does not apply (no offset, a date
+ range, or a non-Week grain), in which case callers fall back to the
+ original per-call calendar-offset behavior.
+ """
+ if (
+ not time_grain
+ or time_grain not in WEEK_GRAINS
+ or not time_offset
+ or ExploreMixin.is_valid_date_range_static(time_offset)
+ or df.empty
+ ):
+ return None
+
+ reference_column = df.iloc[:, 0]
+ reference_values = reference_column[
+ reference_column.apply(
+ lambda value: hasattr(value, "strftime") and pd.notna(value)
+ )
+ ]
+ if reference_values.empty:
+ return None
+
+ # The reference must be picked by value, not row position: two rows
+ # exactly one grain apart can shift by calendar spans that differ by
+ # up to a whole week (depending on how many leap days fall inside
+ # each row's own span), so whichever row happened to land first
+ # would make the resolved constant depend on DataFrame row order.
+ # The minimum is deterministic for a given set of dates regardless
+ # of ordering.
+ reference = reference_values.min()
+ calendar_offset = DateOffset(**normalize_time_delta(time_offset))
+ calendar_shifted = reference + calendar_offset
+ # Timedelta.days floors toward negative infinity, which would round
+ # e.g. an 83-hour ("< half a week") shift down to a full week instead
+ # of zero; dividing by a one-day Timedelta keeps the exact fraction.
+ exact_days = (calendar_shifted - reference) / Timedelta(days=1)
+ weeks = round(exact_days / 7)
+ if weeks == 0:
+ # A sub-week offset (e.g. "3 days ago") is not the weekday-drift
+ # case this resolution exists to fix -- it does not touch a
+ # calendar unit wider than a week, so per-row rounding cannot
+ # disagree between rows. Returning a zero-day DateOffset here
+ # would override the raw per-row calendar shift with a no-op,
+ # leaving every row on its own current week instead of shifting
+ # it back. Returning None restores that raw per-row behavior.
+ return None
+ return DateOffset(days=weeks * 7)
Review Comment:
<!-- Bito Reply -->
The suggestion to change the guard from `weeks == 0` to `abs(exact_days) <
7` is correct and appropriate. The original `weeks == 0` check only catches
offsets of 1-3 days because `round()` rounds to the nearest integer, causing
4-6 day offsets to round to -1 week and bypass the guard. Using
`abs(exact_days) < 7` correctly identifies all sub-week offsets, ensuring they
fall back to the original per-row calendar behavior as intended.
**superset/models/helpers.py**
```
if abs(exact_days) < 7:
# A sub-week offset (e.g. "3 days ago", "5 days ago") is not the
# weekday-drift case this resolution exists to fix -- it does not
# touch a calendar unit wider than a week, so per-row rounding
# cannot disagree between rows. Returning a zero-day DateOffset
# here would override the raw per-row calendar shift with a
# no-op, leaving every row on its own current week instead of
# shifting it back. Returning None restores that raw per-row
# behavior. Checking the day count directly (rather than
# `weeks == 0`) also catches 4-6 day offsets, which round to a
# full week and would otherwise escape this guard.
return None
```
--
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]
