fix: add hogql anti-pattern rules to mcp sql guidance - #70536
Merged
Conversation
The execute-sql querying guidance never told MCP clients how property access actually works: properties.foo is resolved by the HogQL printer to a materialized column when one exists (up to ~100x cheaper) and falls back to JSON extraction itself when none does, so hand-written JSONExtract*(properties, ...) can only tie or lose. An online evaluation on execute-sql generations measures the hand-written form at roughly 2% of MCP-authored queries. Adds the dot-access rule (with typed-read casts and a carve-out for legitimate JSONExtract on non-properties JSON columns) to the property access section and the performance rules of guidelines.md, which feeds both the querying-posthog-data skill and the execute-sql tool description. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHNqSqPGjvFfgv6ahQn7hQ
Extends the querying guidance with the remaining measured anti-patterns from the execute-sql online evaluations: - Search types: substring LIKE/ILIKE on events strings is a full scan; discover real values via read-data-schema and use equality or an anchored prefix (largest failure mode, ~20% of MCP-authored queries). - Time ranges: the bound must be a WHERE predicate on timestamp (aggregate-argument conditions filter nothing), and id-anchored point lookups still need one (dominant class of judge failures). - JOINs: scan events once with conditional aggregates instead of self-joins, repeated same-range subqueries, or twice-referenced CTEs. - Other rules: uniq(person_id) not uniq(distinct_id); avoid unbounded high-cardinality GROUP BY over wide windows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHNqSqPGjvFfgv6ahQn7hQ
Benchmarked both on the Test Cluster: uniq(person_id) is a correctness rule (distinct_id overcounts users; perf roughly neutral since person resolution adds the overrides join), and the high-cardinality GROUP BY rule pays on the memory axis (6x peak memory on identical scans), not latency. Label them accordingly so the performance framing stays honest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHNqSqPGjvFfgv6ahQn7hQ
andyzzhao
marked this pull request as ready for review
July 13, 2026 20:57
Contributor
|
Reviews (1): Last reviewed commit: "fix(phai): label person_id rule correctn..." | Re-trigger Greptile |
andyzzhao
marked this pull request as draft
July 13, 2026 22:03
The HogQL engine already rewrites JSONExtractString(properties, 'x') to the materialized column when one exists, the never-rescued variants measure at only ~1% of MCP-authored queries, and the remaining forms (Raw on nested JSON, key listing, existence checks) have legitimate uses that a blanket rule would suppress. Keep the guidance focused on the patterns with the largest measured cost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHNqSqPGjvFfgv6ahQn7hQ
andyzzhao
marked this pull request as ready for review
July 13, 2026 22:41
Contributor
|
Reviews (2): Last reviewed commit: "fix(phai): drop the jsonextract dot-acce..." | Re-trigger Greptile |
…idelines.md Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
MattPua
approved these changes
Jul 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
LLMs can write poor-performing HogQL. I created an LLM judge evaluation to find anti-patterns in LLM-written HogQL queries.
Changes
products/posthog_ai/skills/querying-posthog-data/references/guidelines.md. It's bundled into the MCP server, so the rules land in both theexecute-sqltool description and the querying skill.LIKE '%term%'. Look up the real names withread-data-schemaand match them exactly. A wildcard search reads every row in the time range; an exact match lets ClickHouse skip most of the table.countIf(...)doesn't count, and a selective-looking id filter (session, trace, user) doesn't either: without a time filter the whole history gets scanned.countIfinstead of self-joins or repeating the same subquery. AWITHsubquery used twice runs twice.uniq(person_id), notuniq(distinct_id). This one is about correctness than speedLIMITdoesn't help.Left alone on purpose: the
uniq(distinct_id)examples inmodels-mcp.md, which mirror backend code and belong to the MCP analytics team.How did you test this code?
Benchmarked the two biggest rewrites on our test ClickHouse cluster:
After this merges, the same evaluations that produced the fail rates above tell us whether the rules actually changed client behavior.
Automatic notifications
Docs update
Not needed: this PR is itself the guidance change.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written by Claude Code, directed by the assignee, as part of a quality loop: capture every MCP SQL query with its stated intent, score them with online evaluations, fix the guidance where the scores show problems, then watch the rates move. The rules come from the internal
optimizing-clickhouse-and-hogql-queriesskill plus the benchmarks above. Started as a single JSONExtract rule, expanded to the full measured set, then the JSONExtract rule was dropped on the assignee's direction after we confirmed the HogQL engine already rewrites the common form to the materialized column. No repo skills were invoked for the edits themselves (markdown-only change).