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

Fix cache for multiple time comparisons #5828

Merged
merged 3 commits into from
Sep 7, 2018

Conversation

betodealmeida
Copy link
Member

Currently we have these two conflicting behaviors:

  1. When computing the cache key from the query object "we remove datetime bounds that are hard values, and replace them with the use-provided inputs to bounds, which may be time-relative (as in "5 days ago" or "now")." This removes from_dttm and to_dttm from the query object when generating the cache dict.
  2. When running a query for a time comparison, say "1 week", we move the values stored in the keys from_dttm and to_dttm in the query object to inner_from_dttm and inner_to_dttm, and update the original keys with the shifted values.

When the second query runs, the cache key will be different from the first one even though from_dttm and to_dttm are stripped from both query objects, because the second one has the inner_from_dttm and inner_to_dttm keys set.

The problem is that multiple time shifts will have the same cache key, since the only thing differing them is from_dttm and to_dttm. This results in a false positive cache hit. I fixed it by allowing passing extra cache keys when fetching a dataframe. This way each time comparison will add, eg, time_compare: "1 week" to the cache dict.

@codecov-io
Copy link

codecov-io commented Sep 6, 2018

Codecov Report

Merging #5828 into master will increase coverage by 0.02%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5828      +/-   ##
==========================================
+ Coverage    63.7%   63.73%   +0.02%     
==========================================
  Files         366      368       +2     
  Lines       23143    23220      +77     
  Branches     2599     2600       +1     
==========================================
+ Hits        14744    14800      +56     
- Misses       8384     8405      +21     
  Partials       15       15
Impacted Files Coverage Δ
superset/viz.py 81.43% <80%> (+0.07%) ⬆️
...ts/src/dashboard/components/gridComponents/Tab.jsx 82.43% <0%> (-3.87%) ⬇️
superset/utils.py 89.02% <0%> (-0.15%) ⬇️
superset/assets/src/explore/components/Control.jsx 38.09% <0%> (ø) ⬆️
superset/data/__init__.py 100% <0%> (ø) ⬆️
superset/assets/src/visualizations/filter_box.jsx 0% <0%> (ø) ⬆️
...uperset/assets/src/visualizations/paired_ttest.jsx
superset/assets/src/visualizations/BigNumber.jsx
.../assets/src/visualizations/BigNumber/BigNumber.jsx 0% <0%> (ø)
.../src/dashboard/components/DeleteComponentModal.jsx 86.95% <0%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5eff7a8...79c0203. Read the comment docs.

@@ -327,7 +327,7 @@ def get_json(self):
self.get_payload(),
default=utils.json_int_dttm_ser, ignore_nan=True)

def cache_key(self, query_obj):
def cache_key(self, query_obj, **extra):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to add something in the docstring about what extra is used for currently, and could be used for in the future.

@mistercrunch
Copy link
Member

Minor comment on improving the cache_key docstring otherwise LGTM

@betodealmeida betodealmeida merged commit 299e20a into apache:master Sep 7, 2018
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 11, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 11, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 11, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Oct 29, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Nov 2, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Nov 2, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring

(cherry picked from commit 299e20a)
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Fix cache for multiple time comparisons

* Remove simple cache

* Improve docstring
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.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.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants