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): Use distinct_id-based person overrides for PERSON_ID_OVERRIDE_PROPERTIES_JOINED mode #21520

Merged
merged 12 commits into from
Apr 16, 2024

Conversation

tkaemming
Copy link
Contributor

@tkaemming tkaemming commented Apr 12, 2024

Problem

#21504 did not implement reading from the new person_distinct_id_overrides table as the source for overrides, but that is the table we eventually want to use for HogQL queries as it has all of the data necessary to be transparently substitutable with the equivalent queries that do joins on the full person_distinct_id2 table.

Changes

This updates the PERSON_ID_OVERRIDE_PROPERTIES_JOINED mode to use the new person_distinct_id_overrides table. This change should be safe to make for this version, since nobody was previously opted into this mode. The data returned by this mode should be generally the same as the data returned when not opted into any particular mode, so it should be safe to move teams in and out of this mode without any noticeable impact beyond improved performance.

This leaves PERSON_ID_OVERRIDE_PROPERTIES_ON_EVENTS using the old table, and we can determine the upgrade path for those teams independently.

This will result in support for legacy and HogQL queries for the modes introduced in #21504 as shown:

Mode Overrides Legacy HogQL
DISABLED N/A Yes Yes
PERSON_ID_NO_OVERRIDE_PROPERTIES_ON_EVENTS N/A Yes Yes
PERSON_ID_OVERRIDE_PROPERTIES_ON_EVENTS person_overrides (v2) Yes Yes
PERSON_ID_OVERRIDE_PROPERTIES_JOINED person_distinct_id_overrides (v3) No Yes

Does this work well for both Cloud and self-hosted?

Yes, this nobody should be using the override query modes.

How did you test this code?

Updated tests, also manually verified some queries via /debug.

This comment was marked as off-topic.

Copy link
Contributor

github-actions bot commented Apr 12, 2024

Size Change: +155 kB (+18%) ⚠️

Total Size: 999 kB

Filename Size Change
frontend/dist/toolbar.js 999 kB +155 kB (+18%) ⚠️

compressed-size-action

@tkaemming tkaemming marked this pull request as ready for review April 12, 2024 23:31
@tkaemming tkaemming requested review from timgl and a team April 12, 2024 23:32
@posthog-bot

This comment was marked as outdated.

@posthog-bot

This comment was marked as outdated.

@tkaemming tkaemming marked this pull request as draft April 15, 2024 17:53
@tkaemming

This comment was marked as resolved.

@tkaemming tkaemming changed the title feat(hogql): Bring back support for distinct_id-based person overrides feat(hogql): Use distinct_id-based person overrides for PERSON_ID_OVERRIDE_PROPERTIES_JOINED mode Apr 16, 2024
@tkaemming tkaemming marked this pull request as ready for review April 16, 2024 17:09
@tkaemming tkaemming merged commit cb5f851 into master Apr 16, 2024
106 checks passed
@tkaemming tkaemming deleted the hogql-persons-on-events-v3-overrides branch April 16, 2024 18:18
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