Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the query_obj as the basis for the cache key #4260

Merged
merged 2 commits into from
Jan 28, 2018

Conversation

mistercrunch
Copy link
Member

When we recently moved from hashing form_data to define the cache_key
towards using the rendered query instead,
it made is such that non deterministic (time-relative) form
control values like relative times specified in "from" and "until" time
bound resulted in making those miss cache 100% of the time.

Here we move away from using the rendered query and using the query_obj
instead. For this to work, we need to remove ['from_dttm', 'to_dttm'] and add some keys (['since', 'until', 'datasource']) from form_data

When we recently moved from hashing form_data to define the cache_key
towards using the rendered query instead,
it made is such that non deterministic form
control values like relative times specified in "from" and "until" time
bound resulted in making those miss cache 100% of the time.

Here we move away from using the rendered query and using the query_obj
instead.
@john-bodley
Copy link
Member

@mistercrunch doesn't the query object include the form_data and thus wouldn't this issue still persist? #3840

@mistercrunch
Copy link
Member Author

@john-bodley oh man. Good catch. I had forgotten that the query_obj contains the form_data. It's used for templating use case the user is using a tempalted SQL query (a Superset view), coming from SQL Lab.

Now I still think this is the right high level approach, but that we need to mitigate the templating use case somehow. Maybe have users opt-in on passing the form_data (?). Perhaps a new Slice boolean "allow form_data templating". Templating brings potentially other problems around things being non-deterministic... Let me do a bit more thinking here.

@mistercrunch
Copy link
Member Author

What about I deprecate using form_data in templates? Can you run a query as to whether and how it's used at Airbnb?

Here's the query to figure that out:

SELECT * FROM tables WHERE sql like '%{{%form_data%}}%'

@john-bodley
Copy link
Member

john-bodley commented Jan 25, 2018

@mistercrunch I slight tweak to the query, but

SELECT * FROM tables WHERE 'sql' LIKE '%%form_data%%'

returned zero rows (ran in SQL Lab).

@hughhhh
Copy link
Member

hughhhh commented Jan 25, 2018

👍

@john-bodley
Copy link
Member

LGTM

@mistercrunch mistercrunch merged commit e4a95f9 into apache:master Jan 28, 2018
@mistercrunch mistercrunch deleted the cache_key_controls branch January 28, 2018 17:46
@villebro villebro mentioned this pull request May 2, 2018
3 tasks
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* Use the query_obj as the basis for the cache key

When we recently moved from hashing form_data to define the cache_key
towards using the rendered query instead,
it made is such that non deterministic form
control values like relative times specified in "from" and "until" time
bound resulted in making those miss cache 100% of the time.

Here we move away from using the rendered query and using the query_obj
instead.

* Deprecating using form_data in templates
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Use the query_obj as the basis for the cache key

When we recently moved from hashing form_data to define the cache_key
towards using the rendered query instead,
it made is such that non deterministic form
control values like relative times specified in "from" and "until" time
bound resulted in making those miss cache 100% of the time.

Here we move away from using the rendered query and using the query_obj
instead.

* Deprecating using form_data in templates
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants