Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8f4c460a2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5bd49fe67
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| args.AddFilterByTs(filterByTs); | ||
| args.AddFilterByValue(filterByValue); | ||
| args.AddCount(count); | ||
| if (aggregations is { Count: > 0 }) |
There was a problem hiding this comment.
When callers pass aggregation-only options such as flags: TimeSeriesRangeFlags.Empty, align, timeBucket, or bt without a non-empty aggregations list, this guard skips the whole block and silently drops those arguments, so the server receives a raw TS.NRANGE/TS.NREVRANGE and returns unaggregated rows instead of either applying the requested option or reporting an invalid combination. This is especially easy to hit with Empty or timeBucket because the public API exposes them independently; mirror the range builders by throwing when these options are present without aggregation, or otherwise make the server see the invalid option.
Useful? React with 👍 / 👎.
| TsBucketTimestamps? bt) | ||
| { | ||
| var args = new List<object>(keys.Count + 4) { keys.Count }; | ||
| foreach (var key in keys) args.Add(key); |
There was a problem hiding this comment.
When TS.NRANGE/TS.NREVRANGE is used through StackExchange.Redis against a Redis Cluster, these key operands are added as plain strings, so the raw Execute call cannot identify them as keys for slot routing; same-slot keys that live on another shard can be sent to the wrong node instead of being routed like TS.RANGE, which casts its key to RedisKey. Cast each key here to RedisKey before adding it to the command arguments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0783962ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ((flags & TimeSeriesRangeFlags.WithLabels) != 0) args.Add(TimeSeriesArgs.WITHLABELS); | ||
| if ((flags & TimeSeriesRangeFlags.ExcludeEmpty) != 0) args.Add(TimeSeriesArgs.EXCLUDEEMPTY); |
There was a problem hiding this comment.
When callers pass TimeSeriesRangeFlags.WithLabels or ExcludeEmpty to NRange/NRevRange, these branches serialize options that TS.NRANGE/TS.NREVRANGE do not support and that the pivot-row return type cannot represent. In that scenario the client sends stray WITHLABELS/EXCLUDEEMPTY tokens before the range modifiers, so the call either fails server-side or silently does not honor the requested flag; reject these flags for NRANGE instead of emitting them.
Useful? React with 👍 / 👎.
| [Experimental(Experiments.Server_8_10, UrlFormat = Experiments.UrlFormat)] | ||
| public static SerializedCommand Read(string key, TimeStamp timestamp, long? maxCount = null) | ||
| { | ||
| var args = new List<object> { key, timestamp.Value }; |
There was a problem hiding this comment.
When TS.READ is used through StackExchange.Redis against a Redis Cluster, this first operand is added as a plain string, so the raw Execute call has no RedisKey argument to hash for slot routing. Reads for keys that live on another primary can be sent to the wrong node or rely on redirects instead of the intended route; add the key as (RedisKey)key like other routed key operands.
Useful? React with 👍 / 👎.
| (long, long)? filterByValue = null, | ||
| long? count = null, | ||
| TimeStamp? align = null, | ||
| IReadOnlyList<TsAggregation>? aggregations = null, |
There was a problem hiding this comment.
When a TS.NRANGE call needs more than one aggregation for a single key (for example avg,max for one series and sum for another), this IReadOnlyList<TsAggregation> API can only emit one enum token per list entry. Passing both aggregations for one key increases the token count and is interpreted by the server as aggregators for additional keys, so valid 8.10 NRANGE/NREVRANGE requests are rejected instead of being expressible; accept TsAggregations/comma specs per key and parse the widened value layout.
Useful? React with 👍 / 👎.
…ggregates` via implicit conversion)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12d36feded
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Note: the server's BLOCK group is intentionally not exposed - blocking does not compose with the | ||
| // SE.Redis multiplexer - so this only ever builds the immediate-return form. | ||
| [Experimental(Experiments.Server_8_10, UrlFormat = Experiments.UrlFormat)] | ||
| public static SerializedCommand Read(string key, TimeStamp timestamp, long? maxCount = null) |
There was a problem hiding this comment.
$ cursor
When callers need TS.READ's server-side tail cursor ($, start after the current latest sample), this API cannot represent it: Read only accepts TimeStamp, and TimeStamp(string) rejects anything except -, +, and *. That forces users who want the documented “new samples only” cursor to drop to raw Execute or start from + and receive the current latest sample again; add $ support to TimeStamp or expose a string/RedisValue overload for this command.
Useful? React with 👍 / 👎.
| TimeStamp toTimeStamp, | ||
| TimeSeriesRangeFlags flags = TimeSeriesRangeFlags.None, | ||
| IReadOnlyCollection<TimeStamp>? filterByTs = null, | ||
| (long, long)? filterByValue = null, |
There was a problem hiding this comment.
When a time-series stores non-integer values, TS.NRANGE/TS.NREVRANGE can filter by double min/max bounds, but the new public surface fixes filterByValue to (long, long). Callers therefore cannot express ranges such as (0.25, 0.75) and must either widen to integer limits, returning incorrect rows, or bypass the API; use a floating-point tuple here and in the builder/async overloads before shipping this new surface.
Useful? React with 👍 / 👎.
# Conflicts: # Directory.Build.props # src/NRedisStack/Experiments.cs # src/NRedisStack/PublicAPI/PublicAPI.Unshipped.txt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7480433a62
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| bool latest = false, | ||
| IReadOnlyCollection<TimeStamp>? filterByTs = null, | ||
| (long, long)? filterByValue = null, | ||
| (double, double)? filterByValue = null, |
There was a problem hiding this comment.
Changing this shipped TsAggregations overload from (long, long)? to (double, double)? removes the exact metadata signature that existing binaries bind to; because the commit only keeps (long, long)? forwarders for the older TsAggregation? overloads, applications compiled against the previous Range/MRange/async/builder TsAggregations signatures can fail after a package upgrade with MissingMethodException instead of getting the widened behavior. Please keep obsolete (long, long)? overloads for the shipped TsAggregations forms and delegate them to the new double implementation.
Useful? React with 👍 / 👎.
| // one comma-joined aggregator group per key (group count must equal numkeys, enforced server-side); | ||
| // each group may itself hold multiple aggregators, producing multiple value columns for that key. | ||
| foreach (var group in aggregations) args.Add(GetAggregationArgs(group)); | ||
| if (timeBucket.HasValue) args.Add(timeBucket.Value); |
There was a problem hiding this comment.
When callers pass any aggregations list but leave timeBucket at its default, this branch emits AGGREGATION and the per-key aggregators but skips the required bucket-duration operand. That serializes a malformed TS.NRANGE/TS.NREVRANGE such as ... AGGREGATION avg max, so the first aggregation attempt that relies on the default fails with a server arity/protocol error; mirror TS.RANGE by throwing locally or require callers to supply the bucket value before emitting the aggregation clause.
Useful? React with 👍 / 👎.
1 participant
in this PR:
[Flags] enumto subsume the multipleboolargsQueryLabelNames[Async]andQueryLabelValues[Async]TsAggregationsoverTsAggregation; can describe single-aggregations just fineRedisKey) errors, and ensure that the CI runs on standalone and cluster, at least for cluster 8+ - this also involved fixing some test expectations around ordering on non-key-routed commandsNote
Medium Risk
Shipped public API changes
(long,long)?to(double,double)?forfilterByValuecan break existing callers; large new experimental surface depends on a young Redis 8.10 server API, plus cluster routing and test-matrix behavior changes warrant careful review.Overview
Adds Redis 8.10 time-series client support behind the NRS003 experimental diagnostic (
docs/exp/NRS003.md, repo-wideNoWarnfor library builds).New server bindings (sync/async + command builder):
QueryLabelNames/QueryLabelValues(TS.QUERYLABELS), pivotNRange/NRevRangereturningTimeSeriesPivotRow, and forwardReadplus pagedReadEnumerable/ReadAsyncEnumerable(TS.READwithoutBLOCK).MRANGE / MREVRANGE gain overloads that take
TimeSeriesRangeFlags(replacing separatelatest/withLabels/emptybooleans for the new path), includingExcludeEmpty.FILTER_BY_VALUEis wired as(double, double)?across range APIs (public signature change from(long, long)?).Infrastructure fixes: TS command args use
RedisKeyfor correct cluster routing; tests run on cluster 8+ with hash-tagged keys and order-insensitive assertions for fan-out commands;SkipIfRedisnormalizes 2-part vs 3-part version strings (e.g.8.10vs8.10.0).Reviewed by Cursor Bugbot for commit 7480433. Bugbot is set up for automated code reviews on this repo. Configure here.