fix: Fix unintended cache misses with async queries #14291
Conversation
benjreinhart
changed the title
| @@ -1065,6 +1065,9 @@ def to_adhoc( | |||
| elif expression_type == "SQL": | |||
| result.update({"sqlExpression": filt.get(clause)}) | |||
|
|
|||
| deterministic_name = md5_sha_from_dict(result) | |||
| result["filterOptionName"] = deterministic_name | |||
There was a problem hiding this comment.
Yeah, I agree we should get some 👀 on this from folks more familiar. AFAICT, this preserves the existing behavior of generating a unique key (which I assume is needed on clients) but also keeps it deterministic so that hashing form_data will work properly (both with async/non-async caching).
There was a problem hiding this comment.
With a deterministic filterOptionName we of course now have the risk of having duplicate filterOptionNames which probably is precisely what the uuid4 key is aiming to avoid (could cause trouble in React components if these get returned to the frontend). So maybe we should deduplicate filters, too, which in edge cases would avoid unnecessary cache misses (=if the exact same filter has been defined twice vs another query where the same filter is only defined once)?
There was a problem hiding this comment.
Hmm... can you point to a specific example of where that might be an issue? Wouldn't each filter have different attributes? If not, is it a bug on the client that the same filters for the same chart would be added more than once? The idea is this should be deterministic for the same filter dict, but the set of filter dicts should have different properties, causing a different sha.
| # objects modify the form_data object. If the modified version were | ||
| # to be cached here, it will lead to a cache miss when clients | ||
| # attempt to retrieve the value of the completed async query. | ||
| original_form_data = copy.deepcopy(form_data) |
Codecov Report
@@ Coverage Diff @@
## master #14291 +/- ##
==========================================
- Coverage 76.79% 76.71% -0.09%
==========================================
Files 955 955
Lines 48251 48255 +4
Branches 6030 6030
==========================================
- Hits 37055 37018 -37
- Misses 11001 11042 +41
Partials 195 195
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -1065,6 +1065,9 @@ def to_adhoc( | |||
| elif expression_type == "SQL": | |||
| result.update({"sqlExpression": filt.get(clause)}) | |||
|
|
|||
| deterministic_name = md5_sha_from_dict(result) | |||
| result["filterOptionName"] = deterministic_name | |||
There was a problem hiding this comment.
With a deterministic filterOptionName we of course now have the risk of having duplicate filterOptionNames which probably is precisely what the uuid4 key is aiming to avoid (could cause trouble in React components if these get returned to the frontend). So maybe we should deduplicate filters, too, which in edge cases would avoid unnecessary cache misses (=if the exact same filter has been defined twice vs another query where the same filter is only defined once)?
benjreinhart
force-pushed
the
benjreinhart/query-caching
branch
from
860f7fc to
789e109
Compare
|
Following up on this: I just spent some time playing with the UI and poking through the code. I am pretty confident the As mentioned above, the only way I see a conflict is if two adhoc filters contained the exact same key/value pairs, which I don't think is an expected / desired use case to support. I think we're good on dashboards too since the filter handling should be scoped to a single chart. FWIW, I tested adding a duplicate filter pair, running the chart query, saving the chart, reloading it, etc., and it worked as expected. |
* bug: Fix unintended cache misses with async queries * Ensure sort order * Ensure columns are sorted * Update failing tests
mistercrunch
added
🏷️ bot
Some charts are rendering errors when
GLOBAL_ASYNC_QUERIESis turned on (#14289).Some of the fixes:
form_datato be used for validation on subsequent requests when clients request the query results. However,form_datais mutated by the code that runs the query and the background worker is storing the mutated object. Since this object is used to derive a unique cache key, any mutations to it will lead to a different key and, subsequently, a cache miss.form_datain a non-deterministic way before the cache key is derived from the object (e.g., adding UUIDs to it). This will always lead to a unique cache key.Note: even though this is broken in the async queries experience, it will benefit everyone since these fixes should prevent unnecessary cache misses in the regular flow.