Skip to content

support multi-aggregates in TS.[M][REV]RANGE#469

Open
mgravell wants to merge 12 commits intomasterfrom
marc/multi_agg
Open

support multi-aggregates in TS.[M][REV]RANGE#469
mgravell wants to merge 12 commits intomasterfrom
marc/multi_agg

Conversation

@mgravell
Copy link
Copy Markdown
Collaborator

@mgravell mgravell commented Apr 27, 2026

approach:

  1. avoid hard breaks at either runtime or build; use overloads with de-prioritization and forwarding plus hiding
  2. for inbound: introduce new TsAggregates type that behaves as a "zero, one or many" wrapper, without requiring all callers to create arrays
  3. for outbound: create new subclass of TimeSeriesTuple that overrides a new indexer to expose multiple aggregate values

Impact:

  • all code is directly compatible, zero change upgrade
  • existing callers use same path as now - no new allocs
  • folks wanting multi-aggregates will alloc some arrays, but these are constrained

En-passant:

  • tidy up the alloc story in TimeStamp

Note

Medium Risk
Touches core TimeSeries command construction and response parsing, so regressions could affect existing range queries; changes are mitigated by keeping legacy overloads and marking new multi-aggregate APIs as experimental.

Overview
Adds experimental Redis 8.8 multi-aggregate support for time-series RANGE/REVRANGE/MRANGE/MREVRANGE (sync + async) by introducing TsAggregations (zero/one/many wrapper) and new overloads that take it; the previous TsAggregation? overloads are retained but hidden/obsolete and forward into the new APIs.

Updates command arg building to serialize multiple aggregations, extends response parsing and TimeSeriesTuple with an experimental indexer plus TimeSeriesTuple.Create(...) to expose multiple per-bucket aggregate values, and refactors TimeStamp to reduce allocations for well-known string timestamps. Adds NRS002 experimental documentation/suppression wiring and expands tests (plus Docker image pin) to cover multi-aggregate behavior on Redis >= 8.8.

Reviewed by Cursor Bugbot for commit 64dddc6. Bugbot is set up for automated code reviews on this repo. Configure here.

approach:

1. avoid hard breaks at either runtime or build; use overloads with de-prioritization and forwarding plus hiding
2. for inbound: introduce new TsAggregates type that behaves as a "zero, one or many" wrapper, without requiring all callers to create arrays
3. for outbound: create new subclass of TimeSeriesTuple that overrides a new indexer to expose multiple aggregate values

Impact:

- all code is directly compatible, zero change upgrade
- existing callers use same path as now - no new allocs
- folks wanting multi-aggregates will alloc some arrays, but these are constrained

En-passant:

- tidy up the alloc story in TimeStamp
@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented Apr 27, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

Comment thread src/NRedisStack/TimeSeries/TsAggregations.cs
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 677a773. Configure here.

: TimeSeriesTuple(time, values.Span[0])
{
// this is the main point of this class: to provide access to the other values by overriding the indexer
public override double this[int index] => values.Span[index];
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MultiAggregate tuple equality ignores additional aggregate values

Low Severity

MultiAggregateTimeSeriesTuple inherits Equals and GetHashCode from the base class, which only compare Time and Val (the first aggregate value). Two multi-aggregate tuples with the same timestamp and first value but different subsequent aggregate values will be considered equal. This affects any use of these tuples in hash-based collections, Distinct(), or equality assertions like Assert.Equal on collections.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 677a773. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant