feat: Added lazy joins for groups on events #17950
Conversation
There was a problem hiding this comment.
Very cool. Though an extra 👨🍳😗 would to make this work too:
The information is stored in the GroupTypeMapping postgres model. It shouldn't be hard to query that in the create_database method and make some changes unless someone wants to add a group called "team_id" or any of the existing fields.
I just have a feeling users would love to be able to specify .organization instead of wondering which group index it was.
| @@ -85,6 +86,16 @@ class EventsTable(Table): | |||
| # These are swapped out if the user has PoE enabled | |||
| "person": FieldTraverser(chain=["pdi", "person"]), | |||
| "person_id": FieldTraverser(chain=["pdi", "person_id"]), | |||
| "$group_0": StringDatabaseField(name="$group_0"), | |||
There was a problem hiding this comment.
This field is actually the same as properties.$group_0. The $group_0 columns are materialized columns, calculated directly from the property. If we specify properties.$group_0, it'll resolve to the column itself. Other materialized properties start with mat_, but the group ones are "top level" for some reason. I'm not sure if it makes sense to try linking to the property instead of adding a column or not. Fine to keep as is.
There was a problem hiding this comment.
but the group ones are "top level" for some reason.
That's because these are explicitly generated and expected to exist in every installation: https://github.com/PostHog/posthog/blob/master/posthog/models/event/sql.py#L59
whereas the ones starting with mat_ are autogenerated/materialised/not necessarily always there, which is what prompts the sanity check that you shouldn't be explicitly querying for a mat_XYZ property unless you're very sure it will exist.
or so I think was the intent at the time 😅
|
One more thought. Group analytics is an optional upgrade for PostHog. We probably should hide these fields altogether if it's turned off 🤔 |
@mariusandra Hmm, is it turned off even when breaking down trends by group fields? |
Gilbert09
force-pushed
the
feat/hogql-groups-on-events
branch
from
0846c4d
to
bb68ed3
Compare
| for mapping in GroupTypeMapping.objects.filter(team=team): | ||
| database.events.fields[mapping.group_type] = FieldTraverser(chain=[f"group_{mapping.group_type_index}"]) |
There was a problem hiding this comment.
One change I'd do is to double check if the field name is not already taken. Otherwise users might, accidentally or deliberately, override "system level" fields like "timestamp" or "distinct_id" and cause all manner of problems.
There was a problem hiding this comment.
Yeep - that's fair. Where do people define the group mappings? Somewhere in the settings?
There was a problem hiding this comment.
Yep, under project settings:
* Added lazy joins for groups on events * Update query snapshots * Fixed test_resolver.py tests * Added aliases for groups from group mappings --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Problem
groupproperties when dealing with trend breakdownsChanges
LazyJoin's for each of the$group_nfields oneventsHow did you test this code?