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(hogql): Put retention actor appearances in columns #19503

Merged
merged 12 commits into from
Jan 8, 2024

Conversation

webjunkie
Copy link
Contributor

Problem

The result of the retention actors query is hard to understand.

Changes

  • put booleans of the returns as columns
  • we can let the SQL query do that
  • anyone can now filter from frontend or export this way conveniently
image

How did you test this code?

  • click in frontend

@webjunkie
Copy link
Contributor Author

@mariusandra how can I make these HogQL dialogs understand the columns?

image

Copy link
Contributor

github-actions bot commented Dec 22, 2023

Size Change: 0 B

Total Size: 2 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 2 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@mariusandra
Copy link
Collaborator

mariusandra commented Dec 22, 2023

@mariusandra how can I make these HogQL dialogs understand the columns?

Oh, hmm. Those dialogs are currently hardcoded to assume we're selecting from an "events" or "persons" table. There's a hogQLTable var we pass into the taxonomicFilterLogic via the <TaxonomicFilter /> tag.

This then gets passed to the HogQLMetadata query. We then put this table in a random ast.SelectQuery together with the parsed column, and hope for the best. We need to make the other fields available in scope at parsing time for this to work.

The solution is likely to replace the hogQLTable field with a hogQLBase, which would then be a query node, either {kind:'HogQLQuery', query: 'select * from events'}, or a InsightActorsQuery, or normally just an empty EventsQuery.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

# Conflicts:
#	frontend/__snapshots__/scenes-app-experiments--complete-funnel-experiment--dark.png
@webjunkie webjunkie requested a review from mariusandra January 8, 2024 10:39
@webjunkie
Copy link
Contributor Author

webjunkie commented Jan 8, 2024

I realized that interval is the term for the rows already in the general retention result... and then you select one interval to get the appearances/returns... so the columns there should probably be named something else 🤔
I will address that.

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.

LGTM 👍. I'll have a think about the column editing. Not blocking for here though.

@webjunkie webjunkie merged commit 8c1f2f5 into master Jan 8, 2024
99 checks passed
@webjunkie webjunkie deleted the feature/retention-actor-intervals-colums branch January 8, 2024 12:59
jacobwgillespie pushed a commit to jacobwgillespie/posthog that referenced this pull request Jan 12, 2024
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.

3 participants