feat: query metadata to return index usage #29230
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added a new feature to analyze and report index usage in ClickHouse queries, helping users identify why queries might be slow by examining execution plans.
- Added
QueryIndexUsageenum with values "undecisive", "no", "partial", and "yes" to both frontend schema and backend schema.py - Implemented core logic in
posthog/clickhouse/explain.pyto analyze ClickHouse query plans and determine index usage effectiveness - Extended
HogQLMetadataResponseinterface withisUsingIndicesproperty to expose index usage information to users - Added specialized handling for specific tables like 'sharded_events' and 'person_distinct_id_overrides' with custom index usage detection
- Created comprehensive test cases in
test_explain.pycovering proper indexing, missing timestamp conditions, and complex production queries
6 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| for subplan in plan.get("Plans", []): | ||
| reads = reads + find_all_reads(subplan) |
There was a problem hiding this comment.
style: Consider using list concatenation with the += operator instead of creating a new list each time. This would be more efficient for large query plans.
| result["use"] = QueryIndexUsage.UNDECISIVE | ||
| minMax = False | ||
| partition = False | ||
| primary_key = False | ||
| for index in indexes: | ||
| if index.get("Condition", "") == "true": | ||
| continue | ||
| index_type = index.get("Type", "") | ||
| if index_type == "MinMax": | ||
| minMax = selected_less_granules(index) | ||
| elif index_type == "Partition": | ||
| partition = selected_less_granules(index) | ||
| elif index_type == "PrimaryKey": | ||
| primary_key = len(index.get("Keys", [])) > 1 and selected_less_granules(index) | ||
| if (minMax or partition) and primary_key: | ||
| result["use"] = QueryIndexUsage.YES | ||
|
|
||
| return result |
There was a problem hiding this comment.
style: This default case logic duplicates the code from the 'sharded_events' case. Consider refactoring to avoid duplication by extracting this logic into a separate function.
| try: | ||
| explain_results = sync_execute( | ||
| f"EXPLAIN PLAN indexes=1,json=1 {clickhouse_sql}", | ||
| context.values, | ||
| with_column_types=True, | ||
| # workload=workload, | ||
| team_id=team.pk, | ||
| readonly=True, | ||
| ) | ||
| response.isUsingIndices = extract_index_usage_from_plan(explain_results[0][0][0]) | ||
| except: | ||
| response.isUsingIndices = QueryIndexUsage.UNDECISIVE |
There was a problem hiding this comment.
style: The bare except block silently catches all exceptions and sets the index usage to UNDECISIVE. Consider catching specific exceptions or at least logging the error for debugging purposes.
|
Size Change: 0 B Total Size: 9.72 MB ℹ️ View Unchanged
|
Problem
No way to understand why queries are slow
Changes
Added isUsingIndices to HogQL Metadata Response.
Doing a full check is quite a complex task, what is done here is a verification that there are at least some conditions on primary keys.
Does this work well for both Cloud and self-hosted?
Yes.
How did you test this code?
Have tests. Run locally.