sadpandajoe commented on code in PR #43936:
URL: https://github.com/apache/superset/pull/43936#discussion_r4010007814
##########
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:
Fixed in 29cf89f4f0e8875a04be65d963ea01e65dddff53 -- added a pd.notna(value)
check to the reference filter, since hasattr(NaT, "strftime") is True and let
an all-null temporal axis reach round(nan / 7).
##########
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:
Fixed in 8cb5c913d88b5b0360f3e7e2d1d8828c616ec326 -- changed the guard from
weeks == 0 to abs(exact_days) < 7, since round() rounds to nearest rather than
toward zero (round(-4/7) == -1, not 0), which let 4-6 day offsets slip through
the guard and receive an incorrect full-week shift instead of the intended
per-row calendar fallback. Added a regression test covering a 5-day offset.
##########
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
Review Comment:
Fixed in 8cb5c913d88b5b0360f3e7e2d1d8828c616ec326 -- applied your suggested
abs(exact_days) < 7 condition and added a regression test covering a 5-day
offset (the existing 3-day case didn't reach the 4-6 day gap). Thanks for
catching this.
--
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]
