fix(cubestore): advertise INCR, not INC, in the CACHE unknown-command error

[igorlukanin]

Issue

The CACHE command parser accepts incr, but the error it emits for an unknown subcommand advertises INC — a keyword that doesn't parse. An operator who follows the error message gets a second error:

$ CACHE FOO bar;
Unknown cache command: foo, available: SET|GET|KEYS|INC|REMOVE|CLEAR

$ CACHE INC bar;
Unknown cache command: inc, available: SET|GET|KEYS|INC|REMOVE|CLEAR

The accepted set is exactly SET|GET|KEYS|INCR|REMOVE|CLEAR, so INC is the only inaccurate entry. This is the only enumerated-keyword error message in the parser — the other unknown-command arms emit a bare "Unknown … command" with no list.

Description of Change

Corrects the advertised keyword to INCR.

[claude]

Claude finished @igorlukanin's task in 1m 34s —— View job

---

Review complete — approve; 0 high, 0 medium, 1 low (1 inline comment)

Full review

• Read PR diff
• Verify parser match arms vs advertised keywords
• Assess test quality
• Check for other places advertising the same list
• Post review

Verdict

The fix is correct and minimal. parse_cache (rust/cubestore/cubestore/src/sql/parser.rs:480-515) has exactly six arms — set, get, keys, incr, remove, clear — so INCR is the right spelling and INC was the only wrong entry. Grepped the repo: line 511 is the only place this list is spelled out, so there's no sibling message (JS driver, docs, cubesql) left stale.

Test correctness

I traced the message-extraction path by hand since I couldn't run cargo here (see caveat below):

• From<ParserError> for CubeError (rust/cubestore/cubestore/src/lib.rs:212) wraps via format!("{:?}", v), and the User/non-User split in Display (lib.rs:179) prefixes Internal: . So to_string() yields:
  Internal: ParserError("Unknown cache command: nope, available: SET|GET|KEYS|INCR|REMOVE|CLEAR")
• split("available: ").nth(1) → SET|...|CLEAR"), and the final split yields CLEAR"). The trim_matches closure strips the trailing "), so the inline comment about Debug rendering is accurate and the assert_eq! holds.
• The per-command arguments are each minimally well-formed: SET key 'value' satisfies parse_identifier + parse_literal_string after the optional NX/TTL probes; CLEAR takes none; the other four take a single identifier. So a failure in the loop can only mean the keyword itself wasn't recognized — the stated design goal holds.

Asserting the property rather than the literal string is the right call here, and the red-check against three wrong implementations described in the PR body is the part that makes it credible.

Low: coverage is one-directional

Posted inline at the assert_eq!. The test proves advertised ⊆ parsable but not the converse, so the "drift apart in either direction" claim in the description is stronger than what's asserted — adding a new match arm without updating the message fails the hardcoded assert_eq! with a message that points at the wrong side of the drift. Suggested deriving the error string from a single CACHE_COMMANDS const so the two can't disagree by construction. Non-blocking.

Other areas

Nothing to report on security (no input-handling change; this is an error string on an already-rejected statement), performance (cold error path, String allocations are test-only), or docs (internal error text, no public API surface). No CHANGELOG entry needed — Lerna doesn't cover the Rust crates and this is user-invisible beyond the corrected text.

Caveat

I could not execute cargo test/cargo fmt — the sandbox denied the command, so the new test is verified by reading, not by running. CI on this PR should confirm. If you want me to run the Rust suite on future reviews, cargo needs to be added to --allowedTools.

· [Branch](https://github.com/cube-js/cube/tree/igor/core-697-cube-store-cache-parser-error-message-advertises-inc-but-the)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.00%. Comparing base (d8d009b) to head (f998ed0).
⚠️ Report is 3 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (d8d009b) and HEAD (f998ed0). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (d8d009b)	HEAD (f998ed0)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11434       +/-   ##
===========================================
- Coverage   83.95%   59.00%   -24.96%     
===========================================
  Files         257      223       -34     
  Lines       80887    17891    -62996     
  Branches        0     3636     +3636     
===========================================
- Hits        67908    10556    -57352     
+ Misses      12979     6817     -6162     
- Partials        0      518      +518     

Flag	Coverage Δ
cube-backend	59.00% <ø> (?)
cubesql	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @igorlukanin's task in 47s —— View job

---

Review complete — approve; 0 high, 0 medium, 0 low (no inline comments)

Full review

• Re-read PR diff after force-push
• Verify parser match arms vs advertised keywords
• Confirm no other location spells out the list
• Check prior review threads for stale/duplicate concerns
• Post review

What changed since the last review

The PR is now the single-line string fix only — the property test that my earlier comment discussed has been dropped, per @igorlukanin's "Don't add a new test, the fix above is enough." Diff is exactly INC → INCR at rust/cubestore/cubestore/src/sql/parser.rs:511.

Verdict

Correct and minimal. parse_cache (rust/cubestore/cubestore/src/sql/parser.rs:480-515) has exactly six arms — set, get, keys, incr, remove, clear — so the advertised list now matches the accepted set element-for-element, and INC was the only wrong entry. Grepped the repo for the pipe-separated list: line 511 is the only occurrence, so there's no sibling message in the JS driver, cubesql, or docs left stale.

Skipping tests is a reasonable call here — the string is user-visible only on an already-rejected statement, and a test asserting it would either be a tautology on the literal text or reintroduce the one-directional coverage gap I flagged before. The trade-off is that nothing mechanically prevents the same drift if a seventh arm is added later; that's the author's call and I'm not re-raising it.

Other areas

• Security: no input-handling change; the parse already failed before reaching this arm.
• Performance: cold error path, no allocation change.
• Docs / API: internal error text, no public surface. No CHANGELOG entry needed — Lerna doesn't cover the Rust crates.

Prior threads

Both existing threads on this file (my assert_eq! comment and Igor's reply) are already resolved and outdated — nothing for me to resolve or re-affirm. No new inline comments posted.

Caveat

I could not run cargo test / cargo fmt — the sandbox denies cargo, so verification here is by reading. For a one-line string literal inside an existing format! there's no compilation risk, and CI covers it. If you want the Rust suite run on future reviews, cargo needs to be in --allowedTools.

· [Branch](https://github.com/cube-js/cube/tree/igor/core-697-cube-store-cache-parser-error-message-advertises-inc-but-the)