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): groups support - part 1 #15231

Merged
merged 14 commits into from
Apr 28, 2023
Merged

feat(hogql): groups support - part 1 #15231

merged 14 commits into from
Apr 28, 2023

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Apr 25, 2023

Problem

HogQL is missing support for groups.

Changes

This is part 1 of such support. I decided to split it up into two PRs to get the easy wins out quickly, and not get stalled with support work.

  • Refactors the huge database.py into smaller files. The new changes are in groups.py and cohort_people.py
  • Adds fields goe_{index} to events to access "groups on events" properties. No support for "groups not on events" yet, see the TODO below.
  • Splits raw_groups and groups tables. The table without "raw_" contains some SQL magic to remove duplicate fields.
  • Do the same split for the "cohort_people" table because why not: it's been pending and was a small thing.

Todo in part 2

  • Create an automatic join on the events table to access "groups not on events"
  • Use short formats like "event.organization.bla" or "person.account.foo"
  • Double check conflicts and team-id overrides with these short formats
  • Make it work in non-HogQL insights

How did you test this code?

  • I tested selecting from both the groups and cohort_people tables in the UI, with some changed rows in the database. Everything worked.
  • Updated snapshots and queries in tests to show minimal changes.

@mariusandra mariusandra changed the title feat(hogql): groups support feat(hogql): groups support - part 1 Apr 26, 2023
@mariusandra mariusandra marked this pull request as ready for review April 26, 2023 07:21
@mariusandra
Copy link
Collaborator Author

Fixed the tests and this is ready for a review. Since the columns change for different tables with PoE on or off (person_id is added as a string field with PoE on), I had to lock a few select * from events tests down to just one variant.

We're changing PoE to v2 soonish anyway, and the "custom sql fields" PR will change the way fields are resolved as well, so I'm not too bothered by this right now.

Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

This seems straightforward

@Twixes
Copy link
Collaborator

Twixes commented Apr 27, 2023

Maybe one question is: should the raw versions be exposed to users?

@mariusandra mariusandra merged commit c038ffb into master Apr 28, 2023
@mariusandra mariusandra deleted the hogql-groups branch April 28, 2023 06:10
@mariusandra
Copy link
Collaborator Author

mariusandra commented Apr 28, 2023

Maybe one question is: should the raw versions be exposed to users?

Good question. They can be sometimes useful to users, so hard to say. As tables they should just work when selecting (we need to select from somewhere), but we could hide them in the database scene. They could be useful for more advanced users to debug issues though 🤔.

Edit: they could also be useful for "breaking out" of our argMax jail, e.g. to get a speed boost somewhere.

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