fix(chart): fix Chart Time Grain option not working if Dataset is not saved#38766
fix(chart): fix Chart Time Grain option not working if Dataset is not saved#38766mkramer5454 wants to merge 2 commits intoapache:masterfrom
Conversation
Code Review Agent Run #6e1b9eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
codeant-ai-for-open-source
bot
added
the
size:S
Sequence DiagramThis PR ensures chart time grain is applied even when the dataset has not been saved. During adhoc column conversion, Superset now detects temporal columns from metadata and wraps them with the engine timestamp grain expression before query execution. sequenceDiagram
participant User
participant ChartUI
participant QueryBuilder
participant SqlaTable
participant DBEngineSpec
User->>ChartUI: Select time grain for adhoc time column
ChartUI->>QueryBuilder: Build query with adhoc column settings
QueryBuilder->>SqlaTable: Convert adhoc column to SQL column
SqlaTable->>SqlaTable: Check column metadata for temporal type and date format
SqlaTable->>DBEngineSpec: Apply timestamp expression for selected time grain
DBEngineSpec-->>QueryBuilder: Return time grained column expression
QueryBuilder-->>ChartUI: Execute chart query with correct time grouping
Generated by CodeAnt AI |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| sql_expression = col["sqlExpression"] | ||
| time_grain = col.get("timeGrain") | ||
| has_timegrain = col.get("columnType") == "BASE_AXIS" and time_grain | ||
| is_dttm = False | ||
| pdf = None | ||
|
|
||
| if col_in_metadata := self.get_column(sql_expression): | ||
| is_dttm = col_in_metadata.is_temporal |
There was a problem hiding this comment.
Suggestion: Column references are still treated as raw SQL identifiers, so names with spaces or reserved words can generate invalid SQL when time grain is applied. Quote isColumnReference inputs before processing the expression (while still using the raw name for metadata lookup) to avoid syntax errors in unsaved datasets. [logic error]
Severity Level: Major ⚠️
- ❌ Query datasource charts can fail SQL compilation.
- ⚠️ Time-grain charts break on quoted-required column names.
- ⚠️ SQL Lab unsaved-dataset explore flow becomes unreliable.| sql_expression = col["sqlExpression"] | |
| time_grain = col.get("timeGrain") | |
| has_timegrain = col.get("columnType") == "BASE_AXIS" and time_grain | |
| is_dttm = False | |
| pdf = None | |
| if col_in_metadata := self.get_column(sql_expression): | |
| is_dttm = col_in_metadata.is_temporal | |
| sql_expression = ( | |
| self.database.quote_identifier(col["sqlExpression"]) | |
| if col.get("isColumnReference", False) | |
| else col["sqlExpression"] | |
| ) | |
| time_grain = col.get("timeGrain") | |
| has_timegrain = col.get("columnType") == "BASE_AXIS" and time_grain | |
| is_dttm = False | |
| pdf = None | |
| if col_in_metadata := self.get_column(col["sqlExpression"]): |
Steps of Reproduction ✅
1. Call chart data API `POST /api/v1/chart/data` (entrypoint at
`superset/charts/data/api.py:199-260`) with a datasource of type `query` and an adhoc
datetime column in `queries[].columns`.
2. Request builds `QueryContext` via `QueryContextFactory.create()`
(`superset/common/query_context_factory.py:45-89`) and resolves datasource through
`DatasourceDAO.get_datasource()` to `Query` for `DatasourceType.QUERY`
(`superset/daos/datasource.py:39-43`).
3. Query generation in `ExploreMixin` uses `self.adhoc_column_to_sqla()` for adhoc columns
(`superset/models/helpers.py:2880-2883`), dispatching to `Query.adhoc_column_to_sqla()`
(`superset/models/sql_lab.py:397+`).
4. In current PR code, `sqlExpression` is passed unquoted into `_process_sql_expression`
then `literal_column` (`superset/models/sql_lab.py:411-429`), so identifier-like names
requiring quoting (spaces/reserved words) remain raw; when time grain wraps this
expression (`sql_lab.py:430-435`), compiled SQL can be invalid and chart query fails.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/sql_lab.py
**Line:** 411:418
**Comment:**
*Logic Error: Column references are still treated as raw SQL identifiers, so names with spaces or reserved words can generate invalid SQL when time grain is applied. Quote `isColumnReference` inputs before processing the expression (while still using the raw name for metadata lookup) to avoid syntax errors in unsaved datasets.
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #38766 +/- ##
==========================================
- Coverage 65.28% 64.38% -0.91%
==========================================
Files 1819 2532 +713
Lines 72700 129861 +57161
Branches 23239 30023 +6784
==========================================
+ Hits 47460 83606 +36146
- Misses 25240 44799 +19559
- Partials 0 1456 +1456
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes time grain not being applied when exploring charts from an unsaved SQL Lab query by applying the selected grain to eligible ad-hoc datetime columns during SQLAlchemy column generation.
Changes:
- Detect temporal ad-hoc columns in
Query.adhoc_column_to_sqlavia SQL Lab query column metadata. - Apply the engine-specific timestamp/time-grain expression (
get_timestamp_expr) whentimeGrainis provided for the base axis.
| if col_in_metadata := self.get_column(sql_expression): | ||
| is_dttm = col_in_metadata.is_temporal | ||
| pdf = col_in_metadata.python_date_format |
There was a problem hiding this comment.
self.get_column() is annotated to return Optional[dict[str, Any]], but here the returned value is treated as a TableColumn (accessing .is_temporal / .python_date_format). With mypy enabled in pre-commit/CI, this will raise type errors on these attribute accesses (and the get_column implementation also returns TableColumn instances). Consider updating Query.get_column to return TableColumn | None (and adjust its type annotation accordingly), or explicitly casting the result to TableColumn before using these attributes.
| if is_dttm and has_timegrain: | ||
| sqla_column = self.db_engine_spec.get_timestamp_expr( | ||
| col=sqla_column, | ||
| pdf=pdf, | ||
| time_grain=time_grain, | ||
| ) |
There was a problem hiding this comment.
This change introduces new behavior (applying timeGrain via get_timestamp_expr for SQL Lab Query datasources). There are existing unit/integration tests around time-grain handling for SqlaTable, but no coverage here to prevent regressions for unsaved datasets/Query datasources. Adding a focused unit test that builds a Query with extra['columns'] containing a temporal column and asserts the compiled SQL includes the expected time-grain expression would help lock this in.
User description
SUMMARY
Fixing scenario mentioned in #38529, time grain not being applied to charts when the dataset has not yet been saved.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2026-03-09.at.2.41.02.PM.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CodeAnt-AI Description
Apply time grain to adhoc datetime columns for unsaved datasets
What Changed
Impact
✅ Correct chart time grain for unsaved datasets✅ Accurate time-based aggregations in charts✅ Consistent grouping when switching time grains💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.