Repository navigation
Support WITHCOUNT / WITHOUTCOUNT on FT.SEARCH and FT.AGGREGATE - #592
aradalvand wants to merge 3 commits into
Conversation
`Query` gains a `bool? WithCount` property, set with `SetWithCount()` / `SetWithoutCount()`; `AggregationRequest` gains `WithCount()` / `WithoutCount()`. `WITHOUTCOUNT` turns on the query optimizer, which lets a query sorted by a numeric field stop once it has enough results; `WITHCOUNT` keeps the total exact. The server only accepts the option before the first pipeline step of FT.AGGREGATE, so `AggregationRequest` places it right after the query whatever the call order, and a later call replaces an earlier one. Closes redis#591. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XUNs4unCWEtK5q25xYbvgz
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
mgravell
left a comment
There was a problem hiding this comment.
Thanks — the feature itself looks right, and the tests are solid. A few points, mostly on API consistency between the two new surfaces (details inline):
SetCountOptionwrites toargs[1]on replacement, which can clobber a caller-inserted token sinceGetArgs()exposes the live list (inline, with a suggested field-based approach).Query.SetWithCount()/SetWithoutCount()depart from the file'sSet<Flag>(bool value = true)fluent-setter convention.- On
AggregationRequestthe count option is irreversible, while onQueryWithCount = nullrestores the server default — the two surfaces disagree on whether the tri-state is expressible. - The new integration tests lack the
Is.Enterpriseskip that neighbouring aggregation tests carry. - Minor: the
<seealso>doc links are present on half of the four new entry points.
The suggested fix for 1 resolves 3 as well.
| // The query is always the first argument, and every other option or step is appended after it. | ||
| if (hasCountOption) | ||
| { | ||
| args[1] = option; |
There was a problem hiding this comment.
This relies on the count token still being at index 1, but GetArgs() hands out the live internal list, so a caller can invalidate that invariant:
req.WithCount();
req.GetArgs().Insert(1, "VERBATIM"); // or any other direct manipulation
req.WithoutCount();
// args: [query, WITHOUTCOUNT, WITHCOUNT, ...]The caller's token is silently overwritten and the command carries both WITHCOUNT and WITHOUTCOUNT. Rather than a positional special case on an otherwise append-only builder, consider storing the option in a field the way dialect is handled, and composing it into the args at serialization time — that also removes the hasCountOption bookkeeping (and fixes the irreversibility point in the other comment).
There was a problem hiding this comment.
Done in 716dfe9. The option now lives in a bool? withCount field and is inserted after the query in SerializeRedisArgs, next to Dialect(). hasCountOption and the positional overwrite are gone. AggregationCountOptionLeavesCallerInsertedArgsIntact reproduces your example: WithCount(), insert VERBATIM at 1, WithCount(false) now yields [*, WITHOUTCOUNT, VERBATIM].
| /// </remarks> | ||
| public AggregationRequest WithoutCount() => SetCountOption(SearchArgs.WITHOUTCOUNT); | ||
|
|
||
| private AggregationRequest SetCountOption(string option) |
There was a problem hiding this comment.
API-consistency note: on Query, assigning WithCount = null restores the server-default "send neither" state, but here once WithCount()/WithoutCount() has been called there is no way back — the two surfaces introduced in this PR disagree on whether the tri-state is fully expressible. The field-based design suggested above makes clearing trivial.
There was a problem hiding this comment.
Fixed by the same change: WithCount(null) clears the option, so AggregationRequest has the same three states as Query (AggregationCountOptionCanBeCleared).
| /// Set the query to count every result (<c>WITHCOUNT</c>), so that the total is exact. | ||
| /// </summary> | ||
| /// <returns>the query itself</returns> | ||
| public Query SetWithCount() |
There was a problem hiding this comment.
The established convention for flag setters in this file is Set<Flag>(bool value = true) — SetVerbatim, SetNoContent, SetNoStopwords, SetWithScores all follow it, which allows conditional building like q.SetWithCount(userWantsExact). The new pair can't do that, and returning to the server default means leaving the fluent chain to assign q.WithCount = null. Since the underlying property is tri-state, a single SetWithCount(bool? value = true) would cover all three states and match the file's pattern.
There was a problem hiding this comment.
Agreed. SetWithCount()/SetWithoutCount() became a single SetWithCount(bool? value = true), so q.SetWithCount(userWantsExact) works and null restores the server default inside the chain. For symmetry, AggregationRequest likewise has a single WithCount(bool? value = true) in place of WithCount()/WithoutCount().
| Assert.Equal(0, res.TotalResults); | ||
| } | ||
|
|
||
| [SkipIfRedisTheory(Comparison.LessThan, "7.2.0")] |
There was a problem hiding this comment.
Neighbouring aggregation tests in this file also carry [SkipIfRedisTheory(Is.Enterprise)]. The 7.2.0 gate is on the core version, but Enterprise builds' bundled RediSearch versions aren't tied to it — an Enterprise endpoint reporting core ≥ 7.2.0 with RediSearch < 2.8.3 (WITHCOUNT arrived in 2.8.3) would run this and fail with SEARCH_ARG_UNRECOGNIZED instead of skipping. Worth adding the Enterprise skip to the two new tests.
There was a problem hiding this comment.
Good catch. Both tests now use [SkipIfRedisTheory(Is.Enterprise, Comparison.LessThan, "7.2.0")].
| /// The server only accepts the option before the first pipeline step, so it is sent right after the | ||
| /// query wherever in the chain this is called; a later <see cref="WithoutCount"/> replaces it. | ||
| /// </remarks> | ||
| public AggregationRequest WithCount() => SetCountOption(SearchArgs.WITHCOUNT); |
There was a problem hiding this comment.
Minor: WithoutCount() below has <seealso href="https://redis.io/docs/latest/commands/ft.aggregate/"/> but WithCount() doesn't; same asymmetry on Query, where the WithCount property links to the ft.search page but SetWithCount()/SetWithoutCount() don't. Adding the link to the members missing it keeps IntelliSense consistent across the four entry points.
There was a problem hiding this comment.
With one setter per surface there are three entry points now, and each links its command page: Query.WithCount and Query.SetWithCount to ft.search, AggregationRequest.WithCount to ft.aggregate.
Address review feedback on redis#592: - `AggregationRequest` keeps the option in a field and inserts it after the query when serializing, like `dialect`, instead of overwriting `args[1]`. A token a caller inserted through `GetArgs()` is no longer clobbered, and the option can be cleared again. - Both surfaces take one `bool?` setter: `Query.SetWithCount(bool? value = true)`, following the file's `Set<Flag>(bool value = true)` convention, and `AggregationRequest.WithCount(bool? value = true)`. `false` sends `WITHOUTCOUNT` and `null` sends neither. - The integration tests skip Enterprise like their neighbours. - Every entry point links the command's docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XUNs4unCWEtK5q25xYbvgz
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 716dfe953b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (withCount.HasValue) | ||
| { | ||
| // The query is always the first argument, and the server rejects the option after a pipeline step. | ||
| args.Insert(1, withCount.Value ? SearchArgs.WITHCOUNT : SearchArgs.WITHOUTCOUNT); |
There was a problem hiding this comment.
Replace stale count options when reusing an aggregation request
When an AggregationRequest is reused, every Aggregate or ProfileAggregate build calls SerializeRedisArgs() again, but this insertion modifies the persistent argument list without removing the previous count option. Building a request with WithCount(false), then calling WithCount(null) and rebuilding it still sends WITHOUTCOUNT; switching to WithCount() instead emits both flags. Consequently, the documented replacement/reset behavior stops working after the first build, and clearing the option cannot restore server defaults. Serialize the count option into a temporary argument list or replace/remove the previously emitted token when rebuilding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, thanks. Fixed in 51df5ba: the request no longer inserts the token into its own argument list. SearchCommandBuilder.Aggregate/ProfileAggregate copy the arguments into the command's list with the current option after the query, so a reused request reflects every change. AggregationCountOptionChangesTakeEffectWhenRequestIsReused covers clearing and switching after a build.
The DIALECT suffix that SerializeRedisArgs() appends has the same reuse problem, but that predates this PR, so I left it alone here.
`SerializeRedisArgs()` inserted the token into the request's own argument list, so building a reused request again kept the earlier token: clearing the option still sent it, and changing it sent both. The command builders now copy the request's arguments with the current option placed after the query, and leave the request untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XUNs4unCWEtK5q25xYbvgz
Closes #591.
What
Adds the
WITHCOUNT/WITHOUTCOUNToptions ofFT.SEARCHandFT.AGGREGATE:Query: abool? WithCountproperty (true→WITHCOUNT,false→WITHOUTCOUNT,null, the default → neither), set fluently withSetWithCount(bool? value = true), following the file'sSet<Flag>(bool value = true)convention. It is serialized with the other request flags, afterWITHPAYLOADS.AggregationRequest:WithCount(bool? value = true), with the same three states.The options are documented on both command pages (https://redis.io/docs/latest/commands/ft.search/, https://redis.io/docs/latest/commands/ft.aggregate/).
WITHOUTCOUNTturns on the query optimizer that DIALECT 4 enables by default. For a query sorted by a numeric field with a limit, it walks the field in order and stops once it has enough results, instead of evaluating and sorting every match. On 1M documents inclient-libs-test:8.10.0,FT.AGGREGATE idx '@n:[(cursor +inf]' SORTBY 2 @n ASC MAX 10takes 38.6 ms with the cursor at the start and 0.15 ms withWITHOUTCOUNT(server-side,INFO commandstats).Argument position in
FT.AGGREGATEThe server parses these options among the request options, before the first pipeline step:
(
redis-cliagainstclient-libs-test:8.10.0, 100 hashes withn= 0..99. The leading10is the total. UnderWITHOUTCOUNTit is not exact, which is the trade-off the option makes.)The docs describe
WITHCOUNTnext toSORTBY, so callers are likely to chain it there.AggregationRequestappends arguments in call order, soWithCount()stores the option in a field, likedialect, and the command builder places the token directly after the query, wherever in the chain it was set, without modifying the request, so a reused request reflects later changes. A later call replaces an earlier one,nullclears it, and the request never carries both.Version gate
Both options arrived in RediSearch 2.8 (
WITHOUTCOUNTin 2.8.0,WITHCOUNTin 2.8.3), which shipped with Redis Stack 7.2, so the integration tests are gated at7.2.0. Against an older server the option propagates as the server's own error; there's no client-side version check.Category
Unchanged: both remain pure reads, and the options don't affect what a replay does.
Tests
Search/CountOptionTests.cs(no server): exact argument layouts. The default sends neither option; each surface sends each one and can be cleared withnull; anAggregationRequestoption set afterLOAD/SORTBYstill follows the query; a later option replaces an earlier one; a token a caller inserted throughGetArgs()is left intact.SearchTests.TestCountOptionsSortedByNumericField+…Async:WITHCOUNTreturns the exact total.WITHOUTCOUNTreturns the same page, and an aggregation chainingWithCount(false)afterSORTBYandLOADis accepted and returns the right rows.Results for the first commit against
redislabs/client-libs-test:8.10.0(tests/dockers,REDIS_VERSION=8.10.0,-f net10.0):Searchsuite, both topologies (26 skipped: Enterprise / Entra ID)After review, the 9 count-option unit tests and the
Searchaggregation and query tests pass againstredis:8.10.2(standalone; the cluster variants were skipped). The integration tests now also skip Enterprise, like their neighbours.dotnet build src/NRedisStack/NRedisStack.csprojbuilds all target frameworks with no warnings.eng/public-api.pyreports the API files sorted, anddotnet format --verify-no-changespasses.🤖 Generated with Claude Code
Note
Low Risk
Optional read-only search flags with backward-compatible defaults and no change to command retry categories.
Overview
Adds client support for Redis
WITHCOUNTandWITHOUTCOUNTonQuery(WithCount/SetWithCount) andAggregationRequest(WithCount), so callers can request exact totals or allow the optimizer to skip full counting on sorted, limited queries.For aggregations, the count token is injected immediately after the query via new
AddArgsTo, because the server rejects the option after pipeline steps likeLOAD/SORTBYeven whenWithCountis chained later.SearchCommandBuilderaggregate/profile paths now useAddArgsToinstead of appendingGetArgs()wholesale.Unit tests cover argument layout and reuse/clear behavior; integration tests (Redis ≥ 7.2) exercise sorted numeric search/aggregate behavior with both options.
Reviewed by Cursor Bugbot for commit 51df5ba. Bugbot is set up for automated code reviews on this repo. Configure here.