You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2025/07/28 21:08:17 UTC
[PR] feat(timeshift): Add support for date range timeshifts in table charts [superset]
msyavuz opened a new pull request, #34375:
URL: https://github.com/apache/superset/pull/34375
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
## Summary
This PR adds support for date range timeshifts in addition to the existing relative time offsets (like "1 year ago"). Users can now specify explicit date ranges in the timeshift field (e.g., "2015-01-03 : 2015-01-04") to compare data from specific time periods.
## Problem
Previously, Superset only supported relative time offsets for time comparisons (e.g., "1 year ago", "30 days ago"). Users had no way to compare data from specific, non-relative date ranges, which limited the flexibility of time-based analysis.
## Solution
- **Extended timeshift parsing** to detect and handle date range formats (`YYYY-MM-DD : YYYY-MM-DD`)
- **Enhanced query generation** to properly handle explicit date ranges without conflicting temporal filters
- **Improved join logic** to correctly combine data from different time periods when using date range offsets
- **Dynamic temporal column detection** to avoid hardcoded column names and work with various dataset configurations
## Key Changes
### 1. Enhanced Time Offset Processing (`processing_time_offsets`)
- Added `is_valid_date_range()` method to detect date range format timeshifts
- Separate handling logic for date range vs relative offsets
- Fixed temporal filter conflicts that were causing empty results
- Proper management of `inner_from_dttm`/`inner_to_dttm` bounds for different offset types
### 2. Improved Join Logic (`join_offset_dfs`)
- Dynamic temporal column detection using query granularity and datetime series detection
- Special aggregation logic for date range offsets to prevent duplicate rows
- Preserved original behavior for relative time offsets
- Cross-join with aggregation when no non-temporal dimensions exist
### 3. Updated Time Delta Normalization (`normalize_time_delta`)
- Enhanced to handle date range formats gracefully
- Maintains backward compatibility with existing relative time expressions
## Usage Examples
**Relative Offsets (existing functionality):**
```
Time Range: 2024-01-01 to 2024-01-31
Timeshift: "1 year ago"
→ Compares Jan 2024 data with Jan 2023 data
```
**Date Range Offsets (new functionality):**
```
Time Range: 2024-01-01 to 2024-01-31
Timeshift: "2023-06-15 : 2023-07-15"
→ Compares Jan 2024 data with mid-June to mid-July 2023 data
```
## Testing
- ✅ Existing relative timeshift functionality preserved
- ✅ New date range timeshift functionality working
- ✅ Proper SQL query generation with correct temporal filters
- ✅ Data aggregation and joining working correctly
- ✅ No conflicts between different offset types
## Backward Compatibility
This change is fully backward compatible. All existing timeshift functionality continues to work exactly as before. The new date range format is additive and doesn't affect existing usage patterns.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(timeshift): Add support for date range timeshifts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz merged PR #34375:
URL: https://github.com/apache/superset/pull/34375
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(timeshift): Add support for date range timeshifts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #34375:
URL: https://github.com/apache/superset/pull/34375#issuecomment-3146305381
This is a multi part implementation, frontend might be contributed later
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(timeshift): Add support for date range timeshifts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #34375:
URL: https://github.com/apache/superset/pull/34375#discussion_r2247901020
##########
superset/common/query_context_processor.py:
##########
@@ -522,34 +545,92 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
offset, outer_to_dttm
)
+ # For relative offsets, keep the original behavior
Review Comment:
I am not really altering the existing behaviour just moving it into the else block for feature flag.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(timeshift): Add support for date range timeshifts [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #34375:
URL: https://github.com/apache/superset/pull/34375#issuecomment-3145361657
@msyavuz Could you add some screenshots demonstrating the new filters?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(timeshift): Add support for date range timeshifts [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #34375:
URL: https://github.com/apache/superset/pull/34375#discussion_r2247640819
##########
superset/common/query_context_processor.py:
##########
@@ -476,38 +481,56 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
)
time_grain = self.get_time_grain(query_object)
-
metric_names = get_metric_names(query_object.metrics)
-
# use columns that are not metrics as join keys
join_keys = [col for col in df.columns if col not in metric_names]
for offset in query_object.time_offsets:
try:
- # pylint: disable=line-too-long
- # Since the x-axis is also a column name for the time filter, x_axis_label will be set as granularity # noqa: E501
- # these query object are equivalent:
- # 1) { granularity: 'dttm_col', time_range: '2020 : 2021', time_offsets: ['1 year ago']} # noqa: E501
- # 2) { columns: [
- # {label: 'dttm_col', sqlExpression: 'dttm_col', "columnType": "BASE_AXIS" } # noqa: E501
- # ],
- # time_offsets: ['1 year ago'],
- # filters: [{col: 'dttm_col', op: 'TEMPORAL_RANGE', val: '2020 : 2021'}], # noqa: E501
- # }
original_offset = offset
- if self.is_valid_date_range(offset):
+ is_date_range_offset = self.is_valid_date_range(offset)
+
+ logger.info(
+ f"Processing offset: {offset}, "
+ f"is_date_range: {is_date_range_offset}"
+ )
+
+ if is_date_range_offset and feature_flag_manager.is_feature_enabled(
+ "DATE_RANGE_TIMESHIFTS_ENABLED"
+ ):
+ # DATE RANGE OFFSET LOGIC (like "2015-01-03 : 2015-01-04")
try:
# Parse the specified range
offset_from_dttm, offset_to_dttm = (
get_since_until_from_time_range(time_range=offset)
)
+ logger.info(
Review Comment:
Same here
##########
superset/common/query_context_processor.py:
##########
@@ -476,38 +481,56 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
)
time_grain = self.get_time_grain(query_object)
-
metric_names = get_metric_names(query_object.metrics)
-
# use columns that are not metrics as join keys
join_keys = [col for col in df.columns if col not in metric_names]
for offset in query_object.time_offsets:
try:
- # pylint: disable=line-too-long
- # Since the x-axis is also a column name for the time filter, x_axis_label will be set as granularity # noqa: E501
- # these query object are equivalent:
- # 1) { granularity: 'dttm_col', time_range: '2020 : 2021', time_offsets: ['1 year ago']} # noqa: E501
- # 2) { columns: [
- # {label: 'dttm_col', sqlExpression: 'dttm_col', "columnType": "BASE_AXIS" } # noqa: E501
- # ],
- # time_offsets: ['1 year ago'],
- # filters: [{col: 'dttm_col', op: 'TEMPORAL_RANGE', val: '2020 : 2021'}], # noqa: E501
- # }
original_offset = offset
- if self.is_valid_date_range(offset):
+ is_date_range_offset = self.is_valid_date_range(offset)
+
+ logger.info(
Review Comment:
This might be a debug log instead
##########
superset/common/query_context_processor.py:
##########
@@ -522,34 +545,92 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
offset, outer_to_dttm
)
+ # For relative offsets, keep the original behavior
+ # BUT: Don't set inner bounds that conflict with the main query
+ # range. The inner bounds should match the shifted range, not the
+ # original range
+ query_object_clone.inner_from_dttm = query_object_clone.from_dttm
+ query_object_clone.inner_to_dttm = query_object_clone.to_dttm
+
x_axis_label = get_x_axis_label(query_object.columns)
query_object_clone.granularity = (
query_object_clone.granularity or x_axis_label
)
+
+ logger.info(
Review Comment:
Same here for debug
##########
superset/common/query_context_processor.py:
##########
@@ -522,34 +545,92 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
offset, outer_to_dttm
)
+ # For relative offsets, keep the original behavior
+ # BUT: Don't set inner bounds that conflict with the main query
+ # range. The inner bounds should match the shifted range, not the
+ # original range
+ query_object_clone.inner_from_dttm = query_object_clone.from_dttm
+ query_object_clone.inner_to_dttm = query_object_clone.to_dttm
+
x_axis_label = get_x_axis_label(query_object.columns)
query_object_clone.granularity = (
query_object_clone.granularity or x_axis_label
)
+
+ logger.info(
+ f"After setting dates - from_dttm: {query_object_clone.from_dttm}, "
+ f"to_dttm: {query_object_clone.to_dttm}"
+ )
+ logger.info(
+ f"Inner bounds - from: {query_object_clone.inner_from_dttm}, "
+ f"to: {query_object_clone.inner_to_dttm}"
+ )
+
except ValueError as ex:
raise QueryObjectValidationError(str(ex)) from ex
- # make sure subquery use main query where clause
- query_object_clone.inner_from_dttm = outer_from_dttm
- query_object_clone.inner_to_dttm = outer_to_dttm
+
query_object_clone.time_offsets = []
query_object_clone.post_processing = []
+
# Get time offset index
index = (get_base_axis_labels(query_object.columns) or [DTTM_ALIAS])[0]
- # The comparison is not using a temporal column so we need to modify
- # the temporal filter so we run the query with the correct time range
- if not dataframe_utils.is_datetime_series(df.get(index)):
- # Lets find the first temporal filter in the filters array and change
- # its val to be the result of get_since_until with the offset
- for flt in query_object_clone.filter:
- if flt.get("op") == FilterOperator.TEMPORAL_RANGE and isinstance(
- flt.get("val"), str
- ):
- time_range = cast(str, flt.get("val"))
- if self.is_valid_date_range(offset):
- flt["val"] = (
- f"{query_object_clone.from_dttm} : {query_object_clone.to_dttm}" # noqa: E501
+
+ # Handle temporal filters
+ if is_date_range_offset and feature_flag_manager.is_feature_enabled(
+ "DATE_RANGE_TIMESHIFTS_ENABLED"
+ ):
+ # DATE RANGE OFFSETS: Clean slate approach
+ logger.info("Handling temporal filters for date range offset")
+
+ # Create a completely new filter list to avoid conflicts
+ query_object_clone.filter = copy.deepcopy(query_object_clone.filter)
+
+ # Remove any existing temporal filters that might conflict
+ query_object_clone.filter = [
+ flt
+ for flt in query_object_clone.filter
+ if not (flt.get("op") == FilterOperator.TEMPORAL_RANGE)
+ ]
+
+ # Add our specific temporal filter
+ temporal_col = query_object_clone.granularity or x_axis_label
+ if temporal_col:
+ new_temporal_filter: QueryObjectFilterClause = {
+ "col": temporal_col,
+ "op": FilterOperator.TEMPORAL_RANGE,
+ "val": (
+ f"{query_object_clone.from_dttm} : "
+ f"{query_object_clone.to_dttm}"
+ ),
+ }
+ query_object_clone.filter.append(new_temporal_filter)
+ logger.info(
+ f"Added temporal filter for date range offset: "
+ f"{new_temporal_filter}"
+ )
+
+ else:
+ # RELATIVE OFFSETS: Original logic but fixed
+ logger.info("Handling temporal filters for relative offset")
Review Comment:
Same here for debug
##########
superset/common/query_context_processor.py:
##########
@@ -522,34 +545,92 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
offset, outer_to_dttm
)
+ # For relative offsets, keep the original behavior
+ # BUT: Don't set inner bounds that conflict with the main query
+ # range. The inner bounds should match the shifted range, not the
+ # original range
+ query_object_clone.inner_from_dttm = query_object_clone.from_dttm
+ query_object_clone.inner_to_dttm = query_object_clone.to_dttm
+
x_axis_label = get_x_axis_label(query_object.columns)
query_object_clone.granularity = (
query_object_clone.granularity or x_axis_label
)
+
+ logger.info(
+ f"After setting dates - from_dttm: {query_object_clone.from_dttm}, "
+ f"to_dttm: {query_object_clone.to_dttm}"
+ )
+ logger.info(
+ f"Inner bounds - from: {query_object_clone.inner_from_dttm}, "
+ f"to: {query_object_clone.inner_to_dttm}"
+ )
+
except ValueError as ex:
raise QueryObjectValidationError(str(ex)) from ex
- # make sure subquery use main query where clause
- query_object_clone.inner_from_dttm = outer_from_dttm
- query_object_clone.inner_to_dttm = outer_to_dttm
+
query_object_clone.time_offsets = []
query_object_clone.post_processing = []
+
# Get time offset index
index = (get_base_axis_labels(query_object.columns) or [DTTM_ALIAS])[0]
- # The comparison is not using a temporal column so we need to modify
- # the temporal filter so we run the query with the correct time range
- if not dataframe_utils.is_datetime_series(df.get(index)):
- # Lets find the first temporal filter in the filters array and change
- # its val to be the result of get_since_until with the offset
- for flt in query_object_clone.filter:
- if flt.get("op") == FilterOperator.TEMPORAL_RANGE and isinstance(
- flt.get("val"), str
- ):
- time_range = cast(str, flt.get("val"))
- if self.is_valid_date_range(offset):
- flt["val"] = (
- f"{query_object_clone.from_dttm} : {query_object_clone.to_dttm}" # noqa: E501
+
+ # Handle temporal filters
+ if is_date_range_offset and feature_flag_manager.is_feature_enabled(
+ "DATE_RANGE_TIMESHIFTS_ENABLED"
+ ):
+ # DATE RANGE OFFSETS: Clean slate approach
+ logger.info("Handling temporal filters for date range offset")
+
+ # Create a completely new filter list to avoid conflicts
+ query_object_clone.filter = copy.deepcopy(query_object_clone.filter)
+
+ # Remove any existing temporal filters that might conflict
+ query_object_clone.filter = [
+ flt
+ for flt in query_object_clone.filter
+ if not (flt.get("op") == FilterOperator.TEMPORAL_RANGE)
+ ]
+
+ # Add our specific temporal filter
+ temporal_col = query_object_clone.granularity or x_axis_label
+ if temporal_col:
+ new_temporal_filter: QueryObjectFilterClause = {
+ "col": temporal_col,
+ "op": FilterOperator.TEMPORAL_RANGE,
+ "val": (
+ f"{query_object_clone.from_dttm} : "
+ f"{query_object_clone.to_dttm}"
+ ),
+ }
+ query_object_clone.filter.append(new_temporal_filter)
+ logger.info(
+ f"Added temporal filter for date range offset: "
+ f"{new_temporal_filter}"
+ )
+
+ else:
+ # RELATIVE OFFSETS: Original logic but fixed
Review Comment:
Wondering if the fix should live on a different PR (and probably be shipped before this one?) otherwise your PR is not "just adding feature behind a FF"
##########
superset/common/query_context_processor.py:
##########
@@ -522,34 +545,92 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
offset, outer_to_dttm
)
+ # For relative offsets, keep the original behavior
+ # BUT: Don't set inner bounds that conflict with the main query
+ # range. The inner bounds should match the shifted range, not the
+ # original range
+ query_object_clone.inner_from_dttm = query_object_clone.from_dttm
+ query_object_clone.inner_to_dttm = query_object_clone.to_dttm
+
x_axis_label = get_x_axis_label(query_object.columns)
query_object_clone.granularity = (
query_object_clone.granularity or x_axis_label
)
+
+ logger.info(
+ f"After setting dates - from_dttm: {query_object_clone.from_dttm}, "
+ f"to_dttm: {query_object_clone.to_dttm}"
+ )
+ logger.info(
Review Comment:
same
##########
superset/common/query_context_processor.py:
##########
@@ -581,14 +689,26 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
cache = QueryCacheManager.get(
cache_key, CacheRegion.DATA, query_context.force
)
- # whether hit on the cache
+
if cache.is_loaded:
offset_dfs[offset] = cache.df
queries.append(cache.query)
cache_keys.append(cache_key)
continue
query_object_clone_dct = query_object_clone.to_dict()
+
Review Comment:
All these should be debugs or removed
##########
superset/common/query_context_processor.py:
##########
@@ -522,34 +545,92 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
offset, outer_to_dttm
)
+ # For relative offsets, keep the original behavior
Review Comment:
Not quite sure why altering the existing behavior, if we go this path then the FF doesn't guarantee we're not affecting the existing API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(timeshift): Add support for date range timeshifts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34375:
URL: https://github.com/apache/superset/pull/34375#discussion_r2245500381
##########
superset/common/query_object.py:
##########
@@ -302,6 +303,37 @@ def _validate_no_have_duplicate_labels(self) -> None:
)
)
+ def _validate_time_offsets(self) -> None:
+ """Validate time_offsets configuration"""
+ if not self.time_offsets:
+ return
+
+ for offset in self.time_offsets:
+ # Check if this is a date range offset (YYYY-MM-DD : YYYY-MM-DD format)
+ if self._is_valid_date_range(offset):
+ if not feature_flag_manager.is_feature_enabled(
+ "DATE_RANGE_TIMESHIFTS_ENABLED"
+ ):
+ raise QueryObjectValidationError(
Review Comment:
### Incomplete Time Offsets Validation Docstring <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The docstring doesn't explain why the validation is needed or what the time_offsets configuration should look like.
###### Why this matters
Without understanding the purpose and requirements of time_offsets validation, developers may misuse the feature or miss important constraints.
###### Suggested change ∙ *Feature Preview*
"""Validate time_offsets configuration
Validates that date range time shifts (in YYYY-MM-DD:YYYY-MM-DD format) are only used when
the DATE_RANGE_TIMESHIFTS_ENABLED feature flag is enabled. This prevents usage of date
range time shifts when the feature is not properly configured.
"""
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d3a44d0-b215-472e-b10c-51741b2492f7/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d3a44d0-b215-472e-b10c-51741b2492f7?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d3a44d0-b215-472e-b10c-51741b2492f7?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d3a44d0-b215-472e-b10c-51741b2492f7?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d3a44d0-b215-472e-b10c-51741b2492f7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ff1928f3-fb0a-400b-8be2-9e90ed2026aa -->
[](ff1928f3-fb0a-400b-8be2-9e90ed2026aa)
##########
superset/common/query_context_processor.py:
##########
@@ -642,15 +782,164 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
offset_dfs[offset] = offset_metrics_df
if offset_dfs:
+ logger.info("Before joining offset DataFrames:")
+ logger.info(f"Main DataFrame shape: {df.shape}")
+ logger.info(f"Main DataFrame:\n{df.to_string()}")
+ for offset_key, offset_df in offset_dfs.items():
+ logger.info(f"Offset '{offset_key}' DataFrame shape: {offset_df.shape}")
+ logger.info(
+ f"Offset '{offset_key}' DataFrame:\n{offset_df.to_string()}"
+ )
+
df = self.join_offset_dfs(
df,
offset_dfs,
time_grain,
join_keys,
)
+ logger.info("After joining offset DataFrames:")
+ logger.info(f"Final DataFrame shape: {df.shape}")
+ logger.info(f"Final DataFrame:\n{df.to_string()}")
+
return CachedTimeOffset(df=df, queries=queries, cache_keys=cache_keys)
+ def _process_date_range_offset(
+ self, offset_df: pd.DataFrame, join_keys: list[str]
+ ) -> tuple[pd.DataFrame, list[str]]:
+ """Process date range offset data and return modified DataFrame and keys."""
+ temporal_cols = ["ds", "__timestamp", "dttm"]
+ non_temporal_join_keys = [key for key in join_keys if key not in temporal_cols]
+
+ logger.info(
+ f"Non-temporal join keys for date range offset: {non_temporal_join_keys}"
+ )
+
+ if non_temporal_join_keys:
+ return offset_df, non_temporal_join_keys
+
+ # Aggregate offset data for cross join
+ logger.info("No non-temporal join keys found, aggregating offset data")
+
+ metric_columns = [col for col in offset_df.columns if col not in temporal_cols]
+ logger.info(f"Aggregating metric columns: {metric_columns}")
+
+ if metric_columns:
+ aggregated_values = {}
+ for col in metric_columns:
+ if pd.api.types.is_numeric_dtype(offset_df[col]):
+ aggregated_values[col] = offset_df[col].sum()
+ else:
+ aggregated_values[col] = (
+ offset_df[col].iloc[0] if not offset_df.empty else None
+ )
+
+ offset_df = pd.DataFrame([aggregated_values])
+ logger.info(f"Aggregated offset DataFrame:\n{offset_df.to_string()}")
+
+ return offset_df, []
+
+ def _apply_cleanup_logic(
+ self,
+ df: pd.DataFrame,
+ offset: str,
+ time_grain: str | None,
+ join_keys: list[str],
+ is_date_range_offset: bool,
+ ) -> pd.DataFrame:
Review Comment:
### Vague Cleanup Logic Documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The existing docstring doesn't explain what cleanup is being performed or the significance of is_date_range_offset parameter.
###### Why this matters
The current docstring is too vague to understand why different cleanup logic is needed for different offset types.
###### Suggested change ∙ *Feature Preview*
"""Apply appropriate cleanup logic based on offset type.
Different cleanup is performed depending on whether the offset is a date range
or relative offset:
- For relative offsets with time_grain: Reorders columns and drops temporary join columns
- For date range offsets: Only drops temporary right suffix columns
- For other cases: Only drops right suffix columns
"""
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c043d8c-73e6-4eee-bb6f-10321c6f0bef/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c043d8c-73e6-4eee-bb6f-10321c6f0bef?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c043d8c-73e6-4eee-bb6f-10321c6f0bef?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c043d8c-73e6-4eee-bb6f-10321c6f0bef?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c043d8c-73e6-4eee-bb6f-10321c6f0bef)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2de8d635-b1cc-405e-95e6-4dfc20c0daf5 -->
[](2de8d635-b1cc-405e-95e6-4dfc20c0daf5)
##########
superset/common/query_object.py:
##########
@@ -302,6 +303,37 @@
)
)
+ def _validate_time_offsets(self) -> None:
+ """Validate time_offsets configuration"""
+ if not self.time_offsets:
+ return
+
+ for offset in self.time_offsets:
+ # Check if this is a date range offset (YYYY-MM-DD : YYYY-MM-DD format)
+ if self._is_valid_date_range(offset):
+ if not feature_flag_manager.is_feature_enabled(
+ "DATE_RANGE_TIMESHIFTS_ENABLED"
+ ):
+ raise QueryObjectValidationError(
+ "Date range timeshifts are not enabled. "
+ "Please contact your administrator to enable the "
+ "DATE_RANGE_TIMESHIFTS_ENABLED feature flag."
+ )
+
+ def _is_valid_date_range(self, date_range: str) -> bool:
+ """Check if string is a valid date range in YYYY-MM-DD : YYYY-MM-DD format"""
+ try:
+ # Attempt to parse the string as a date range in the format
+ # YYYY-MM-DD:YYYY-MM-DD
+ start_date, end_date = date_range.split(":")
+ datetime.strptime(start_date.strip(), "%Y-%m-%d")
+ datetime.strptime(end_date.strip(), "%Y-%m-%d")
Review Comment:
### Missing Date Range Order Validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The date validation in _is_valid_date_range doesn't check if the start date is before the end date.
###### Why this matters
Without this validation, users could input invalid date ranges where the end date precedes the start date, potentially causing unexpected behavior in time series analysis.
###### Suggested change ∙ *Feature Preview*
```python
def _is_valid_date_range(self, date_range: str) -> bool:
try:
start_date_str, end_date_str = date_range.split(":")
start_date = datetime.strptime(start_date_str.strip(), "%Y-%m-%d")
end_date = datetime.strptime(end_date_str.strip(), "%Y-%m-%d")
return start_date <= end_date
except ValueError:
return False
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ce2b6d0-dd50-427a-b126-2cbb01869527/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ce2b6d0-dd50-427a-b126-2cbb01869527?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ce2b6d0-dd50-427a-b126-2cbb01869527?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ce2b6d0-dd50-427a-b126-2cbb01869527?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9ce2b6d0-dd50-427a-b126-2cbb01869527)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0f8b8908-ed3d-4683-b925-7f3afa6aa4ee -->
[](0f8b8908-ed3d-4683-b925-7f3afa6aa4ee)
##########
superset/common/query_context_processor.py:
##########
@@ -522,34 +545,92 @@
offset, outer_to_dttm
)
+ # For relative offsets, keep the original behavior
+ # BUT: Don't set inner bounds that conflict with the main query
+ # range. The inner bounds should match the shifted range, not the
+ # original range
+ query_object_clone.inner_from_dttm = query_object_clone.from_dttm
+ query_object_clone.inner_to_dttm = query_object_clone.to_dttm
Review Comment:
### Conflicting Time Range Bounds <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
For relative offsets, the inner bounds are set to match the shifted range, but this could cause conflicts with the main query range in certain scenarios.
###### Why this matters
Setting inner bounds to match the shifted range may create incorrect results when the shifted range overlaps with the main query range, leading to unexpected data filtering.
###### Suggested change ∙ *Feature Preview*
Remove the inner bounds setting for relative offsets to avoid conflicts:
```python
# For relative offsets, don't set inner bounds at all
query_object_clone.inner_from_dttm = None
query_object_clone.inner_to_dttm = None
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63239539-8dcf-4f72-a825-3567c01fbbf4/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63239539-8dcf-4f72-a825-3567c01fbbf4?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63239539-8dcf-4f72-a825-3567c01fbbf4?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63239539-8dcf-4f72-a825-3567c01fbbf4?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/63239539-8dcf-4f72-a825-3567c01fbbf4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8ef2208e-6d24-4754-9e81-75bc5887a06f -->
[](8ef2208e-6d24-4754-9e81-75bc5887a06f)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(timeshift): Add support for date range timeshifts in table charts [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on PR #34375:
URL: https://github.com/apache/superset/pull/34375#issuecomment-3132740398
Having this type of comparison doesn't sound like an offset at all to me, i.e, an offset is just a measure you apply to some base reference, this "date range offset" is more "compare two arbitrary dates". Additionally, you're mentioning table charts but changing the global `query_context_processor.py` so this might impact all other vizes that also support time offsets.
I like the idea behind this implementation but I consider we should not mix two concepts (`offset` vs `arbitrary`), thus, probably better to extend the current API? or probably even possible to leverage some post processor that does the comparison between N different ranges?
Also, having this type of arbitrary dates would require some UI changes to guide the user and I see nothing on that regard here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(timeshift): Add support for date range timeshifts [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #34375:
URL: https://github.com/apache/superset/pull/34375#discussion_r2247625567
##########
superset/common/query_context_processor.py:
##########
@@ -642,15 +782,164 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
offset_dfs[offset] = offset_metrics_df
if offset_dfs:
+ logger.info("Before joining offset DataFrames:")
+ logger.info(f"Main DataFrame shape: {df.shape}")
+ logger.info(f"Main DataFrame:\n{df.to_string()}")
+ for offset_key, offset_df in offset_dfs.items():
+ logger.info(f"Offset '{offset_key}' DataFrame shape: {offset_df.shape}")
+ logger.info(
+ f"Offset '{offset_key}' DataFrame:\n{offset_df.to_string()}"
+ )
Review Comment:
Let's remove all these logs for readability
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(timeshift): Add support for date range timeshifts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #34375:
URL: https://github.com/apache/superset/pull/34375#discussion_r2247911047
##########
superset/common/query_context_processor.py:
##########
@@ -522,34 +545,92 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
offset, outer_to_dttm
)
+ # For relative offsets, keep the original behavior
+ # BUT: Don't set inner bounds that conflict with the main query
+ # range. The inner bounds should match the shifted range, not the
+ # original range
+ query_object_clone.inner_from_dttm = query_object_clone.from_dttm
+ query_object_clone.inner_to_dttm = query_object_clone.to_dttm
+
x_axis_label = get_x_axis_label(query_object.columns)
query_object_clone.granularity = (
query_object_clone.granularity or x_axis_label
)
+
+ logger.info(
+ f"After setting dates - from_dttm: {query_object_clone.from_dttm}, "
+ f"to_dttm: {query_object_clone.to_dttm}"
+ )
+ logger.info(
+ f"Inner bounds - from: {query_object_clone.inner_from_dttm}, "
+ f"to: {query_object_clone.inner_to_dttm}"
+ )
+
except ValueError as ex:
raise QueryObjectValidationError(str(ex)) from ex
- # make sure subquery use main query where clause
- query_object_clone.inner_from_dttm = outer_from_dttm
- query_object_clone.inner_to_dttm = outer_to_dttm
+
query_object_clone.time_offsets = []
query_object_clone.post_processing = []
+
# Get time offset index
index = (get_base_axis_labels(query_object.columns) or [DTTM_ALIAS])[0]
- # The comparison is not using a temporal column so we need to modify
- # the temporal filter so we run the query with the correct time range
- if not dataframe_utils.is_datetime_series(df.get(index)):
- # Lets find the first temporal filter in the filters array and change
- # its val to be the result of get_since_until with the offset
- for flt in query_object_clone.filter:
- if flt.get("op") == FilterOperator.TEMPORAL_RANGE and isinstance(
- flt.get("val"), str
- ):
- time_range = cast(str, flt.get("val"))
- if self.is_valid_date_range(offset):
- flt["val"] = (
- f"{query_object_clone.from_dttm} : {query_object_clone.to_dttm}" # noqa: E501
+
+ # Handle temporal filters
+ if is_date_range_offset and feature_flag_manager.is_feature_enabled(
+ "DATE_RANGE_TIMESHIFTS_ENABLED"
+ ):
+ # DATE RANGE OFFSETS: Clean slate approach
+ logger.info("Handling temporal filters for date range offset")
+
+ # Create a completely new filter list to avoid conflicts
+ query_object_clone.filter = copy.deepcopy(query_object_clone.filter)
+
+ # Remove any existing temporal filters that might conflict
+ query_object_clone.filter = [
+ flt
+ for flt in query_object_clone.filter
+ if not (flt.get("op") == FilterOperator.TEMPORAL_RANGE)
+ ]
+
+ # Add our specific temporal filter
+ temporal_col = query_object_clone.granularity or x_axis_label
+ if temporal_col:
+ new_temporal_filter: QueryObjectFilterClause = {
+ "col": temporal_col,
+ "op": FilterOperator.TEMPORAL_RANGE,
+ "val": (
+ f"{query_object_clone.from_dttm} : "
+ f"{query_object_clone.to_dttm}"
+ ),
+ }
+ query_object_clone.filter.append(new_temporal_filter)
+ logger.info(
+ f"Added temporal filter for date range offset: "
+ f"{new_temporal_filter}"
+ )
+
+ else:
+ # RELATIVE OFFSETS: Original logic but fixed
Review Comment:
Same here, I am not really altering the existing behaviour just moving it into the else block for feature flag.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org