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

filter out dimension names that do not have dimension values #3726

Closed
wants to merge 5 commits into from

Conversation

briangregoryholmes
Copy link
Contributor

@briangregoryholmes briangregoryholmes commented Dec 19, 2023

When adding a temporary "filter" without an associated dimension value (a necessary part of the new filter button flow), the runtime throws an error.

There are three ways to solve this, in my opinion:

  1. Create a new piece of state on the FE to hold "temporary" filters that have no dimension values and render those out in addition to filters actually included in the dashboard state and queries. This PR does not do this as I believe it would add some unnecessary complexity.
  2. Filter out, on the frontend, the filters whose in property has no length before sending the query. This PR does this, but not in a centralized way. I would consider this a stopgap solution as adding a "filter" without a dimension value still triggers a new query, which is unnecessary. Changing this behavior would require additional thought.
  3. Allow for the runtime to receive filters with no dimension values, something that was possible, I believe, prior to Adding support for defining filters on measures #3624. This PR makes a small change that achieves this, but I have spent limited time looking at the runtime code and there may be a better approach.

2 and 3 both solve the issue independently, but I see no reason not to include both. I would likely separate them out into separate PRs. This is just to consolidate discussion.

@briangregoryholmes briangregoryholmes self-assigned this Dec 19, 2023
@briangregoryholmes briangregoryholmes marked this pull request as ready for review December 19, 2023 16:01
@briangregoryholmes briangregoryholmes marked this pull request as draft December 19, 2023 16:02
@ericpgreen2
Copy link
Contributor

Working backwards...

  • On 3, I think it’s a reasonable assumption/requirement that the runtime needs both a dimension name and a dimension value in order to construct a valid filter, so I think it’s correct that it returns a 400 in this scenario.
  • On 2, this does sound like a passable stopgap solution. But a couple things to note:
    • We've sought to reduce excess queries for Druid-backed dashboards in particular to improve performance & cost. So we should have a path toward preventing an intermediate filter state from triggering a new query.
    • We'll need to accept both in and like filter conditions.
  • On 1, I actually like this solution. I like it because it keeps the query payload (the filter object) clean until we have a valid one that we're ready to send to the runtime. This solution might make it easier to avoid the excessive, intermediate query requests.

@ericpgreen2
Copy link
Contributor

To reduce scope on 1, I don't think this temporary state needs to be stored in the URL. So we'd just have to add a new field to the dashboardStore, but not to the protobuf that's serialized into the URL's state query parameter. CC @AdityaHegde to check me on this.

@briangregoryholmes
Copy link
Contributor Author

Closed in favor of #3728

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants