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

feat(insights): Add dashboard filters to hogql insights #18076

Merged
merged 13 commits into from
Oct 20, 2023

Conversation

robbie-c
Copy link
Member

@robbie-c robbie-c commented Oct 18, 2023

Status: it works!

Problem

#18065

Changes

  • Add apply_dashboard_filters to the hogql query runner
  • Add a top level apply_dashboard_filters function which can route query nodes into the correct runner
  • Hook this into to_representation in InsightSerializer, and into generate_insight_cache_key

How did you test this code?

@robbie-c robbie-c changed the title Add dashboard filters to hogql feat(insights): Add dashboard filters to hogql insights Oct 19, 2023
@robbie-c robbie-c force-pushed the add-dashboard-filters-to-hogql branch from 2a88714 to 2e37b3b Compare October 19, 2023 08:05
@robbie-c robbie-c changed the base branch from master to convert-all-insight-schema-to-py October 19, 2023 08:05
Base automatically changed from convert-all-insight-schema-to-py to master October 19, 2023 08:54
@robbie-c robbie-c force-pushed the add-dashboard-filters-to-hogql branch from 03ef6f6 to c851102 Compare October 19, 2023 09:12
@robbie-c robbie-c marked this pull request as ready for review October 19, 2023 14:45
@robbie-c robbie-c force-pushed the add-dashboard-filters-to-hogql branch from 8b3e185 to 5fc6527 Compare October 19, 2023 14:58
@robbie-c robbie-c requested a review from thmsobrmlr October 20, 2023 08:08
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Some comments, though nothing that seems blocking right now. Might be worth removing the query mutation though, as I'm always afraid of odd side effects with these things..

frontend/src/queries/schema.ts Outdated Show resolved Hide resolved
Comment on lines +70 to +81
def apply_dashboard_filters(self, dashboard_filter: DashboardFilter) -> HogQLQuery:
self.query.filters = self.query.filters or HogQLFilters()
self.query.filters.dateRange = self.query.filters.dateRange or DateRange()

if dashboard_filter.date_to or dashboard_filter.date_from:
self.query.filters.dateRange.date_to = dashboard_filter.date_to
self.query.filters.dateRange.date_from = dashboard_filter.date_from

if dashboard_filter.properties:
self.query.filters.properties = (self.query.filters.properties or []) + dashboard_filter.properties

return self.query
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we actually should mutate self.query here. It could be an object that's passed to the runner without any expectation that it's going to change.

Perhaps it's thus "cleaner" to save the filters to self._dashboard_filter = dashboard_filter, and merge it on the fly in to_query?

Alternatively, we could just store this instance variable in a generic function query_runner.py, and expose something like get_dashboard_filters, which each query runner calls at the right time to fetch them down, if any... 🤔

posthog/models/insight.py Show resolved Hide resolved
posthog/models/insight.py Show resolved Hide resolved
@robbie-c robbie-c force-pushed the add-dashboard-filters-to-hogql branch from 529872e to 7b6fcfe Compare October 20, 2023 11:45
@robbie-c robbie-c merged commit b3c4d41 into master Oct 20, 2023
73 checks passed
@robbie-c robbie-c deleted the add-dashboard-filters-to-hogql branch October 20, 2023 14:00
daibhin pushed a commit that referenced this pull request Oct 23, 2023
* WIP

* Kinda sorta working

* Add dashboard filters to cache key

* Make mypy happy

* Add support for properties

* Use get_query_runner

* Add tests for dashboard_query

* Add tests for merging of filters

* Add test for hash when dashboard filters are applied

* Silently accept having no valid query runner

* Remove index type from DashboardFilter

* Fix type of dashboard_query

* Fix test typing
Justicea83 pushed a commit to Justicea83/posthog that referenced this pull request Oct 24, 2023
* WIP

* Kinda sorta working

* Add dashboard filters to cache key

* Make mypy happy

* Add support for properties

* Use get_query_runner

* Add tests for dashboard_query

* Add tests for merging of filters

* Add test for hash when dashboard filters are applied

* Silently accept having no valid query runner

* Remove index type from DashboardFilter

* Fix type of dashboard_query

* Fix test typing
@zakbenney
Copy link

Hi @robbie-c ! Sorry for referencing an old thread and apologies for being a noob. I can see that you added a fix to the issue of not being able to apply dashboard filters to HogQL insights. However, I can't figure out how to adjust my queries so that this works for me. Any chance you might be able to give me some guidance??

@thmsobrmlr
Copy link
Contributor

Hi @robbie-c ! Sorry for referencing an old thread and apologies for being a noob. I can see that you added a fix to the issue of not being able to apply dashboard filters to HogQL insights. However, I can't figure out how to adjust my queries so that this works for me. Any chance you might be able to give me some guidance??

Hey @zakbenney, you can use {filters} in your HogQL query, which will then get expanded. For an example head to the SQL tab on a new insight page.

@zakbenney
Copy link

Hi @thmsobrmlr . Thanks so much! That works perfectly. Apologies again for my lack of expertise!

@thmsobrmlr
Copy link
Contributor

Hi @thmsobrmlr . Thanks so much! That works perfectly. Apologies again for my lack of expertise!

No worries. Glad I could help.

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.

5 participants