[Redis 8.10] Add FT.AGGREGATE REDUCE COLLECT support with complex-value FieldValue - #3878
[Redis 8.10] Add FT.AGGREGATE REDUCE COLLECT support with complex-value FieldValue#3878uglide wants to merge 26 commits into
Conversation
…th String # Conflicts: # .gitignore
…pdate all affected files. Add additional unit and integration tests for the WITHSCORE param.
… field - it stays generic K.
…ot neede and fix all affected files.
…o be returned by search.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03a3031e49
ℹ️ 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".
| * @return {@code this} {@link SugAddArgs}. | ||
| */ | ||
| public SugAddArgs<K, V> payload(V payload) { | ||
| public SugAddArgs payload(String payload) { |
There was a problem hiding this comment.
Preserve binary suggestion payloads
When a connection uses ByteArrayCodec or another non-UTF-8 value codec, changing the payload from V to String and encoding it with args.add(...) makes arbitrary binary FT.SUGADD payloads impossible to send; the corresponding Suggestion.getPayload() change also forces replies through UTF-8 and can corrupt previously round-trippable bytes. Keep payloads codec-backed or expose a binary-safe alternative.
Useful? React with 👍 / 👎.
| } catch (RedisCommandExecutionException e) { | ||
| assumeTrue(false, "FT.AGGREGATE REDUCE COLLECT not supported by this Redis Search build: " + e.getMessage()); |
There was a problem hiding this comment.
Let unexpected COLLECT failures fail the test
On a Redis build that supports COLLECT, any server-side rejection—including malformed argument counts, invalid syntax, or a regression in this builder—is converted into an aborted assumption, so the new integration test reports success without exercising the feature. Restrict the skip to the specific unsupported-feature response and rethrow other command errors.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
| for (Map<String, String> result : reply.getResults()) { | ||
| for (HybridReply.HybridResult<String> hybridResult : reply.getResults()) { | ||
| Map<String, FieldValue> result = hybridResult.getFields(); | ||
| if ("apple".equals(result.get("brand"))) { |
There was a problem hiding this comment.
Compare the brand's scalar value
After result changed to Map<String, FieldValue>, result.get("brand") is a FieldValue, so neither String comparison can ever match and all AVG/MIN/MAX assertions are skipped even when the returned reducer values are wrong. Compare result.get("brand").asString() and assert that the expected groups were found.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
| * | ||
| * @author Viktoriya Kutsarova | ||
| */ | ||
| class AggregateArgsTest { |
There was a problem hiding this comment.
Rename the new unit tests for Surefire
AggregateArgsTest and the newly added HybridReplyTest are pure unit tests, but pom.xml excludes **/*Test from Surefire and routes that suffix to Failsafe, whose integration phase is disabled by the normal mvn test workflow. Rename both classes to end in UnitTests so these assertions run in the unit suite.
AGENTS.md reference: AGENTS.md:L131-L134
Useful? React with 👍 / 👎.
| * @param value the raw field value exactly as returned by the server. Must not be {@code null}. | ||
| * @return a {@link Kind#SCALAR} {@link FieldValue} view over the given bytes | ||
| */ | ||
| public static FieldValue of(byte[] value) { |
There was a problem hiding this comment.
Add @SInCE to every new FieldValue API
FieldValue.of(byte[]) is a newly introduced public method but lacks the required @since 7.7 tag; the same omission occurs on other new public members such as asBytes(), asString(), and isNull(). Add the release tag to each new public element.
AGENTS.md reference: AGENTS.md:L145-L146
Useful? React with 👍 / 👎.
| CommandArgs<K, V> args = new CommandArgs<>(codec).add(index).add(fieldName); | ||
|
|
||
| return createCommand(FT_TAGVALS, new ValueListOutput<>(codec), args); | ||
| return createCommand(FT_TAGVALS, new StringListOutput<>(codec), args); |
There was a problem hiding this comment.
Decode FT.TAGVALS entries through the value codec
When a Tag field was written through a non-identity value codec, FT.TAGVALS returns the encoded field bytes; replacing ValueListOutput with StringListOutput interprets those bytes directly as UTF-8 instead of applying codec.decodeValue, so serialized or binary tag values no longer round-trip and may be corrupted. Keep the codec-backed List<V> result or expose a raw binary-safe representation.
Useful? React with 👍 / 👎.
Adds the experimental
REDUCE COLLECTreducer forFT.AGGREGATE, which gathers per-row field projections within eachGROUPBYgroup. To carry its nested column values,FieldValueis extended from a scalar-only wrapper into a tagged union (Kind:SCALAR,ARRAY,MAP,NULL), andSearchReplyParserdecodes nested reply structures recursively instead of failing on theByteBuffercast.Key Decisions & Assumptions
FieldValueextension assumes that reply model and should be rebased if it changes.ARRAY, RESP3MAP); normalization happens at read time via a lenientasMap()that interprets a flat even-length array as key/value pairs. Callers read collected columns uniformly asasList()→ per-entryasMap()on both protocols. Any even-length scalar array (for example aTOLISTcolumn) is accepted by this view; documented as caller responsibility.COLLECTstays inAggregateArgs(theaggregateutilspackage serves FT.HYBRID only).Behavioral / Conceptual Changes
FieldValuearrays/maps; previously the parser failed on them. Scalar columns decode byte-identically to before.FieldValuethrough the wrong kind (for exampleasString()on an array) throwsIllegalStateException; an odd-length pair array is rejected rather than silently truncated.FIELDS *and explicit fields are mutually exclusive, and a reducer without fields fails at encode time.Testing
New
FieldValueUnitTestscover kind discrimination, the pair-normalizingasMap(), immutability, and structural equality.SearchReplyCollectParserUnitTestsverify RESP2/RESP3 collected-column parsing produces equal normalized entries, null preservation inside entries, and TOLIST-style arrays. Command-builder tests assert the exactREDUCE COLLECTwire format; an integration test runs a full COLLECT aggregation, self-skipping on servers withoutsearch-enable-unstable-features.Notes
COLLECTis gated behindsearch-enable-unstable-features; both the server feature and this API are marked@Experimental.HybridReplyParserstill decodes field values as scalars only; FT.HYBRID cannot emit COLLECT today, so this is a known limitation, not a regression.Note
Cursor Bugbot is generating a summary for commit 03a3031. Configure here.