Repository navigation
perf(hogql): index case-insensitive table lookups in TableNode - #88182
Conversation
A lookup miss scanned every child with a lowercase comparison to serve the case-insensitive fallback. Foreign-key wiring on large schemas generates thousands of misses per compile, so the scan dominated query compile time. Cache a lowercase index per node instead, self-invalidated by children identity/length and dropped explicitly on add_child. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015JhmceYYUyY1427j1FeMFv
|
😎 Merged successfully - details. |
🤖 CI report
|
The slim setstate restores the cache rather than dropping it; the pickle memo keeps the children identity, so the restored cache is valid. The old comment claimed the opposite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8Uyj628NgticUdB6DJJ7x
|
Reviews (1): Last reviewed commit: "chore(hogql): correct the lookup-cache p..." | Re-trigger Greptile |
There was a problem hiding this comment.
Contained perf optimization to an internal HogQL query-compilation lookup, not touching schema/migrations, public APIs, auth, billing, or ingestion; I traced every place the codebase mutates the affected children dict and the cache-invalidation logic correctly handles all of them, with tests pinning the specific staleness scenarios the author identified.
- Author wrote 0% of the modified lines and has 6 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 32L, 1F substantive, 80L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (80L, 2F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 8b9a6c2 · reviewed head 8b9a6c2 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8Uyj628NgticUdB6DJJ7x
A new stamphog review started for this PR — the fresh verdict replaces this approval.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8Uyj628NgticUdB6DJJ7x
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8Uyj628NgticUdB6DJJ7x
There was a problem hiding this comment.
Internal performance optimization to HogQL's in-memory table-lookup structure, not a persisted data model, migration, API contract, or other risky surface; cache-invalidation logic is sound (verified against add_child/merge_with call sites) and covered by targeted new unit tests for the exact edge cases (staleness, collisions, opt-in matching).
- Author wrote 0% of the modified lines and has 6 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 29L, 1F substantive, 77L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (77L, 2F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 89988f3 · reviewed head 89988f3 |
|
/trunk merge |
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Internal query-compiler perf optimization (caches case-insensitive child lookups) with cache-invalidation logic verified against the real add_child/merge_with/pickle code, solid targeted tests, and two current-head human approvals plus a positive bot review with no unresolved concerns.
- Author wrote 0% of the modified lines and has 6 merged PRs in these paths (familiarity MODERATE).
- andyzzhao, georgemunyoro reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 29L, 1F substantive, 77L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (77L, 2F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ df8ec3b · reviewed head df8ec3b |
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Perf-only caching optimization for case-insensitive table lookups in HogQL's internal TableNode structure; cache invalidation correctly covers both dict-identity and length-based mutations seen elsewhere in the codebase (prune_to_table_names reassigns the dict, _apply_system_table_access deletes a key), and add_child explicitly clears the cache for same-key overrides that neither check would catch. Two current-head approvals plus solid new unit tests covering exact-match precedence, opt-in scoping, collision order, and staleness give independent assurance even though the author isn't on the owning team.
- Author wrote 0% of the modified lines and has 6 merged PRs in these paths (familiarity MODERATE).
- andyzzhao, georgemunyoro reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 29L, 1F substantive, 77L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (77L, 2F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ e638e64 · reviewed head e638e64 |
A new stamphog review started for this PR — the fresh verdict replaces this approval.
|
/trunk merge |
There was a problem hiding this comment.
The cache-invalidation logic (identity+length check plus explicit pop on add_child/merge_with) is correct on inspection and is directly exercised by new unit tests covering the exact edge cases (override, external delete, collision, exact-match precedence); the snapshot diff reflects a person-property join pattern that already exists elsewhere in the codebase, so it's unrelated pre-existing behavior being refreshed, not new behavior from this change. George Munyoro, who is most familiar with these lines, already approved, and CI is green with no unresolved concerns.
- Author wrote 0% of the modified lines and has 6 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 29L, 1F substantive, 153L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (153L, 3F, two-areas, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 6a52a9d · reviewed head 6a52a9d |
The branch carried a query snapshot (products/product_analytics/backend/tests/api/__snapshots__/test_insight.ambr) from an earlier tests-posthog[bot] auto-update commit that predates master's #88182 (perf(hogql): index case-insensitive table lookups in TableNode). That snapshot is unrelated to this PR's egress work and was stale relative to current master, causing spurious Django test failures in the product_analytics shard. Reverting it to match master's current content removes it from this PR's diff entirely.
The branch carried a query snapshot (products/product_analytics/backend/tests/api/__snapshots__/test_insight.ambr) from an earlier tests-posthog[bot] auto-update commit that predates master's #88182 (perf(hogql): index case-insensitive table lookups in TableNode). That snapshot is unrelated to this PR's egress work and was stale relative to current master, causing spurious Django test failures in the product_analytics shard. Reverting it to match master's current content removes it from this PR's diff entirely.
The branch carried a query snapshot (products/product_analytics/backend/tests/api/__snapshots__/test_insight.ambr) from an earlier tests-posthog[bot] auto-update commit that predates master's #88182 (perf(hogql): index case-insensitive table lookups in TableNode). That snapshot is unrelated to this PR's egress work and was stale relative to current master, causing spurious Django test failures in the product_analytics shard. Reverting it to match master's current content removes it from this PR's diff entirely.
Scratch republish to drop a stale product_analytics snapshot that leaked in from an earlier tests-posthog[bot] auto-update commit and no longer matches master (post #88182). This carries only this PR's egress changes.
Problem
Fresh insight queries on teams with large warehouse schemas spend seconds inside HogQL compilation before ClickHouse sees any SQL. Profiling one trends compile on our internal project put ~2.5 s of that inside a single method,
TableNode._match_child.The cause is the fallback path for case-insensitive lookups. When an exact name lookup misses,
_match_childscans every child of the node and lowercases each key looking for a case-insensitive match. A linear scan would be fine if misses were rare, but foreign-key wiring for Postgres-backed warehouse tables probes thousands of candidate names per compile, and nearly all of those probes miss. On the profiled compile that meant ~16,500 misses scanning ~7,000 children each: about 115 million string comparisons that produced nothing.Changes
_match_childnow answers misses from a lazily built index: a dict of lowercased name to child, containing only the children that opt into case-insensitive matching. A single-series trends compile on the same project drops from 4.4 s to 1.8 s. Nothing changes for users except speed.Lookup semantics are identical: an exact match still wins, only opted-in children match case-insensitively, and on a collision the first child in iteration order still wins (the index is built with
setdefault, mirroring the scan it replaces).The substance of the diff is invalidation and storage, not the index itself. The cache validates against the identity and length of
children, which covers reassignment, additions, and deletions. The one mutation that check cannot see is a same-name override, soadd_childdrops the cache explicitly, andmerge_withfunnels every write throughadd_child. The cache lives in the instance__dict__rather than a pydantic PrivateAttr, because any private state would take every node off the_slim_pickle_getstatefast path that the static-catalog pickle depends on.How did you test this code?
New
TestTableNodeCaseInsensitiveLookupunit tests (no DB) pin the regressions the cache could introduce: staleness after anadd_childoverride or an externaldel, matching a child that did not opt in, changing the collision winner, and an exact match losing to the index. The Snowflake case-insensitive resolution suites exercise the path end to end in CI. The before and after timings come from cProfile runs of one compile against a production-sized schema.Not run locally: the DB-backed suites, because the authoring sandbox has no Postgres. CI covers them.
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with Claude Code during a profiling investigation into slow query compiles on large warehouse schemas. Skills invoked: /writing-tests, /writing-pr-descriptions, /writing-code-comments. The cache moved from the initially planned pydantic PrivateAttr to a plain
__dict__entry after finding that any private state defeats the slim-pickle fast path inmodels.py.