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/11/27 17:10:12 UTC
[PR] fix(timeshift): Add a more reliable strategy for correct temporal col [superset]
msyavuz opened a new pull request, #36309:
URL: https://github.com/apache/superset/pull/36309
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Add a more reliable strategy: Picking the temporal column from the applied filter, to pick correct temporal column for timeshift timeranges.
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### 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] fix(timeshift): Add a more reliable strategy for correct temporal col [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36309:
URL: https://github.com/apache/superset/pull/36309#issuecomment-3586823340
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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] fix(timeshift): Add a more reliable strategy for correct temporal col [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz merged PR #36309:
URL: https://github.com/apache/superset/pull/36309
--
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] fix(timeshift): Add a more reliable strategy for correct temporal col [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36309:
URL: https://github.com/apache/superset/pull/36309#discussion_r2576990092
##########
superset/models/helpers.py:
##########
@@ -1698,31 +1698,32 @@ def _get_temporal_column_for_filter( # noqa: C901
Helper method to reliably determine the temporal column for filtering.
This method tries multiple strategies to find the correct temporal column:
- 1. Use explicitly set granularity
- 2. Use x_axis_label if it's a temporal column
- 3. Find any datetime column in the datasource
+ 1. Use the column from existing TEMPORAL_RANGE filter
+ 2. Use explicitly set granularity
+ 3. Use x_axis_label if it exists
:param query_object: The query object
:param x_axis_label: The x-axis label from the query
:return: The name of the temporal column, or None if not found
"""
- # Strategy 1: Use explicitly set granularity
+ # Strategy 1: Use the column from existing TEMPORAL_RANGE filter
+ if query_object.filter:
+ for flt in query_object.filter:
+ if flt.get("op") == FilterOperator.TEMPORAL_RANGE:
+ col = flt.get("col")
+ if isinstance(col, str):
+ return col
+ elif isinstance(col, dict) and col.get("sqlExpression"):
+ return str(col.get("label") or col.get("sqlExpression"))
Review Comment:
Hey, AdhocColumn type is a dict containing label and no name. That's why i am checking it here. If the payload doesn't match the type we should do the update there but i don't think that would be the case.
```py
class AdhocColumn(TypedDict, total=False):
hasCustomLabel: bool | None
label: str
sqlExpression: str
isColumnReference: bool | None
columnType: Literal["BASE_AXIS", "SERIES"] | None
timeGrain: str | 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: 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] fix(timeshift): Add a more reliable strategy for correct temporal col [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #36309:
URL: https://github.com/apache/superset/pull/36309#discussion_r2571968258
##########
superset/models/helpers.py:
##########
@@ -1698,31 +1698,32 @@ def _get_temporal_column_for_filter( # noqa: C901
Helper method to reliably determine the temporal column for filtering.
This method tries multiple strategies to find the correct temporal column:
- 1. Use explicitly set granularity
- 2. Use x_axis_label if it's a temporal column
- 3. Find any datetime column in the datasource
+ 1. Use the column from existing TEMPORAL_RANGE filter
+ 2. Use explicitly set granularity
+ 3. Use x_axis_label if it exists
:param query_object: The query object
:param x_axis_label: The x-axis label from the query
:return: The name of the temporal column, or None if not found
"""
- # Strategy 1: Use explicitly set granularity
+ # Strategy 1: Use the column from existing TEMPORAL_RANGE filter
+ if query_object.filter:
+ for flt in query_object.filter:
+ if flt.get("op") == FilterOperator.TEMPORAL_RANGE:
+ col = flt.get("col")
+ if isinstance(col, str):
+ return col
+ elif isinstance(col, dict) and col.get("sqlExpression"):
+ return str(col.get("label") or col.get("sqlExpression"))
Review Comment:
When would `col` be a `dict`? From my tests, if you use custom SQL to create the original time range filter, the chart won't allow you to add a time comparison config. You can use a calculated column, but looking at `queries[0].filter` I still see this in the payload:
```
{col: "${COLUMN_NAME}", op: "TEMPORAL_RANGE", val: "2005 : 2006"}
```
Is this scenario a precautionary measure, or something you were able to reproduce? I'm asking it because I noticed you're looking for `label` or `sqlExpression`, and I was wondering if we should look for `name` too (which I think it depends if this is for a particular calculated column flow I couldn't reproduce, or specifically for custom SQL in the filter).
--
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] fix(timeshift): Add a more reliable strategy for correct temporal col [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #36309:
URL: https://github.com/apache/superset/pull/36309#discussion_r2577228712
##########
superset/models/helpers.py:
##########
@@ -1698,31 +1698,32 @@ def _get_temporal_column_for_filter( # noqa: C901
Helper method to reliably determine the temporal column for filtering.
This method tries multiple strategies to find the correct temporal column:
- 1. Use explicitly set granularity
- 2. Use x_axis_label if it's a temporal column
- 3. Find any datetime column in the datasource
+ 1. Use the column from existing TEMPORAL_RANGE filter
+ 2. Use explicitly set granularity
+ 3. Use x_axis_label if it exists
:param query_object: The query object
:param x_axis_label: The x-axis label from the query
:return: The name of the temporal column, or None if not found
"""
- # Strategy 1: Use explicitly set granularity
+ # Strategy 1: Use the column from existing TEMPORAL_RANGE filter
+ if query_object.filter:
+ for flt in query_object.filter:
+ if flt.get("op") == FilterOperator.TEMPORAL_RANGE:
+ col = flt.get("col")
+ if isinstance(col, str):
+ return col
+ elif isinstance(col, dict) and col.get("sqlExpression"):
+ return str(col.get("label") or col.get("sqlExpression"))
Review Comment:
Gotcha!
--
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