Repository navigation
Conversation
✅ 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}
| cached = _cache_get(cache_key) | ||
| if cached is not None: | ||
| _count("cache_hit") | ||
| return cached | ||
|
|
||
| failure = _cache_get(_failure_cache_key(cache_key)) | ||
| if failure is not None: | ||
| _count("failure_cache_hit") | ||
| if errors is not None: | ||
| errors.extend(failure) | ||
| return None | ||
|
|
||
| _count("cache_miss") | ||
| probe_errors: list[str] = [] | ||
| cached = _run_probe( | ||
| database, catalog, schema, transform, distinct, errors=probe_errors |
There was a problem hiding this comment.
Suggestion: Concurrent requests can all miss this key and call _run_probe before any result is stored, causing a cold probe to fan out to the warehouse.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Race condition
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/connectors/sqla/partition_mapping.py
**Line:** 1922:1937
**Comment:**
*Race Condition: Concurrent requests can all miss this key and call `_run_probe` before any result is stored, causing a cold probe to fan out to the warehouse.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of hughhhh
Not changing this: the fan-out is real, but it's bounded, and I think a lock costs more than it saves. _cached_probe has no lock, so concurrent requests on a cold key can each call _run_probe.
Each of those probes is one SELECT of at most PARTITION_TRANSFORM_PROBE_MAX_VALUES columns, abandoned after PARTITION_TRANSFORM_PROBE_TIMEOUT where the alarm can fire (main thread only). The first result then fills the cache for later requests. Superset's chart-data cache has no single-flight either: I found no lock in query_context_processor.py or query_cache_manager.py.
A lock would need a backend-specific primitive, and it wouldn't work under the per-process fallback across workers. The request that loses the lock would also have to wait on the chart path or give up pruning. That trade-off is my judgment, and I didn't measure the fan-out under load, so I'm leaving this thread open for a maintainer to overrule.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45559 +/- ##
=======================================
Coverage 84.93% 84.93%
=======================================
Files 3077 3077
Lines 208205 208292 +87
Branches 49105 49117 +12
=======================================
+ Hits 176834 176923 +89
+ Misses 28002 28001 -1
+ Partials 3369 3368 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
requires:rebase
|
2 similar comments
|
|
00d1607 to
89317b9
Compare
The synchronous probe on the chart-query path had no upper bound on its work: a failing transform was re-probed on every query, a wide IN list became one SELECT column per value (PostgreSQL refuses a 1,665th), nothing limited its runtime, and under the default NullCache even successful probes re-ran on every chart load. - PARTITION_TRANSFORM_PROBE_MAX_VALUES (1000): decline wider filters. - PARTITION_TRANSFORM_PROBE_TIMEOUT (10s): utils.timeout around get_df. - PARTITION_TRANSFORM_PROBE_FAILURE_CACHE_TIMEOUT (5m): remember failures, with the engine's messages so the preview can still explain them. - Fall back to DATA_CACHE_CONFIG, then a bounded per-process cache, when CACHE_CONFIG is a NullCache. - partition_mapping.probe.* outcome counters and latency_ms timing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The retry test set the failure timeout to 0, which disables the failure cache, so nothing was ever stored to expire. It would still pass if failures were kept for the 24-hour success timeout. Hold the failure for 300s, then move the cache clock: still remembered at 299s, retried at 301s. The zero-timeout case keeps its own test. Also correct the `_run_probe` docstring, which still said a failed probe is never cached. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI's auto-walrus hook rewrote this assignment and failed pre-commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…apart Master's comment-stripping decline landed with no `declined.<reason>` metric, so it was invisible among the probe's other outcomes. Pin that data validation's fresh probe neither reads nor writes the failure memory, and move the value cap into its own helper to keep `evaluate_transform` within the complexity limit now that it also carries the fresh path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
89317b9 to
14907c0
Compare
| seen.add(key) | ||
| distinct.append(value) | ||
|
|
||
| if _exceeds_probe_cap(distinct, errors): |
There was a problem hiding this comment.
When this cap rejects the combined probe, _evaluate_with_text_forms treats the None as the engine refusing the text forms: it re-asks the typed values alone and records every text answer as TEXT_REFUSED, even though the text forms were never sent. With the default 1,000 cap, a mapping with 501 to 1,000 distinct timestamps (each adds its text form, so the combined probe is over the cap while the typed values are not) reaches that path, and the text-vs-typed comparison in _declined_for_reading_by_type is replaced by the looser _is_bucket_of check. A transform that answers the day for a typed timestamp but keeps the hour for its text form would then pass and mirror a midnight-to-midnight predicate that excludes matching rows. Should an over-cap probe decline outright in _evaluate_with_text_forms (or should the cap be applied before the text forms are added) so it never falls back to the typed-only answer?
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of hughhhh
Fixed in 02d334c: you're right, and an over-cap probe now declines outright. _evaluate_with_text_forms checks the cap on the values plus their text forms before it sends anything, so the typed-only fallback only runs after a real engine refusal.
test_text_forms_crossing_the_probe_cap_decline_rather_than_read_as_refused failed before the fix: the typed values were probed and TEXT_REFUSED was recorded. It now asserts no get_df call, empty text_answers, and the cap message in errors. That test checks the decline directly; I didn't run your day-versus-hour transform end to end, because it can no longer reach _is_bucket_of this way.
The effective default is therefore 500 distinct timestamps in one probe, which the docs now state. I've left this thread open for you to check.
| assert errors == ["Partition transform probe exceeded 7s"] | ||
|
|
||
|
|
||
| def test_a_null_cache_falls_back_to_a_local_one(app: Flask) -> None: |
There was a problem hiding this comment.
_probe_cache() now has a middle tier (DATA_CACHE_CONFIG), but this test sets both caches to NullCache, so it only exercises the per-process fallback. If cache_manager.data_cache were dropped from the lookup, or checked before cache_manager.cache, every test here would still pass, and a deployment that configures only DATA_CACHE_CONFIG would go back to re-probing on every chart load. Could you add a case with a null primary cache and a SimpleCache data cache that asserts one get_df call across two evaluations (with _LOCAL_PROBE_CACHE cleared in between), and one where a usable primary cache is preferred over the data cache?
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of hughhhh
Added in 02d334c: this was a real gap in the tests, and _probe_cache itself didn't need changing.
test_a_null_primary_cache_falls_back_to_the_data_cacheuses a null primary and aSimpleCachedata cache, clears_LOCAL_PROBE_CACHEbetween evaluations, and asserts oneget_dfcall.test_a_usable_primary_cache_is_preferred_over_the_data_cacheasserts the data cache is never read or written when the primary is usable.
I checked both by mutating the lookup: dropping data_cache fails the first test, and swapping the order fails the second. I've left this thread open for you to check.
|
|
||
| The evaluation is bounded. Every distinct filter value is one column of a single `SELECT`, | ||
| and engines cap the select list — PostgreSQL refuses more than 1,664 entries — so a filter | ||
| with more than `PARTITION_TRANSFORM_PROBE_MAX_VALUES` distinct values (1,000 by default) is |
There was a problem hiding this comment.
This reads as a per-filter limit, but mirrored_predicates flattens every request in the query into one list before evaluate_transform dedupes and applies the cap, and each timestamp is also probed again in its text form. Two filters with 600 distinct values each therefore exceed the default and none of the query's mirrored predicates apply, even though neither filter is over 1,000. Could the text here (and the setting's description in the table below) say the cap counts the combined distinct inputs of one probe rather than the values of one filter?
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of hughhhh
Fixed in 02d334c: the docs and the setting's table row now say the cap counts one evaluation's distinct inputs. That means all of a query's mirrored filters together, with each timestamp counted twice, and the docs use your two-filters-of-600 example. The comment on the setting in superset/config.py says the same.
test_the_probe_cap_counts_every_filter_of_the_query pins the behaviour through build_mirrored_predicates: two filters of two values against a cap of 3 mirror nothing and make no get_df call. It passed before this change, since only the wording was wrong. I've left this thread open for you to check.
When the text forms pushed a probe past PARTITION_TRANSFORM_PROBE_MAX_VALUES, `_evaluate_with_text_forms` read the decline as the engine refusing the text: it re-asked the typed values alone and recorded TEXT_REFUSED for text that was never sent. That replaced the typed-versus-text comparison with the looser `_is_bucket_of`, so a transform reading the two differently could mirror a range that excludes matching rows. Decline the over-cap probe outright. Say in the docs and config that the cap counts one probe's distinct inputs: every mirrored filter's values together, with each timestamp counted twice. Cover the DATA_CACHE_CONFIG tier and the order `_probe_cache` tries it in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
removed
the
requires:rebase
SUMMARY
Stacked on #43870, addressing the review point on probe bounds. Before this change, the synchronous probe on the chart-query path had nothing limiting how much work it did:
SELECTcolumn, so 1,665 values produce 1,665 columns. Reproduced againstpostgres:17: 1,664 columns succeed, and 1,665 fail withtarget lists can have at most 1664 entries. That failure was also not cached, so it repeated on every load.NullCachere-probed successes too, and the pruning-indicator verdict was never stored.Chart-result cache hits were already protected:
get_extra_cache_keysbuilds without mirrors.Changes
PARTITION_TRANSFORM_PROBE_MAX_VALUES0means no cap.PARTITION_TRANSFORM_PROBE_TIMEOUTutils.timeoutaroundget_df, best effort like Superset's other timeouts (SIGALRM, main thread only).0disables it.PARTITION_TRANSFORM_PROBE_FAILURE_CACHE_TIMEOUT<key>:failedalong with the engine's messages, so the preview can still explain them.0restores retry-on-every-query.CACHE_CONFIGis aNullCache, probe results and verdicts go toDATA_CACHE_CONFIG, and when that is also aNullCache, to a per-processSimpleCache(threshold=1024). The preview rate limiter still readsCACHE_CONFIGonly.partition_mapping.probe.:cache_hit,cache_miss,failure_cache_hit,success,error,timeout,declined.{too_many_values,value_does_not_round_trip,unstorable_transform,unsafe_probe_sql}, plus alatency_mstiming. A stats-logger failure never affects the chart.test_a_failed_probe_is_not_cachedasserted the old contract. It is replaced by "remembered briefly" and "retried once expired (TTL 0)" tests, plus new tests for the cap, the timeout, the NullCache fallback, metrics, and a broken stats logger. Atests/unit_tests/conftest.pyfixture clears the per-process fallback between tests.TESTING INSTRUCTIONS
Locally: 644 passed for those three files. A wider sweep (
models,connectors,datasets,commands/dataset,views/datasource,common,charts,dao/dataset_test.py, plus the Hive and Impala engine-spec tests) gave 2,936 passed and 2 xfailed. ruff and the pre-commit mypy 1.15 env are clean.ADDITIONAL INFORMATION
PARTITION_FILTER_MAPPING🤖 Generated with Claude Code