Repository navigation
Add missing plain overloads for experimental vector-set commands - #4693
kalayciburak wants to merge 19 commits into
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: cfc526490f
ℹ️ 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 elements similar to a given vector with their similarity scores. | ||
| * @since 8.1 | ||
| */ |
There was a problem hiding this comment.
When generated API documentation is built, this newly inserted Javadoc sits between the original detailed block and the existing params-taking declaration, so Java associates only the new block with the no-params overload and leaves vsimWithScores(byte[], float[], VSimParams) undocumented. The same adjacent-comment pattern removes documentation from the other score overloads here and from the reduceDim and score overloads in both pipeline interfaces; place each new overload and its documentation before the original block instead.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
|
Thanks for the PR! I'll try to take a look once I get some free cycles. Heads up — I'm on vacation starting next week, so the review may take a bit longer. |
14fc8c4 to
36d68b5
Compare
Vector-set methods with REDUCE or WITHSCORES had no no-params overload, so callers had to pass null. Add the additive overloads requested in redis#4688 on command, binary, and pipeline surfaces. Null-tolerance removal is left for a follow-up.
36d68b5 to
eafb853
Compare
Codex Review SummaryThis 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddea610b48
ℹ️ 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".
| * @since 8.1 | ||
| */ | ||
| @Experimental | ||
| Map<String, VSimScoreAttribs> vsimWithScoresAndAttribs(String key, float[] vector); |
There was a problem hiding this comment.
When callers use Pipeline or Transaction, the two new no-params WITHATTRIBS operations are unavailable: a repo-wide search shows that neither pipeline interface nor PipeliningBase exposes vsimWithScoresAndAttribs or vsimByElementWithScoresAndAttribs (the parameterized forms are absent too), even though the commit now exposes both direct String/binary forms. Add both typed pipeline pairs so this command change does not leave pipeline/transaction users behind.
AGENTS.md reference: AGENTS.md:L67-L77
Useful? React with 👍 / 👎.
| * @since 8.1 | ||
| */ | ||
| @Experimental | ||
| boolean vadd(String key, float[] vector, String element, int reduceDim); |
There was a problem hiding this comment.
When users consult the 8.1 upgrade/release documentation, this new public API is invisible: the diff changes no file under docs/, and a repo-wide rg finds no vector-set API page to update. Add an entry to docs/release-notes/8.1.0.md (or a dedicated linked API page) covering the no-params overloads; public-API changes are required to update the relevant MkDocs documentation.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eb90c6fff
ℹ️ 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".
| import redis.clients.jedis.params.VSimParams; | ||
| import redis.clients.jedis.resps.VSimScoreAttribs; | ||
|
|
||
| public class PipeliningBaseVectorSetCommandsTest extends PipeliningBaseMockedTestBase { |
There was a problem hiding this comment.
When CI runs mvn formatter:validate, this new file is skipped: the formatter allowlist's only vector-set glob is **/VectorSet*.java at pom.xml:555, which does not match the PipeliningBaseVectorSetCommandsTest.java basename, and no exact include was added. Register this path (or rename and register the file) so the repository's required formatting check covers it.
AGENTS.md reference: AGENTS.md:L85-L89
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7005fd2922
ℹ️ 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 new CommandObject<>(args, BuilderFactory.BOOLEAN); | ||
| } | ||
|
|
||
| public final CommandObject<Boolean> vadd(String key, float[] vector, String element, int reduceDim) { |
There was a problem hiding this comment.
When the 8.1 API Javadocs are generated, this new public CommandObjects method (and the other 11 overloads added to this class) has no release provenance because there is no Javadoc/@since tag. CommandObjects is a public API surface, so add @since 8.1 to each of these overloads rather than limiting the tag to command-interface declarations.
AGENTS.md reference: AGENTS.md:L27-L31
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4652f0ec2f
ℹ️ 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".
| * Add a new element into the vector set with dimension reduction. | ||
| * @since 8.1 | ||
| */ | ||
| public final CommandObject<Boolean> vadd(String key, float[] vector, String element, int reduceDim) { |
There was a problem hiding this comment.
For callers that use the public CommandObjects API directly, this overload and the other 11 factories added here carry no @Experimental marker. These methods do not override the annotated command-interface methods, so the marker is not inherited and generated API documentation/tooling presents them as stable even though the vector-set API may change or be removed; annotate each new factory as CommandObjects.ftHybrid is.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1b4d8b78d
ℹ️ 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".
| } | ||
|
|
||
| @Override | ||
| public boolean vadd(String key, float[] vector, String element, int reduceDim) { |
There was a problem hiding this comment.
When consumers or documentation tools inspect the concrete client classes, this new overload (and the other added UnifiedJedis, Jedis, and PipeliningBase overrides) has no @Experimental marker. Java method annotations are not inherited from implemented interfaces, so these newly exposed preview operations appear stable on the classes users call; add @Experimental to every new concrete override, as is already done for ftHybrid.
AGENTS.md reference: AGENTS.md:L85-L89
Useful? React with 👍 / 👎.
…4758) * Initial commit. Signed-off-by: yordantsintsov <yordan.tsintsov@gmail.com> * Refactored RESP2 parsing from JSONArray to custom logic. Signed-off-by: yordantsintsov <yordan.tsintsov@gmail.com> * Optimize JSON number array parsing without split() Replace inefficient split() and trim() calls with index-based manual parsing. Single pass for decimal detection. Rename parseNumber → parseNullOrNumber. Add comprehensive validation: reject empty elements, leading/trailing commas, consecutive commas. Add 19 new tests covering valid arrays, nulls, scientific notation, whitespace, and error cases. * Rename jsonNumIncrByNumber to jsonNumIncrByAsNumbers Method name should indicate return type (List<Number>) not parameter type. Clearer that it returns numeric values, not increments by a number. * Polish Signed-off-by: yordantsintsov <yordan.tsintsov@gmail.com> * Polish Signed-off-by: yordantsintsov <yordan.tsintsov@gmail.com> * Format JSON number parsing files Add JsonBuilderFactory and JsonBuilderFactoryTest to the formatter includes and format them. * Fix javadoc that fails the docs build Unescaped List<Object> in the NUMBER_LIST javadoc is read as an HTML tag by javadoc (error: unknown tag: Object) and fails the Build docs step. Wrap generics in {@code} and link the boxed types. * Simplify JSON number array element loop Drop the expectToken flag and two unreachable blocks (leading-trim loop, empty-token check after it): after whitespace is skipped the scan always starts on a non-blank char. Restore the one-line indexOf integral check; the manual scan bought nothing on 1-20 char tokens. Behavior unchanged: rejects empty elements, leading/trailing commas. * Use SafeEncoder.encode in JsonBuilderFactoryTest Project convention: never String.getBytes(), it breaks under GBK. --------- Signed-off-by: yordantsintsov <yordan.tsintsov@gmail.com> Co-authored-by: ggivo <ivo.gaydazhiev@redis.com>
Vector-set methods with REDUCE or WITHSCORES had no no-params overload, so callers had to pass null. Add the additive overloads requested in redis#4688 on command, binary, and pipeline surfaces. Null-tolerance removal is left for a follow-up.
…tor-set-plain-overloads
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3314842933
ℹ️ 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 elements similar to a given vector with their similarity scores. | ||
| * @since 8.1 | ||
| */ |
There was a problem hiding this comment.
For consumers browsing generated Javadocs through the binary or pipeline command interfaces, this new block is a dead end: it omits parameter/return documentation and has no @see link to the fully documented String overload. The other newly added binary overloads and pipeline overloads follow the same unlinked pattern, even though the command-extension procedure requires binary variants to link to the String API and pipeline variants to use {@link ...}; update these blocks accordingly so every new surface exposes the canonical documentation.
AGENTS.md reference: AGENTS.md:L85-L89
Useful? React with 👍 / 👎.
3 participants
Summary
Experimental vector-set methods that take
reduceDimor return scores currently have no no-params overload, so callers have to passnull. The rest of Jedis models optional params as a plain method plus a params-taking one.This is the additive part of #4688:
vadd/vaddFP32withreduceDimand no paramsvsimWithScores/vsimByElementWithScoreswith no paramsvsimWithScoresAndAttribs/vsimByElementWithScoresAndAttribswith no params (command surfaces only; pipeline does not have those variants)Plain overloads encode the same argv as the existing params overload with
null. Null-tolerance removal is left for a follow-up.Test plan
mvn -Dtest=VectorSetPlainOverloadsTest test(12/12)mvn -DskipTests compile test-compilemvn formatter:validateNote
Low Risk
Additive API surface on experimental vector-set and JSON commands; existing parameterized methods are unchanged.
Overview
Adds parameter-free overloads for experimental vector-set APIs so callers no longer need to pass
nullfor optionalVAddParams/VSimParams. New methods covervadd/vaddFP32withreduceDim, plusvsimWithScores,vsimWithScoresAndAttribs, and thevsimByElement*score/attrib variants, wired throughCommandObjects,Jedis,UnifiedJedis, pipeline interfaces, andPipeliningBase(each delegates to the existing params overload withnull).Also introduces
jsonNumIncrByAsNumbersforJSON.NUMINCRBY, returningList<Number>withLong/Doubletyping via a newJsonBuilderFactory.NUMBER_LIST(RESP3 list + RESP2 JSON array parsing), exposed on sync and pipeline JSON v2 APIs.Release notes document the vector-set overloads; tests assert command equivalence,
@Experimentalmarking, JSON/pipeline behavior, and extend formatter coverage for touched files.Reviewed by Cursor Bugbot for commit 3314842. Bugbot is set up for automated code reviews on this repo. Configure here.