feat: add support for limiting queries by stored_at#3965
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughPropagates a new Unix-time filter for stored_at through query params and the ClickHouse connector into meter query SQL generation, adds a minmax index on the events table stored_at column, and updates tests to cover the new stored_at WHERE clause. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
openmeter/streaming/clickhouse/meter_query_test.go (1)
82-110: Great start—please add a few more cases for full filter coverage.The new case is good, but right now it only exercises
$lt. I’d add table cases for$gt/$gte/$lte, nested$and/$or, and an empty filter object to lock down behavior on all supported branches.As per coding guidelines "
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/streaming/clickhouse/meter_query_test.go` around lines 82 - 110, Add more unit test cases in the queryMeter table in meter_query_test.go to cover all filter branches: create additional entries similar to the "basic query with decimal stored at offset" case but exercising FilterStoredAtOffset with Lt, Gt, Gte, Lte values, a case using a nested Filter (combined $and/$or) to validate complex boolean logic, and a case with an empty FilterStoredAtOffset (or nil/empty filter object) to lock down default behavior; for each case populate queryMeter fields (Database, EventsTableName, Namespace, Meter, FilterStoredAtOffset, FilterSubject, From, To, GroupBy, WindowSize, EnableDecimalPrecision) and assert corresponding wantSQL and wantArgs to reflect the expected WHERE clauses and arguments (use the same symbols like queryMeter, FilterStoredAtOffset, FilterSubject, EnableDecimalPrecision, wantSQL, wantArgs to locate and add the cases).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/streaming/clickhouse/event_query.go`:
- Line 35: The new index is only defined during CREATE TABLE via the call to
sb.Define(fmt.Sprintf("INDEX %s_stored_at stored_at TYPE minmax GRANULARITY 4",
d.EventsTableName)), so existing tables won’t receive it; add a migration path
that runs on connector init (or upgrade) which executes an ALTER TABLE
<db>.<table> ADD INDEX IF NOT EXISTS <name> stored_at TYPE minmax GRANULARITY 4
for d.EventsTableName (or uses EventsTableName-derived index name) to ensure
existing tables get the index; implement this in the same initialization flow
that currently builds tables so the ALTER is idempotent and logged/handled on
error.
In `@openmeter/streaming/clickhouse/meter_query.go`:
- Around line 320-323: The code currently calls
d.FilterStoredAtOffset.SelectWhereExpr(...) and passes its result directly to
query.Where, which can produce an empty WHERE clause; update the block handling
FilterStoredAtOffset to first check for nil and then call an IsEmpty() (or
equivalent) on FilterStoredAtOffset before building the expression: only call
SelectWhereExpr(getColumn("stored_at"), query) and pass its result to
query.Where when the filter is non-nil and not empty, otherwise skip adding the
WHERE clause (or return/propagate a validation error as appropriate) to prevent
query.Where("") from being invoked; modify the code around FilterStoredAtOffset,
SelectWhereExpr, getColumn("stored_at"), and query.Where accordingly.
In `@pkg/filter/filter.go`:
- Around line 534-541: Nested $and/$or branches on FilterTimeUnix are delegating
to FilterTime.SelectWhereExpr, causing nested predicates to bind time.Time
instead of Unix seconds; modify the f.And and f.Or handling so the mapper calls
FilterTimeUnix.SelectWhereExpr (i.e., use each element as FilterTimeUnix when
mapping) instead of FilterTime.SelectWhereExpr, keeping the same parameters
(field, q) and preserving the lo.Map(...) and q.And/q.Or wrapping.
---
Nitpick comments:
In `@openmeter/streaming/clickhouse/meter_query_test.go`:
- Around line 82-110: Add more unit test cases in the queryMeter table in
meter_query_test.go to cover all filter branches: create additional entries
similar to the "basic query with decimal stored at offset" case but exercising
FilterStoredAtOffset with Lt, Gt, Gte, Lte values, a case using a nested Filter
(combined $and/$or) to validate complex boolean logic, and a case with an empty
FilterStoredAtOffset (or nil/empty filter object) to lock down default behavior;
for each case populate queryMeter fields (Database, EventsTableName, Namespace,
Meter, FilterStoredAtOffset, FilterSubject, From, To, GroupBy, WindowSize,
EnableDecimalPrecision) and assert corresponding wantSQL and wantArgs to reflect
the expected WHERE clauses and arguments (use the same symbols like queryMeter,
FilterStoredAtOffset, FilterSubject, EnableDecimalPrecision, wantSQL, wantArgs
to locate and add the cases).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3a6be6e-b4b0-44c7-a6fe-79ebbf7fda72
📒 Files selected for processing (6)
openmeter/streaming/clickhouse/connector.goopenmeter/streaming/clickhouse/event_query.goopenmeter/streaming/clickhouse/meter_query.goopenmeter/streaming/clickhouse/meter_query_test.goopenmeter/streaming/query_params.gopkg/filter/filter.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/streaming/clickhouse/meter_query_test.go (1)
82-112: Good new case—please add a couple of edge/operator variants too.This case is solid for the
Ltpath. I’d add a few nearby cases (e.g.,Gte, nested$and/$or, and emptyFilterStoredAt) to lock down SQL generation behavior for the full new filter surface.As per coding guidelines, "
**/*_test.go: Make sure the tests are comprehensive and cover the changes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/streaming/clickhouse/meter_query_test.go` around lines 82 - 112, Add additional test cases alongside the existing one in meter_query_test.go to cover other FilterStoredAt variants: a case using filter.FilterTimeUnix with FilterTime.Gte set (use storedAtOffset and expect stored_at >= ? in wantSQL and storedAtOffset.Unix() in wantArgs), a case with an empty/nil FilterStoredAt to ensure no stored_at predicate is emitted, and a case with nested logical filters (e.g., $and/$or) to assert correct SQL generation for combined conditions; locate the query definitions using the queryMeter struct and the FilterStoredAt field, mirror the existing pattern (Meter, From/To, GroupBy, EnableDecimalPrecision) and provide corresponding wantSQL and wantArgs for each variant to lock down SQL generation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/streaming/clickhouse/meter_query_test.go`:
- Around line 82-112: Add additional test cases alongside the existing one in
meter_query_test.go to cover other FilterStoredAt variants: a case using
filter.FilterTimeUnix with FilterTime.Gte set (use storedAtOffset and expect
stored_at >= ? in wantSQL and storedAtOffset.Unix() in wantArgs), a case with an
empty/nil FilterStoredAt to ensure no stored_at predicate is emitted, and a case
with nested logical filters (e.g., $and/$or) to assert correct SQL generation
for combined conditions; locate the query definitions using the queryMeter
struct and the FilterStoredAt field, mirror the existing pattern (Meter,
From/To, GroupBy, EnableDecimalPrecision) and provide corresponding wantSQL and
wantArgs for each variant to lock down SQL generation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e45d3707-e039-4b6c-80b3-c567395441c9
📒 Files selected for processing (6)
openmeter/streaming/clickhouse/connector.goopenmeter/streaming/clickhouse/event_query_test.goopenmeter/streaming/clickhouse/meter_query.goopenmeter/streaming/clickhouse/meter_query_test.goopenmeter/streaming/query_params.gopkg/filter/filter.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/streaming/query_params.go
- openmeter/streaming/clickhouse/connector.go
tothandras
approved these changes
Mar 18, 2026
mark-vass-konghq
pushed a commit
that referenced
this pull request
Mar 19, 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
2 participants
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.
Overview
add support for limiting meter queries by stored_at.
A skiplist index is added for faster queries. The skiplist index is minmax typed, as usually a block is written in 1-2 secs, so the stored_at field's value will be in a few seconds range for each block, thus this is quite conservative from the storage perspective.
Notes for reviewer
Summary by CodeRabbit
New Features
Performance
Tests