[cache] Using the query as the basis of the cache key #4016

john-bodley · 2017-12-06T01:47:00Z

This PR resolves issue #3840 where previously the visualization data query key was a hashed form of the form_data.

Per the description in the issue the form_data stored in the DB which is what the /warm_up_cache/ endpoint leverages is not the same form data in the explorer view (which is augmented by both the front- and back-ends). Given the cached data is the query response it makes more sense to simply use the hashed query string as the cache key. In addition to resolving this issue the cache is based purely on the relevant portions of the form_data which are used to generate the query, thus UI changes to the color palette etc. will not violate the cache.

Note there is no way to determine via the cache API when the cache key was created and thus it is necessary to cache the DTTM alongside the data.

Note the cache is now explorer view agnostic, i.e., a cache key may be shared by multiple explorer views/slices.

to: @graceguo-supercat @michellethomas @mistercrunch @timifasubaa

john-bodley · 2017-12-06T01:48:00Z

superset/viz.py

@@ -203,8 +203,6 @@ def query_obj(self):

    @property
    def cache_timeout(self):
-        if self.form_data.get('cache_timeout'):


Given that the cache is explorer view agnostic the cache timeout is only associated with the query and not the explorer form_data.

mistercrunch · 2017-12-09T22:50:57Z

If using the query as the cache key, then we should use the query results as the payload.

john-bodley · 2017-12-10T08:18:10Z

@mistercrunch shouldn't the payload which includes annotations etc. remain unchanged per https://github.com/apache/incubator-superset/blob/master/superset/views/core.py#L1082. Wouldn't a augmented response to the /explore_json/ API endpoint require a number of additional frontend changes?

mistercrunch · 2017-12-11T05:28:02Z

My point is there are numerous controls that don't affect the query but do affect the pandas logic. Take the rolling averages on line charts for instance: same SQL, different backend pandas logic...

So there are two potential approaches here:

change the caching location from the whole payload to just the interactions from the backend to the database
be more cautious around the subset of the form data keys (which controls really) affect the cache_key

john-bodley · 2018-01-03T22:00:53Z

@mistercrunch sorry for the delay in responding to your comment. The PR is doing what you suggest as the first potential approach, i.e., the whole payload is not cached, only the data which is fetched from the database. The cache key is the deterministic database query.

mistercrunch · 2018-01-03T22:25:27Z

superset/viz.py

-        merge_extra_filters(form_data)
-        s = str([(k, form_data[k]) for k in sorted(form_data.keys())])
+    def cache_key(self, query_obj):
+        s = self.datasource.get_query_str(query_obj)


This makes it such that get_query_str needs to be deterministic. I think that's the case. Let's note that if we see cache-hit-misses in the future it may be related to get_query_str not being fully deterministic.

Correct. I'll add a docstring to provide more context.

mistercrunch · 2018-01-03T22:27:34Z

👍 This looks good to me. I'll let @graceguo-supercat push the merge button so that you can fit it best in your internal release schedule.

fabianmenges · 2018-01-04T13:07:59Z

superset/viz.py

-        form_data = self.form_data.copy()
-        merge_extra_filters(form_data)
-        s = str([(k, form_data[k]) for k in sorted(form_data.keys())])
+    def cache_key(self, query_obj):


We probably need to include the datasource itself => you can have 2 databases with the same tables, which as far as I can tell would cause collisions

Good catch, I think so too.

@fabianmenges/@mistercrunch I agree. I made the cache key a tuple of the database name and query string. PTAL.

mistercrunch · 2018-01-04T19:24:22Z

LGTM

fabianmenges · 2018-01-04T20:43:23Z

superset/viz.py

+
+        return hashlib.md5(
+            json.dumps((
+                self.datasource.database.database_name,


I think just using the Database name as part of the cache key can cause authorization issues. I think we need to include the superset Datasource unique id.

E.g. I'm authorized to only query a single schema in a Database but data for a schema that I don't have access to is cached.

Also the database name can still collide with other database names on a different server.

@fabianmenges I've made the change to use the datasource ID (which indirectly has the database information encoded in it).

GeorgeSirois · 2018-01-04T23:05:13Z

superset/viz.py

+        return hashlib.md5(
+            json.dumps((
+                self.datasource.id,
+                self.datasource.get_query_str(query_obj).encode('utf-8'),


I think this is probably already an issue with the existing setup, but you may also want to consider cases where different users may see different results for the same query and datasource based on out of band information, such as a user that gets passed through in the query context (e.g. row-level permission in a view).

closes apache#4222 Related to: apache#4016

closes #4222 Related to: #4016

* Working polygon layer for deckGL * add js controls * add thumbnail * better description * refactor to leverage line_column controls * templates: open code and documentation on a new tab (#4217) As they are external resources. * Fix tutorial doesn't match the current interface #4138 (#4215) * [bugfix] markup and iframe viz raise 'Empty query' (#4225) closes #4222 Related to: #4016 * [bugfix] time_pivot entry got missing in merge conflict (#4221) PR here #3518 missed a line of code while merging conflicts with time_pivot viz * Improve deck.gl GeoJSON visualization (#4220) * Improve geoJSON * Addressing comments * lint * refactor to leverage line_column controls * refactor to use DeckPathViz * oops

closes apache#4222 Related to: apache#4016

* Working polygon layer for deckGL * add js controls * add thumbnail * better description * refactor to leverage line_column controls * templates: open code and documentation on a new tab (apache#4217) As they are external resources. * Fix tutorial doesn't match the current interface apache#4138 (apache#4215) * [bugfix] markup and iframe viz raise 'Empty query' (apache#4225) closes apache#4222 Related to: apache#4016 * [bugfix] time_pivot entry got missing in merge conflict (apache#4221) PR here apache#3518 missed a line of code while merging conflicts with time_pivot viz * Improve deck.gl GeoJSON visualization (apache#4220) * Improve geoJSON * Addressing comments * lint * refactor to leverage line_column controls * refactor to use DeckPathViz * oops

closes apache#4222 Related to: apache#4016

* Working polygon layer for deckGL * add js controls * add thumbnail * better description * refactor to leverage line_column controls * templates: open code and documentation on a new tab (apache#4217) As they are external resources. * Fix tutorial doesn't match the current interface apache#4138 (apache#4215) * [bugfix] markup and iframe viz raise 'Empty query' (apache#4225) closes apache#4222 Related to: apache#4016 * [bugfix] time_pivot entry got missing in merge conflict (apache#4221) PR here apache#3518 missed a line of code while merging conflicts with time_pivot viz * Improve deck.gl GeoJSON visualization (apache#4220) * Improve geoJSON * Addressing comments * lint * refactor to leverage line_column controls * refactor to use DeckPathViz * oops

closes apache#4222 Related to: apache#4016

* Working polygon layer for deckGL * add js controls * add thumbnail * better description * refactor to leverage line_column controls * templates: open code and documentation on a new tab (apache#4217) As they are external resources. * Fix tutorial doesn't match the current interface apache#4138 (apache#4215) * [bugfix] markup and iframe viz raise 'Empty query' (apache#4225) closes apache#4222 Related to: apache#4016 * [bugfix] time_pivot entry got missing in merge conflict (apache#4221) PR here apache#3518 missed a line of code while merging conflicts with time_pivot viz * Improve deck.gl GeoJSON visualization (apache#4220) * Improve geoJSON * Addressing comments * lint * refactor to leverage line_column controls * refactor to use DeckPathViz * oops

john-bodley commented Dec 6, 2017

View reviewed changes

john-bodley force-pushed the john-bodley-issue-3840 branch 3 times, most recently from b09ca5e to 88c341e Compare December 7, 2017 08:22

john-bodley force-pushed the john-bodley-issue-3840 branch from 88c341e to fbe50c6 Compare December 11, 2017 19:06

john-bodley force-pushed the john-bodley-issue-3840 branch from fbe50c6 to 1ca47ea Compare January 3, 2018 22:09

mistercrunch reviewed Jan 3, 2018

View reviewed changes

john-bodley force-pushed the john-bodley-issue-3840 branch 2 times, most recently from 5e4940a to 96542e1 Compare January 4, 2018 07:20

fabianmenges reviewed Jan 4, 2018

View reviewed changes

john-bodley force-pushed the john-bodley-issue-3840 branch from 96542e1 to f4a8c6b Compare January 4, 2018 19:16

fabianmenges reviewed Jan 4, 2018

View reviewed changes

john-bodley force-pushed the john-bodley-issue-3840 branch from f4a8c6b to f3771a1 Compare January 4, 2018 21:03

GeorgeSirois reviewed Jan 4, 2018

View reviewed changes

john-bodley force-pushed the john-bodley-issue-3840 branch 4 times, most recently from ddf1a92 to 932cd27 Compare January 10, 2018 03:21

[cache] Using the query as the basis of the cache key

3c9c69a

john-bodley force-pushed the john-bodley-issue-3840 branch from 932cd27 to 3c9c69a Compare January 10, 2018 04:42

graceguo-supercat merged commit a7a6678 into apache:master Jan 12, 2018

mistercrunch added a commit to mistercrunch/superset that referenced this pull request Jan 17, 2018

[bugfix] markup and iframe viz raise 'Empty query'

de2b615

closes apache#4222 Related to: apache#4016

mistercrunch mentioned this pull request Jan 17, 2018

[bugfix] markup and iframe viz raise 'Empty query' #4225

Merged

mistercrunch added a commit that referenced this pull request Jan 17, 2018

[bugfix] markup and iframe viz raise 'Empty query' (#4225)

5897d85

closes #4222 Related to: #4016

hughhhh pushed a commit to lyft/incubator-superset that referenced this pull request Jan 18, 2018

[bugfix] markup and iframe viz raise 'Empty query' (apache#4225)

46411bc

closes apache#4222 Related to: apache#4016

john-bodley deleted the john-bodley-issue-3840 branch January 19, 2018 01:28

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018

[cache] Using the query as the basis of the cache key (apache#4016)

99e56be

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018

[bugfix] markup and iframe viz raise 'Empty query' (apache#4225)

53fffdc

closes apache#4222 Related to: apache#4016

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

[cache] Using the query as the basis of the cache key (apache#4016)

226e446

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

[bugfix] markup and iframe viz raise 'Empty query' (apache#4225)

ba9ed18

closes apache#4222 Related to: apache#4016

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cache] Using the query as the basis of the cache key #4016

[cache] Using the query as the basis of the cache key #4016

john-bodley commented Dec 6, 2017 •

edited

Loading

john-bodley Dec 6, 2017

mistercrunch commented Dec 9, 2017

john-bodley commented Dec 10, 2017

mistercrunch commented Dec 11, 2017

john-bodley commented Jan 3, 2018

mistercrunch Jan 3, 2018

john-bodley Jan 3, 2018

mistercrunch commented Jan 3, 2018

fabianmenges Jan 4, 2018

mistercrunch Jan 4, 2018

john-bodley Jan 4, 2018

mistercrunch commented Jan 4, 2018

fabianmenges Jan 4, 2018 •

edited

Loading

john-bodley Jan 4, 2018

GeorgeSirois Jan 4, 2018

[cache] Using the query as the basis of the cache key #4016

[cache] Using the query as the basis of the cache key #4016

Conversation

john-bodley commented Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

mistercrunch commented Dec 9, 2017

john-bodley commented Dec 10, 2017

mistercrunch commented Dec 11, 2017

john-bodley commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Jan 4, 2018

fabianmenges Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Dec 6, 2017 •

edited

Loading

fabianmenges Jan 4, 2018 •

edited

Loading