feat: topk in group by in counts api#1702
Conversation
current: counts api returns the date binned result for each distinct value for the field(s) present in group by if the field used is high cardinal, the response will be bloated Prism uses this response to build a chart that might crash in render change: add optional topk param to group by default to 10 api returns topk results for the group by field(s)
Walkthrough
Changestop_k support for grouped count queries
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/query/mod.rs (1)
1063-1078: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
top_kalias in the deserialization test.The struct explicitly accepts
top_k, but this test only lockstopKandtopk. Add one assertion for the snake_case request key.Suggested test addition
let conditions: CountConditions = serde_json::from_value(json!({ "groupBy": ["host"], "topk": 3 })) .unwrap(); assert_eq!(conditions.top_k, Some(3)); + + let conditions: CountConditions = serde_json::from_value(json!({ + "groupBy": ["host"], + "top_k": 7 + })) + .unwrap(); + assert_eq!(conditions.top_k, Some(7));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/query/mod.rs` around lines 1063 - 1078, The CountConditions deserialization test only verifies the camelCase and lowercase aliases, so add one assertion for the snake_case request key to cover the explicitly supported top_k field. Update test_count_conditions_accepts_top_k in src/query/mod.rs to deserialize a payload using top_k and assert it maps to conditions.top_k, alongside the existing topK and topk checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/query/mod.rs`:
- Around line 721-722: The top_groups subquery in the query-building logic uses
ORDER BY SUM("count") DESC LIMIT {top_k}, which can return different tied groups
across runs. Update the ordering in the top_groups SELECT to append the group
columns as a deterministic tie-breaker after the count sort, keeping the change
within the formatted SQL assembled in src/query/mod.rs near the
grouped_counts/top_groups query construction.
---
Nitpick comments:
In `@src/query/mod.rs`:
- Around line 1063-1078: The CountConditions deserialization test only verifies
the camelCase and lowercase aliases, so add one assertion for the snake_case
request key to cover the explicitly supported top_k field. Update
test_count_conditions_accepts_top_k in src/query/mod.rs to deserialize a payload
using top_k and assert it maps to conditions.top_k, alongside the existing topK
and topk checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
current: counts api returns the date binned result for each distinct value for the field(s) present in group by
if the field used is high cardinal, the response will be bloated
Prism uses this response to build a chart that might crash in render
change: add optional topk param to group by
default to 10
api returns topk results for the group by field(s)
Summary by CodeRabbit
New Features
Bug Fixes