Repository navigation
Conversation
…NCRBY - CMS.INITBYDIM/INITBYPROB take an optional cellSize (1, 2, 4, or 8 bytes) argument, sent as CELL_SIZE <n>. - CmsInformation.CellSize now parses the "cell_size" field correctly; it previously looked for "cell size" (with a space) and always read -1. - Add regression tests for CELL_SIZE and for negative CMS.INCRBY increments (already passed through unchanged, now covered).
InitByDim/InitByProb (and their Async/interface/builder counterparts) keep their original 3-arg signatures; CELL_SIZE support is added via new 4-arg overloads instead of an extra optional parameter on the existing methods, so the existing compiled signatures are unchanged.
![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: e940b7c2f9
ℹ️ 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".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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: 8ce6301acf
ℹ️ 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".
…x row - CMS.INITBYDIM/INITBYPROB CELL_SIZE tests and CMS.INCRBY negative-increment tests are SkipIfRedisTheory-gated at 8.12.0, since older servers reject the CELL_SIZE argument (wrong number of arguments) and negative increments. - Add tests/dockers/.env.v8.12, pinned to an unstable preview image pending an official 8.12 release, and add "8.12" to both the PR/push and nightly redis-version matrices in integration.yml.
mgravell
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7f71a59d8
ℹ️ 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".
| count = (long)redisResults[i]; | ||
| break; | ||
| case "cell size": | ||
| case "cell_size": |
There was a problem hiding this comment.
cell size response field
When querying Redis 8.10, this regresses CMS.INFO: the preceding parser intentionally handled its response field as "cell size" (d5644f5, using the official 8.10 test image). Replacing that case instead of accepting both spellings makes CmsInformation.CellSize fall back to -1 for those existing servers even though they return a value. Match both "cell size" and "cell_size".
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ 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 c7f71a5. Configure here.
| @@ -1 +1,12 @@ | |||
| #nullable enable | |||
| NRedisStack.CountMinSketch.DataTypes.CmsInformation.CellSize.get -> int | |||
There was a problem hiding this comment.
Duplicate shipped CellSize public API
Low Severity
CmsInformation.CellSize is appended to PublicAPI.Unshipped.txt, but that getter is already recorded in PublicAPI.Shipped.txt. The property signature did not change in this PR, so the unshipped line is a duplicate. eng/public-api.py --promote treats this as a clash (and notes that the analyzer reports RS0025), so the next release promotion will fail until the extra entry is removed.
Reviewed by Cursor Bugbot for commit c7f71a5. Configure here.
This branch has not been deployed
1 participant
Summary
CMS.INITBYDIM/CMS.INITBYPROBgain an optionalcellSizeparameter (1, 2, 4, or 8), sent asCELL_SIZE <n>.CmsInformation.CellSizeparsing: the server reports the field ascell_size, notcell size(with a space) as previously coded, so the value was always -1.CMS.INCRBYalready passed negative increments through to the server unchanged; add test coverage for that path.CMS.INCRBY'sOOR SKIP|SAT|FAILflag andCMS.MERGEacross differingCELL_SIZEvalues are not yet supported client-side, pending server-side availability.Test plan
dotnet build(Rebuild) onsrc/NRedisStackandtests/NRedisStack.Tests, 0 warnings/errors, PublicAPI analyzer cleanCmsTestssuite passes againstredislabs/client-libs-test:unstable-34786335206-debian, including newTestInitByDimCellSize(Async),TestInitByProbCellSize,TestInitByDimInvalidCellSize,TestIncrByNegative(Async)Note
Low Risk
Additive public API and a response-parser bugfix for CMS; CI adds another Redis matrix leg using a preview image.
Overview
Adds Redis 8.12 to integration CI (including a preview
tests/dockers/.env.v8.12image) so new CMS behavior can be exercised on PRs and nightly runs.For Count-Min Sketch (CMS),
InitByDim/InitByProb(sync and async) gain overloads with optionalcellSize(1, 2, 4, or 8), emitted asCELL_SIZE <n>with client-side validation.CMS.INFOparsing is corrected to read thecell_sizefield soCmsInformation.CellSizeis populated instead of staying at -1.Integration tests cover
cellSizeinitialization, invalidcellSize, and negativeCMS.INCRBYincrements (gated to Redis ≥ 8.12). Public API entries are updated for the new surface.Reviewed by Cursor Bugbot for commit c7f71a5. Bugbot is set up for automated code reviews on this repo. Configure here.