-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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(Filters): Apply native & cross filters on common columns #30438
base: master
Are you sure you want to change the base?
Conversation
/testenv up |
@kgabryje Ephemeral environment spinning up at http://35.90.248.247:8080. Credentials are |
superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts
Outdated
Show resolved
Hide resolved
Just realized that highlighting a filter that is not applicable should also be added as an enhancement for this PR. Working on that. |
…o/fix/filters-applicability
Holding on this while testing more cases |
Some things I observed while testing Screen.Recording.2024-10-09.at.14.52.06.mov
I noticed this behavior only happens when you create a dashboard and go add filters. If you load a dashboard from the dashboard list, then these bugs disappear. |
In this example, the last 2 charts are from different datasets but both have a Screen.Recording.2024-10-09.at.15.08.23.mov |
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.
I found some problems while testing with the ephemeral environment.
…o/fix/filters-applicability
Thanks @michael-s-molina, I could not see many of these issues locally due to #30569 as I was under the false impression that I was testing the latest state of the code. I reverted that PR and now looking at these problems. Thanks for catching them! |
/testenv up |
@geido Ephemeral environment spinning up at http://54.214.55.123:8080. Credentials are |
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.
I did my set of tests and was not able to find problems! Even datasets with calculated columns worked.
Nice work @geido. This PR should significantly reduce the number of requests to the server, reducing costs and improving performance. Thank you!
Given that this PR touches a critical part of Superset, it would be good to have more approvals before merging the PR.
Screen.Recording.2024-10-11.at.6.40.10.PM.movI'm getting an unexpected server error on the FCC examples dashboard. Steps to repro:
|
SUMMARY
When in scope (global or specific) charts that are not applicable to the filter (do not share the same datasource or columns) were being applied anyway, causing unnecessary queries. This PR checks that the charts have columns in common or will exclude the charts.
AFTER
COVID.Vaccine.Dashboard.mp4
COVID.Vaccine.Dashboard.1.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION