fix(charts): preserve time filter for expression axes

[mikebridge]

SUMMARY

Fixes query construction for charts whose x-axis is an adhoc SQL expression and whose time range comes from dashboard filtering.

Without an explicit granularity, the physical datetime column was not retained as the time-filter subject. On ClickHouse this could produce an effectively unbounded read and trigger TOO_MANY_ROWS, even though the dashboard supplied a narrow time range. The fix infers the dataset main datetime column only for bounded adhoc expression axes and keeps that inferred value out of the legacy axis-rewrite and temporal-filter-removal path.

Regression coverage verifies that the expression remains the grouping dimension, the physical datetime predicate is generated, physical temporal axes are not rewritten, and independent temporal filters are preserved.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

Manual browser reproduction for screenshots:

1. Configure a ClickHouse table containing:

   • A physical DateTime column used as the dataset’s main datetime column.
   • Columns suitable for a calculated numeric dimension.
   • More total rows than the ClickHouse user’s max_rows_to_read setting.
   • Fewer rows than that limit within a narrow time range.

2. Add the table as a Superset dataset and configure its physical datetime column as the dataset’s main datetime
   column.

3. Create a chart with:

   • COUNT(*) as the metric.
   • An adhoc SQL expression as the x-axis.
   • No explicit granularity or physical datetime x-axis.

4. Add the chart to a dashboard and create a native time-range filter targeting the dataset’s main datetime
   column.

5. Select a narrow range whose matching rows are below max_rows_to_read.

6. On master, refresh the dashboard. The chart fails with ClickHouse TOO_MANY_ROWS because the generated
   aggregation does not include the physical datetime predicate.

7. Switch to this branch and refresh the same dashboard. The chart renders successfully.

8. Inspect the generated SQL and verify that:

   • The adhoc expression remains in SELECT and GROUP BY.
   • The dataset’s main datetime column appears in WHERE.
   • The predicate contains the selected dashboard time bounds.
   • Any independent temporal filters remain present.

TESTING INSTRUCTIONS

Automated:

pytest -q tests/unit_tests/common/test_query_context_factory.py tests/unit_tests/models/test_no_filter_time_range.py

Expected: 49 tests pass.

Manual ClickHouse reproduction:

1. Configure a ClickHouse dataset with a physical main datetime column and an adhoc SQL expression as the chart x-axis.
2. Set a restrictive max_rows_to_read value on the ClickHouse connection.
3. Apply a narrow dashboard native time-range filter.
4. On master, refresh the chart and observe TOO_MANY_ROWS because the physical time predicate is absent.
5. On this branch, refresh the same chart and verify it renders and the generated SQL filters the main datetime column while grouping by the SQL expression.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • 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

[bito-code-review]

Code Review Agent Run #f7fd79

Actionable Suggestions - 0

Review Details

• Files reviewed - 3 · Commit Range: 5915065..5915065

  • superset/common/query_context_factory.py
  • tests/unit_tests/common/test_query_context_factory.py
  • tests/unit_tests/models/test_no_filter_time_range.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.21%. Comparing base (753113d) to head (87a64f4).

Files with missing lines	Patch %	Lines
superset/common/query_context_factory.py	25.00%	2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (753113d) and HEAD (87a64f4). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (753113d)	HEAD (87a64f4)
python	4	2

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #42052      +/-   ##
==========================================
- Coverage   64.44%   56.21%   -8.24%     
==========================================
  Files        2747     2747              
  Lines      153874   153878       +4     
  Branches    35285    35286       +1     
==========================================
- Hits        99161    86499   -12662     
- Misses      52801    66549   +13748     
+ Partials     1912      830    -1082     

Flag	Coverage Δ
hive	39.05% <0.00%> (-0.01%)	⬇️
mysql	?
presto	41.04% <25.00%> (-0.01%)	⬇️
python	41.09% <25.00%> (-16.93%)	⬇️
sqlite	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mikebridge]

How this change fits into query construction

QueryContextFactory is the backend normalization step between the chart-data request and dataset SQL generation:

Browser chart configuration
  → QueryObjectFactory
  → QueryContextFactory
  → dataset SQL generator
  → database

QueryObjectFactory parses a dashboard time_range into from_dttm and to_dttm. Those values describe the bounds, but they do not identify the physical datetime column that belongs in the SQL WHERE clause.

In this code path, QueryObject.granularity is the physical datetime column used as the time-filter subject; the time grain such as hour/day/month is represented separately. Given:

granularity = "event_time"
from_dttm = ...
to_dttm = ...

the dataset SQL generator can produce:

WHERE event_time >= ...
  AND event_time < ...

Before this change, _apply_granularity() only reconciled the x-axis and temporal filters when query_object.granularity was already populated. For an adhoc SQL-expression x-axis, the request could contain valid from_dttm/to_dttm values but no granularity. The SQL generator therefore had bounds without a physical column and omitted the time predicate, allowing ClickHouse to read the full table and hit max_rows_to_read.

This patch adds a narrow fallback. When:

• the x-axis is an adhoc SQL expression;
• the query has no explicit granularity;
• at least one time bound exists; and
• the dataset main datetime column is present and temporal;

we set the validated main datetime column as the filter subject.

The immediate return is intentional. The rest of _apply_granularity() handles explicitly selected granularities and may rewrite the temporal x-axis or remove a temporal filter that it considers replaced. An inferred granularity has a narrower purpose: supply the physical column needed for the time predicate while leaving the SQL-expression grouping axis and independent temporal filters unchanged.

The resulting query keeps the expression in SELECT/GROUP BY and adds the dashboard bounds against the physical datetime column in WHERE.

[mikebridge]

Possible future follow-up: validate bounded queries have a time-filter subject

This PR fixes the concrete normalization gap, but a useful defensive improvement would be to validate normalized QueryObject instances before SQL generation.

The targeted invariant would be:

• from_dttm or to_dttm is present;
• no physical granularity/time column is present; and
• no explicit temporal filter identifies a physical column.

In that state, the supplied bounds cannot affect the generated SQL. Superset could emit a targeted warning or raise a query-validation error rather than silently executing an unbounded query.

A generic check that every QueryObject field appears in SQL would be too noisy because some fields intentionally affect post-processing, caching, annotations, or separate subqueries. The narrower time-constraint invariant has clear semantics and would have detected this failure before ClickHouse reached max_rows_to_read.

This is a suggested future follow-up, not additional scope or a blocker for this PR.

[eschutho]

Nice! Thanks @mikebridge!

[mikebridge]

Potential future follow-up (not a blocker for this PR): when granularity is inferred from main_dttm_col, a query that also contains an explicit TEMPORAL_RANGE filter on that same column may generate two equivalent time predicates because the inferred path returns before the existing filter cleanup. This should not change query results, but removing only the matching same-column temporal filter could avoid redundant SQL while continuing to preserve independent filters on other datetime columns.

It would also be useful for that follow-up to add coverage through the public query-context factory path—not only direct _apply_granularity tests—so we explicitly verify the distinction between a dashboard time_range and bounds derived solely from an explicit temporal filter, plus the same-column case above.

[bito-code-review]

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it