Repository navigation
8.12 CMS: support CELL_SIZE, fix CMS.INFO field parsing, cover negative INCRBY - #562
mgravell wants to merge 4 commits into
Conversation
Sorry, something went wrong.
…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.
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 GitHubCodex 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".
Sorry, something went wrong.
There was a problem hiding this comment.
Stale Bugbot comment from a previous run.
Sorry, something went wrong.
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 GitHubCodex 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".
Sorry, something went wrong.
…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.
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 GitHubCodex 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".
Sorry, something went wrong.
| count = (long)redisResults[i]; | ||
| break; | ||
| case "cell size": | ||
| case "cell_size": |
There was a problem hiding this comment.
Handle the legacy 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 👍 / 👎.
Sorry, something went wrong.
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.
Sorry, something went wrong.
| @@ -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.
Sorry, something went wrong.
This branch has not been deployed
Reviewers
At least 1 approving review is required to merge this pull request.
Summary
Test plan
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.12 image) 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 optional cellSize (1, 2, 4, or 8), emitted as CELL_SIZE <n> with client-side validation. CMS.INFO parsing is corrected to read the cell_size field so CmsInformation.CellSize is populated instead of staying at -1.
Integration tests cover cellSize initialization, invalid cellSize, and negative CMS.INCRBY increments (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.