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(Filters): Apply native & cross filters on common columns #30438

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

geido
Copy link
Member

@geido geido commented Sep 30, 2024

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

  1. Create a native filter that has no common columns with a chart
  2. Add the chart in scope
  3. Apply the filter
  4. The chart should not reload and emit a query

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot bot added dashboard:cross-filters Related to the Dashboard cross filters dashboard:native-filters Related to the native filters of the Dashboard labels Sep 30, 2024
@michael-s-molina michael-s-molina linked an issue Sep 30, 2024 that may be closed by this pull request
3 tasks
@kgabryje
Copy link
Member

/testenv up

Copy link
Contributor

@kgabryje Ephemeral environment spinning up at http://35.90.248.247:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@geido
Copy link
Member Author

geido commented Oct 1, 2024

Just realized that highlighting a filter that is not applicable should also be added as an enhancement for this PR. Working on that.

@geido geido marked this pull request as draft October 1, 2024 18:07
@geido geido marked this pull request as ready for review October 4, 2024 14:09
@sadpandajoe sadpandajoe added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Oct 4, 2024
@github-actions github-actions bot added doc Namespace | Anything related to documentation packages labels Oct 8, 2024
@geido
Copy link
Member Author

geido commented Oct 9, 2024

Holding on this while testing more cases

@michael-s-molina
Copy link
Member

michael-s-molina commented Oct 9, 2024

Some things I observed while testing

Screen.Recording.2024-10-09.at.14.52.06.mov
  • The first 2 charts should be affected by the filter but there's no filter indicator
  • If you check the query, the filter was applied but the chart was not reloaded

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.

@michael-s-molina
Copy link
Member

In this example, the last 2 charts are from different datasets but both have a gender column. Initially, the filter is correctly applied to both datasets but if I remove the last chart from the filter scope, it's still being affected.

Screen.Recording.2024-10-09.at.15.08.23.mov

Copy link
Member

@michael-s-molina michael-s-molina left a 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.

@geido
Copy link
Member Author

geido commented Oct 10, 2024

I found some problems while testing with the ephemeral environment.

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!

@apache apache deleted a comment from kgabryje Oct 10, 2024
@apache apache deleted a comment from github-actions bot Oct 10, 2024
@apache apache deleted a comment from github-actions bot Oct 10, 2024
@yousoph
Copy link
Member

yousoph commented Oct 10, 2024

/testenv up

Copy link
Contributor

@yousoph Ephemeral environment spinning up at http://34.214.222.103:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@apache apache deleted a comment from github-actions bot Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dashboard:cross-filters Related to the Dashboard cross filters dashboard:native-filters Related to the native filters of the Dashboard doc Namespace | Anything related to documentation packages size/XL v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cross-filtering is reloading unaffected charts
6 participants