fix: caching on viz with relative time ranges and time compare #10061
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10061 +/- ##
==========================================
- Coverage 70.49% 70.48% -0.01%
==========================================
Files 585 585
Lines 31074 31075 +1
Branches 3185 3185
==========================================
- Hits 21905 21904 -1
- Misses 9060 9061 +1
- Partials 109 110 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I looked up how inner_from_dttm and inner_to_dttm is instantiated and used, and it appears they're only used in one place, and even there they get the value of from_dttm/to_dttm: https://github.com/apache/incubator-superset/blob/280ade826c038d7e1172e3291e9dc14ebbce0dc2/superset/viz.py#L1249-L1250
Later on we can see that it is in fact falling back to using from_dttm/to_dttm: https://github.com/apache/incubator-superset/blob/280ade826c038d7e1172e3291e9dc14ebbce0dc2/superset/connectors/sqla/models.py#L997-L1001 and https://github.com/apache/incubator-superset/blob/280ade826c038d7e1172e3291e9dc14ebbce0dc2/superset/connectors/sqla/models.py#L1035-L1047
In addition, QueryObject doesn't support these properties. So it appears they're note really needed, and can be removed, and replaced with from_dttm/to_dttm wherever referenced.
| @@ -396,8 +396,9 @@ def cache_key(self, query_obj: QueryObjectDict, **extra: Any) -> str: | |||
| cache_dict = copy.copy(query_obj) | |||
| cache_dict.update(extra) | |||
|
|
|||
| for k in ["from_dttm", "to_dttm"]: | |||
| del cache_dict[k] | |||
| for k in ["from_dttm", "to_dttm", "inner_from_dttm", "inner_to_dttm"]: | |||
There was a problem hiding this comment.
@etr2460 can you also make this change in viz_sip38.py? Also we should probably update the comment on line 392 to reflect the change.
There was a problem hiding this comment.
According to @villebro he'd pull any viz changes into viz_sip38 sometime in the future. is that still the case? will update the comment
There was a problem hiding this comment.
Yes, don't worry about it, I'll be making updates to some of the SIP-38 affected files soon and can take care of these changes during that update.
tests/core_tests.py
Outdated
| viz.cache_key(qobj), viz.cache_key(qobj, time_compare="12 weeks") | ||
| ) | ||
|
|
||
| qobj["inner_from_dttm"] = "foo" |
There was a problem hiding this comment.
Could we make this a datetime object for consistency? I personally find unit tests can be a good mechanism for understanding how the code operates and thus ensuring the types are accurate helps to improve the readability.
|
@villebro i think we need to keep I assume there's some reason why we need to keep the original params around, and that's why they're referenced throughout the models |
|
@villebro we probably should double check the logic, i.e., it's clear they get the values of |
etr2460
force-pushed
the
erik-ritter--time-compare-cache
branch
from
4df9a48
to
87c67fb
Compare
|
where |
etr2460
force-pushed
the
erik-ritter--time-compare-cache
branch
from
87c67fb
to
905500e
Compare
There was a problem hiding this comment.
@etr2460 good point, I didn't realize that they were separated to facilitate time shifting. While you're at it, would it be possible to add an additional test that ensures that a different time_compare generates a different cache_key?
etr2460
force-pushed
the
erik-ritter--time-compare-cache
branch
from
905500e
to
ca8a9fb
Compare
|
@villebro i added another test case |
mistercrunch
added
🏷️ bot
SUMMARY
When loading a recently loaded chart with a relative time range and a time compare selected, the chart renders with the "cached" icons, but it clearly takes far too long to load (like a minute compared to the usual half a second for a cached chart. After investigation, it looks like the cache key isn't consistent for relative time range time compare extra queries. This fixes that issue
TEST PLAN
CI + new unit tests
ADDITIONAL INFORMATION
to: @john-bodley @mistercrunch @villebro