-
Notifications
You must be signed in to change notification settings - Fork 14k
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
chore: Remove obsolete legacy visualizations #24694
chore: Remove obsolete legacy visualizations #24694
Conversation
73e55ca
to
1163f6d
Compare
Codecov Report
@@ Coverage Diff @@
## master #24694 +/- ##
==========================================
- Coverage 68.97% 68.89% -0.08%
==========================================
Files 1901 1901
Lines 74008 73927 -81
Branches 8183 8183
==========================================
- Hits 51047 50932 -115
- Misses 20840 20874 +34
Partials 2121 2121
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
1163f6d
to
9e5b773
Compare
) | ||
payload = viz_obj.get_payload() | ||
return payload["data"] | ||
if chart.viz_type in viz_types: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same logic was previously but indented.
query_object.extras["time_grain_sqla"] = time_grain_sqla | ||
|
||
if time_range := overrides.get("time_range"): | ||
from_dttm, to_dttm = get_since_until_from_time_range(time_range) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The QueryObject
has both time_range
and (from_dttm
, to_dttm
) but I was only able to get the annotation override to adhere to the later.
e156bb8
to
20662cc
Compare
20662cc
to
155c6a8
Compare
@@ -424,6 +424,16 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]: | |||
viz_type="table", | |||
metrics=metrics, | |||
), | |||
query_context=get_slice_json( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-s-molina even though you were able to run the Celery tests locally without issue, after a few tweaks (increasing timeouts to address the performance degradation on M1 silica) I was able to see the following error:
I don't fully grok where/when the query context is persisted (the only place I could see where the query context was generated from the payload/form-data was via the /api/chart/v1//data/ RESTful API GET endpoint), but it does seem the existence of query context seemed somewhat sporadic. Maybe @villebro can provide more insights in terms of how/where this is generated/persisted as in the example data currently only the "Pivot Table v2" chart (per the slices.query_context
column) has the query context explicitly defined yet there are swaths of non-legacy charts.
The TL;DR is similar to the "Pivot Table v2" chart when I explicitly added the query context for the "Daily Totals" chart (which backs the annotation layer in the Cypress test which was failing) the test passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for the cleanup.
(cherry picked from commit 1b5a679)
SUMMARY
@michael-s-molina apologies for all the churn, but I'm unable to re-open #24671 in GitHub as the branch has been forced pushed after the PR was closed.
Delving a little deeper into the annotation logic I was able to rework the logic in said PR in order for the Cypress tests to pass., i.e., this PR is a superset (no pun intended) of #24671 . Thankfully means that we're able to remove the
TableViz
visualization type which simplifies the visualization logic, i.e., now every chart is defined as either a legacy chart or a non-legacy chart but not both.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION