fix(analytics): get_analytics start_time/end_time filters returned zero rows - #751
Closed
cb1kenobi wants to merge 6 commits into
Closed
fix(analytics): get_analytics start_time/end_time filters returned zero rows#751cb1kenobi wants to merge 6 commits into
cb1kenobi wants to merge 6 commits into
Conversation
…lytics filter The hdb_analytics table stores its primary key as the compound array [timestamp, nodeId], but get_analytics was passing scalar timestamps for start_time/end_time/customWindow filters. Harper's key encoding sorts scalar numbers and arrays into different ranges, so the range iterator landed past all rows and returned zero results. The response had been hiding this by post-processing result.id down to result.id[0] before returning to the client. Wrap each bound as a single-element array so the comparison stays inside the array key space ([X] sorts before [X, anything]). Also collapsed the start && end branching to two independent ifs and switched the truthiness check to != null so a 0 timestamp is treated as valid. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Reviewed; no blockers found. |
cb1kenobi
marked this pull request as ready for review
May 23, 2026 03:37
kriszyp
reviewed
May 26, 2026
kriszyp
left a comment
Member
There was a problem hiding this comment.
I am not sure I understand the necessity of using an array key. Scalar values can be ordered alongside array values with order-binary, they are just hierarchy ordered:
1 < [1,1] < 2
And if we just doing ranges, where does the failure come from?
| if (startTime != null) { | ||
| conditions.push({ | ||
| attribute: 'id', | ||
| comparator: 'between', |
Member
There was a problem hiding this comment.
From what I recall, I had needed to use between because the query optimizer wasn't smart enough to collapse greater_than_equal and less_than into a single range estimate and would therefore prefer a different secondary index as the initial index to drive the query, leading to long scan of data for entries matching the requested timeframe.
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.
Summary
get_analyticsreturned an empty array wheneverstart_timeorend_timewas supplied.list_metrics(['custom'])was silently broken in the same way.Root cause
hdb_analyticsstores its primary keyidas the compound array[timestamp, nodeId](see resources/analytics/write.ts —storeMetric). But the read path was passing scalar timestamps as the bound forgreater_than_equal/less_than/betweenconditions onid. Harper's key encoding sorts scalar numbers and arrays into different ranges, so a scalar bound lands past the array key space and the range iterator returns nothing. The response had been hiding this by post-processingresult.id = result.id[0]before returning to the client, so users saw scalar ids in responses and naturally assumed they could filter on them directly.Fix
Wrap each bound as a single-element array — e.g.
value: [startTime]. Because[X]sorts before[X, anything]in the compound key ordering, this gives the correct inclusive lower bound and exclusive upper bound semantics against the[timestamp, nodeId]key space.While I was in there:
if (startTime && endTime)/ else branching into two independentifblocks. The both-bounds case used to go throughbetween(inclusive end); it now usesless_than(exclusive end), matching the single-bound branch. Since the old code returned zero rows in real use, no caller can have depended on the inclusive-end behavior.if (startTime)to!= nullso a0timestamp is accepted.listMetrics'customWindowcutoff condition, which was scalar against the same compound key.Where to look
between(inclusive end) to half-open[start, end)is the only semantic shift. Since the previous behavior was broken, this is a fresh choice rather than a regression — but worth a glance to confirm half-open is what we want for analytics queries.idconditions vs onebetween— I don't believesearch.tscollapses two same-attribute primary-key conditions into a single range scan, but the cost is negligible for this read path. Flagging in case there's a known reason to preferbetween.Tests
get(): no time filter,startTimeonly,endTimeonly, both bounds, andstartTime: 0(the falsy-edge fix).listMetricstests to assert the wrapped-array bound.Cross-model review
Reviewed via Gemini CLI — no blockers; the one note was a pre-existing return-type looseness on
get()(Promise<Metric[]>vs the actual async iterable whencoalesceTimeis set), which is out of scope.Generated by Claude (Opus 4.7).