You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/09/14 21:35:23 UTC
[PR] test(mcp): pin distinct cache keys for semantic-view get_table across time ranges [superset]
mikebridge opened a new pull request, #44263:
URL: https://github.com/apache/superset/pull/44263
### SUMMARY
Regression test only — no code change. Pins the fix for SC-120967: an MCP `get_table` on a semantic view served cached results across different `time_range` values (a second request with the same filters but a different range was handed the first range's rows).
**Root cause / why no code change:** `get_table`'s `build_query_dict` (`superset/common/tabular_query.py`) expresses `time_range` only as a `TEMPORAL_RANGE` filter — it never sets `QueryObject.time_range`. `QueryContextFactory` resolves that filter to `from_dttm`/`to_dttm` and `_apply_granularity` drops the granularity-column temporal filter, so the resolved bounds are the only thing distinguishing one range's cache key from another's. Before **#43914** (`412ab43712`, merged 2026-09-10) `QueryObject.cache_key` dropped `from_dttm`/`to_dttm` **unconditionally**, so every range collided. #43914 keeps the bounds when `time_range` is unset — which already fixes this on master. This PR just locks the exact `get_table` scenario by name so it can't silently regress.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — test only.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/common/test_tabular_query.py -q # 33 passed
```
Two new tests in `tests/unit_tests/common/test_tabular_query.py`:
- `test_get_table_cache_key_distinguishes_time_ranges` — two builds through `build_query_dict` (the view path), same filters, different range → different cache keys.
- `test_get_table_cache_key_collides_when_bounds_dropped` — control: with the resolved bounds removed (the pre-#43914 behavior) the two otherwise-identical requests collapse to one key.
Verified the control the other way too: reverting #43914's condition in `cache_key` (dropping the bounds unconditionally) makes `test_get_table_cache_key_distinguishes_time_ranges` fail, confirming the test guards that fix.
### ADDITIONAL INFORMATION
- [x] Has associated issue: SC-120967 (root-caused to #43914; demo core `89964b2a` predates that fix)
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01C7U5oFfrqUyM6hJTZiW2uf
--
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] test(mcp): pin distinct cache keys for semantic-view get_table across time ranges [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44263:
URL: https://github.com/apache/superset/pull/44263#discussion_r4043512613
##########
tests/unit_tests/common/test_tabular_query.py:
##########
@@ -424,3 +425,76 @@ def test_one_sided_bound_uses_a_space_separator() -> None:
)["filters"]
assert "T" not in clause["val"]
+
+
+def _resolved_get_table_query_object(time_range: str) -> QueryObject:
+ """Model the semantic-view get_table query object at cache-key time.
+
+ ``build_query_dict`` (the get_table builder) expresses the range only as a
+ ``TEMPORAL_RANGE`` filter — it never sets ``time_range`` — and
+ ``QueryContextFactory`` then resolves that filter to ``from_dttm``/
+ ``to_dttm`` and ``_apply_granularity`` drops the granularity-column filter
+ before the cache key is taken. This reproduces that resolved state: no
+ ``time_range`` attribute, temporal filter removed, bounds resolved.
+ """
+ from superset.common.utils.time_range_utils import get_since_until_from_time_range
+
+ query_dict = build_query_dict(
+ time_column="metric_time",
+ metrics=["revenue"],
+ dimensions=["metric_time"],
+ filters=[{"col": "product__product_name", "op": "==", "val": "mel-bun"}],
+ time_range=time_range,
+ rewrite_one_sided_time_range=True, # the view path
Review Comment:
Agreed — `rewrite_one_sided_time_range=True` is inert for a two-sided range and the one-sided view ranges never collided with `_apply_granularity`; leaving the test as is. Thanks for the trace.
--
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] test(mcp): pin distinct cache keys for semantic-view get_table across time ranges [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44263:
URL: https://github.com/apache/superset/pull/44263
--
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] test(mcp): pin distinct cache keys for semantic-view get_table across time ranges [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44263:
URL: https://github.com/apache/superset/pull/44263#discussion_r4015796547
##########
tests/unit_tests/common/test_tabular_query.py:
##########
@@ -424,3 +425,76 @@ def test_one_sided_bound_uses_a_space_separator() -> None:
)["filters"]
assert "T" not in clause["val"]
+
+
+def _resolved_get_table_query_object(time_range: str) -> QueryObject:
+ """Model the semantic-view get_table query object at cache-key time.
+
+ ``build_query_dict`` (the get_table builder) expresses the range only as a
+ ``TEMPORAL_RANGE`` filter — it never sets ``time_range`` — and
+ ``QueryContextFactory`` then resolves that filter to ``from_dttm``/
+ ``to_dttm`` and ``_apply_granularity`` drops the granularity-column filter
+ before the cache key is taken. This reproduces that resolved state: no
+ ``time_range`` attribute, temporal filter removed, bounds resolved.
+ """
+ from superset.common.utils.time_range_utils import get_since_until_from_time_range
+
+ query_dict = build_query_dict(
+ time_column="metric_time",
+ metrics=["revenue"],
+ dimensions=["metric_time"],
+ filters=[{"col": "product__product_name", "op": "==", "val": "mel-bun"}],
+ time_range=time_range,
+ rewrite_one_sided_time_range=True, # the view path
Review Comment:
`rewrite_one_sided_time_range=True` is inert for a two-sided range — `_time_range_filters` returns TEMPORAL_RANGE unchanged, so output is identical with `False`. No gap follows: one-sided view ranges emit `>=`/`<`, which `_apply_granularity` keeps, so they never collided. No change needed.
--
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] test(mcp): pin distinct cache keys for semantic-view get_table across time ranges [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44263:
URL: https://github.com/apache/superset/pull/44263#issuecomment-5671132932
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] test(mcp): pin distinct cache keys for semantic-view get_table across time ranges [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44263:
URL: https://github.com/apache/superset/pull/44263#issuecomment-5671183744
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44263?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 57.39%. Comparing base ([`43a0031`](https://app.codecov.io/gh/apache/superset/commit/43a0031995636537ce5ff8909e4b7de07a0ee4d5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`36f4532`](https://app.codecov.io/gh/apache/superset/commit/36f4532c05630da1726f30453d5951ffffb17a8a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (43a0031) and HEAD (36f4532). Click for more details.
>
> <details><summary>HEAD has 7 uploads less than BASE</summary>
>
>| Flag | BASE (43a0031) | HEAD (36f4532) |
>|------|------|------|
>|python|8|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44263 +/- ##
===========================================
- Coverage 80.21% 57.39% -22.82%
===========================================
Files 2926 2926
Lines 172919 172919
Branches 40115 40115
===========================================
- Hits 138706 99246 -39460
- Misses 31619 72730 +41111
+ Partials 2594 943 -1651
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44263/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44263/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44263/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44263/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44263/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.21% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/44263/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.21% <ø> (-45.40%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44263/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44263/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44263?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] test(mcp): pin distinct cache keys for semantic-view get_table across time ranges [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44263:
URL: https://github.com/apache/superset/pull/44263#issuecomment-5680521305
<!-- Bito Reply -->
The observation is correct. In the context of `build_query_dict` and the subsequent resolution of temporal filters into `from_dttm` and `to_dttm` for the `QueryObject`, the `rewrite_one_sided_time_range` flag does not affect two-sided ranges. Since two-sided ranges are already fully defined by their bounds, the internal logic that handles one-sided range rewriting is not triggered, and the resulting `QueryObject` remains identical regardless of the flag's value.
**tests/unit_tests/common/test_tabular_query.py**
```
query_dict = build_query_dict(
time_column="metric_time",
metrics=["revenue"],
dimensions=["metric_time"],
filters=[{"col": "product__product_name", "op": "==", "val": "mel-bun"}],
time_range=time_range,
rewrite_one_sided_time_range=True, # the view path
)
```
--
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