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(CrossFilters): Do not reload unrelated filters in global scope #30252

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

geido
Copy link
Member

@geido geido commented Sep 12, 2024

SUMMARY

This PR partially accounts for the issue reported on #30061. However, more work is required so that when a scope is applied the frontend can check columns / calculated columns across different datasets to see whether the query should be sent over the backend at all. Working on a follow-up PRs that should solve the issue for both native and cross filters.

BEFORE

before.mp4

AFTER

after.mp4

TESTING INSTRUCTIONS

  1. In global scope a cross filter should only affect charts of the same dataset

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 the dashboard:cross-filters Related to the Dashboard cross filters label Sep 12, 2024
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

👍🏼

geido and others added 3 commits September 13, 2024 14:20
Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
@geido geido merged commit dbab2fb into master Sep 13, 2024
34 checks passed
sadpandajoe pushed a commit that referenced this pull request Sep 13, 2024
…30252)

Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
(cherry picked from commit dbab2fb)
@sadpandajoe sadpandajoe added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Sep 13, 2024
@michael-s-molina
Copy link
Member

This PR partially accounts for the issue reported on #30061. However, more work is required so that when a scope is applied the frontend can check columns / calculated columns across different datasets to see whether the query should be sent over the backed at all. Working on a follow-up PRs that should solve the issue for both native and cross filters.

@geido Do you have a link for the follow-up that will fix the cross filter issue?

@geido
Copy link
Member Author

geido commented Sep 27, 2024

This PR partially accounts for the issue reported on #30061. However, more work is required so that when a scope is applied the frontend can check columns / calculated columns across different datasets to see whether the query should be sent over the backed at all. Working on a follow-up PRs that should solve the issue for both native and cross filters.

@geido Do you have a link for the follow-up that will fix the cross filter issue?

Still working on it

@rusackas rusackas deleted the geido/fix/cross-filters-rendering branch September 27, 2024 20:53
@fmannhardt
Copy link
Contributor

@geido I am testing the 4.1 branch and realized that our cross filters are not working anymore. Do I read this PR right in that it would entirely disable cross filtering if the data source is not the same, even if shared columns exist? And that this is fixed, also for columns not actually used in the chart, with: #30438

@geido
Copy link
Member Author

geido commented Oct 8, 2024

@geido I am testing the 4.1 branch and realized that our cross filters are not working anymore. Do I read this PR right in that it would entirely disable cross filtering if the data source is not the same, even if shared columns exist? And that this is fixed, also for columns not actually used in the chart, with: #30438

That is a good point @fmannhardt . I updated #30438 to make sure we are targeting all possible common columns in all datasources. Let me know if that makes sense to you now.

@fmannhardt
Copy link
Contributor

When testing including #30438 I still do not see the chart being refreshed. The query is correct and also when looking at the result table it is correct but the chart visualization itself does not show up refreshed.

@geido
Copy link
Member Author

geido commented Oct 9, 2024

When testing including #30438 I still do not see the chart being refreshed. The query is correct and also when looking at the result table it is correct but the chart visualization itself does not show up refreshed.

@fmannhardt would you mind providing a test case to reproduce this issue on my side, please? That would help.

@fmannhardt
Copy link
Contributor

Ok. I will try to provide it. Maybe it had to do with using JINJA templating in a filtering statement.

@fmannhardt
Copy link
Contributor

I have tried to reproduce with the latest changes on the other PR and the issue is gone. Thanks.

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 size/S 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.

6 participants