fix(surveys): compute survey stats through hogql - #69762
Conversation
andyzzhao
added
the
skip-agent-review
andyzzhao
added
the
skip-agent-review
|
Reviews (1): Last reviewed commit: "fix(surveys): compute survey stats throu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Multiple partial "survey sent" events can exist per submission; only the latest per | ||
| # $survey_submission_id counts (pre-submission-id events group by their own uuid). Deliberately | ||
| # not filtered by survey, matching get_unique_survey_event_uuids_sql_subquery's semantics. | ||
| sent_dedup_sql = f"""(event != {{sent}} OR uuid IN ( | ||
| SELECT argMax(uuid, timestamp) | ||
| FROM events | ||
| WHERE event = {{sent}}{date_conditions} | ||
| GROUP BY if( | ||
| coalesce(properties.$survey_submission_id, '') = '', | ||
| toString(uuid), | ||
| properties.$survey_submission_id | ||
| ) | ||
| ))""" |
There was a problem hiding this comment.
get_unique_survey_event_uuids_sql_subquery semantics
The comment claims this subquery matches get_unique_survey_event_uuids_sql_subquery's semantics (no survey filter inside the subquery). However, get_unique_survey_event_uuids_sql_subquery always appends the survey_id property expression to the GROUP BY clause (see util.py lines 134–136), so the original dedup was always scoped per survey. The new subquery groups purely by $survey_submission_id across all surveys for the team. In practice this is harmless because submission IDs are per-submission UUIDs and won't collide across surveys, but the comment is factually incorrect and could mislead future maintainers into thinking no behavioral change occurred.
044f339 to
d237797
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Query-engine swap (raw SQL to HogQL) for survey stats with disclosed test coverage and a clearly flagged, intentional semantic delta (person-id dedup); the one outstanding reviewer comment is a doc-accuracy nit that greptile itself assessed as functionally harmless, not a correctness issue.
- greptile-apps[bot] reviewed the current head.
- Greptile flagged that the new dedup subquery's comment misdescribes get_unique_survey_event_uuids_sql_subquery's semantics (global vs per-survey grouping) — greptile deems the behavioral difference harmless in practice, but the comment should be corrected to avoid misleading future maintainers.
- Author (andyzzhao) is not on the owning @PostHog/team-surveys, though this is a routing signal only since the change is a tested internal query refactor, not risky-territory (no schema/data-model/API-contract/auth/billing change).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 156L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (156L, 1F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 07a0e0f · reviewed head 044f339 |
get_survey_stats built raw ClickHouse SQL, which silently pins it to the legacy events table: it would keep reading a frozen table after any events-schema cutover, and every schema change has to update it by hand. Expressed in HogQL, table selection and property access follow the query engine, and the queries participate in the dual-schema test matrix. Behavior is deliberately unchanged: person id resolution is pinned to the bare events column via PERSON_ID_NO_OVERRIDE_PROPERTIES_ON_EVENTS, matching the raw SQL this replaces. Override-corrected unique-person counts remain available as a one-line follow-up if surveys wants them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d237797 to
a4b0a81
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Raw-SQL-to-HogQL refactor of survey stats with claimed behavior parity, backed by passing tests (0 failed in CI) and a Greptile review that specifically probed the one semantic gap in the dedup subquery and concluded it's practically harmless (submission IDs are unique UUIDs, so the missing survey-scoped GROUP BY doesn't change real-world counts) — I verified this against the actual util.py implementation and agree.
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
- Greptile's inline comment on stats.py remains unresolved: the code comment claims the dedup subquery mirrors get_unique_survey_event_uuids_sql_subquery's semantics, but that helper always adds survey_id to the GROUP BY while the new subquery doesn't. Behaviorally inconsequential (submission IDs are unique per-survey UUIDs) but the comment should be corrected for future maintainers.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 162L, 1F substantive — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (162L, 1F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 081faff · reviewed head a4b0a81 |
|
Benchmarked the compiled HogQL against the raw SQL it replaces on prod US, using the heaviest survey in the region (team with ~4M survey events over 30d), 3 interleaved runs per arm with
Base stats is within noise (+1%), overlap count is ~15% faster with less memory. The same runs double as a production-data parity check: both arms returned identical results over 4,011,842 events (counts, unique persons, and first/last-seen timestamps to the millisecond). |
|
Extended the production parity check to the code paths the first benchmark's survey didn't exercise, using a second survey on the same team (1,655 dismissals incl. 383 partially-completed, 269 sent) plus 5 real event uuids as an archived-exclusion list:
Every branch (single-survey, global, archived exclusion, partial-completion filter, submission dedup, zero and nonzero overlap) now has old-vs-new parity on production data, with the filter arithmetic confirming each clause did real work rather than matching nothing. |
1 participant
Problem
get_survey_stats(the survey performance numbers behind both the surveys API and the dashboard survey widget) built raw ClickHouse SQL, making it an undocumented pin to the legacy events table: after any events-schema cutover it would silently keep reading a frozen table, and until then every schema change must update it by hand — #63448 in fact missed it entirely. Its two sibling modules inresponses/are already HogQL; this brings the last raw-SQL holdout in the package in line, so table selection and property access follow the engine and the queries run under the dual-schema CI matrix.Changes
execute_hogql_querycalls with taggedquery_types; team scoping, property extraction, and the partial-response dedup subquery are expressed in HogQL. The dedup deliberately mirrorsget_unique_survey_event_uuids_sql_subquery's semantics (no survey filter inside the subquery).PERSON_ID_NO_OVERRIDE_PROPERTIES_ON_EVENTS), exactly matching the raw SQL this replaces — no override join, no count changes, no added query weight. Switching to override-corrected unique-person counts (merged persons counting once) is left as a deliberate one-line product decision for the surveys team.partial_responses_filter/archived_responses_filterhelpers stay:api/survey.pystill embeds them in a separate raw query (out of scope here, noted below).first_seen/last_seenkeep their exact wire format.How did you test this code?
TestSurveyStats(partial-response dedup, archived-response exclusion, global stats, date clamping) plus the dashboard survey-widget suite: 26/26 passing locally against the dev stack, re-run after the person-id parity pin. A field-by-field equivalence audit of old vs new SQL (property scrubbing, bool coercion cases, dedup grouping, LIMIT applicability, timestamp constant printing) is summarized in the commit and was reviewed clause by clause.Automatic notifications
Docs update
Not needed, internal query change.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
per_question_stats.py,fetch_rows.py), including constant-list uuid exclusion.api/survey.py(~line 2280) embeds the same filter helpers in another raw events query — the remaining raw reader in the surveys product.🤖 Generated with Claude Code