shivamgoel opened a new pull request, #43914: URL: https://github.com/apache/superset/pull/43914
### SUMMARY Documents a cache-key collision in `POST /api/v1/datasource/<type>/<id>/query` (#43527) as an executable test. **No fix here** — the fix has a design choice worth deciding first, described below. `build_query_dict` carries a time range as a `TEMPORAL_RANGE` filter and sets `granularity`, but never sets `time_range`. Three behaviours then compose: 1. `QueryObjectFactory` resolves `from_dttm`/`to_dttm` from the filter, so the **SQL is correct**. 2. `QueryContextFactory._apply_granularity` deletes the temporal filter, since `granularity` matches its column. 3. `QueryObject.cache_key` includes `time_range` only when set, and drops `from_dttm`/`to_dttm` unconditionally so relative ranges don't bake in timestamps. Nothing time-related survives into the key. Every range for a given datasource shares one key, so a second request with a different range is served the first range's rows until the entry expires. The frontend always sends `time_range`, which is why `/chart/data` doesn't show this. Reproduced against `birth_names` on unmodified `/chart/data` before this endpoint existed: `1965..1968` and `1966..1967` both returned 4492 rows under key `16bdac534a53`; the correct count for the latter is 1491. ### Two ways to fix, and why this PR doesn't pick one **Narrow** — set `time_range` in `build_query_dict`, matching what the frontend sends. One line, fixes this endpoint, touches nothing shared. Leaves `/chart/data` exposed for any non-frontend caller that sends only a `TEMPORAL_RANGE` filter. **Broad** — also bind the resolved bounds in `QueryObject.cache_key` when `time_range` is absent. Fixes every caller, but changes cache keys globally and invalidates existing entries in every deployment. Worth noting either way: because `cache_key` deliberately keys relative ranges by their expression, `"Last 7 days"` stays keyed to a fixed entry across days. The narrow fix bounds that staleness by cache TTL rather than eliminating it, which is existing chart behaviour. ### TESTING INSTRUCTIONS ```bash pytest tests/unit_tests/common/test_tabular_query_cache_key.py -rxX # 2 passed, 1 xfailed ``` The collision is `xfail(strict=True)`, following `tests/unit_tests/charts/test_non_additive_totals.py`, so CI stays green and a fix flips it to `XPASS(strict)`. Verified by applying the narrow fix locally: exactly one test flips, and the two bracketing invariants (bounds resolve; the temporal filter is dropped) still hold. 132 existing tests across `test_tabular_query`, `test_query_api`, `test_query_context_factory` and `query_object_test` pass with that fix applied. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
