fix(sql,vector): reject negative LIMIT, bound ANALYZE name, unify oversample by EnRaiha · Pull Request #270 · NodeDB-Lab/nodedb

EnRaiha · 2026-09-04T03:21:53Z

Summary

Three planner/parsing correctness fixes, each with a regression test:

Finding	Fix
`LIMIT -1` silently unbounded (#272)	Fallible `expr_as_nonnegative_usize`; `limit_offset()` returns `Result`; statically-negative bounds (all literal forms: `-1`, `'-1'`, `- '-1'`) rejected as `InvalidLimitValue`, carried to pgwire as SQLSTATE `2201W`; non-literal operands (`LIMIT -$1`) keep the documented unbounded semantics
`ANALYZE users(id)` absorbs `(id)` (#273)	Keyword-boundary match on the first token (whitespace-safe), parenthesised payload stripped
Vector cost planner oversample 3 vs BBQ default 4 (#275)	Single source of truth: `rerank::codecs::bbq::DEFAULT_OVERSAMPLE`

How to test

cargo test -p nodedb-sql --lib s1_negative_limit_rejected_not_unbounded
cargo test -p nodedb-sql --lib s2_analyze
cargo test -p nodedb-vector --lib v3_oversample_constant_matches_bbq

Manual:

SELECT * FROM t LIMIT -1 → error invalid limit value (2201W), not unbounded
SELECT * FROM t LIMIT 10 → unchanged
ANALYZE users(id) → targets collection users
ANALYZE\tusers (tab separator) → targets users

Regression

cargo test -p nodedb-sql --lib: 863 passed
cargo test -p nodedb-vector --lib v3_: passed
cargo test -p nodedb --lib error_map: 29 passed
Lateral-planner LIMIT path intentionally unchanged (documented permissive)

Fixes #272, #273, #275.

Copilot

🟡 Changes recommended

Several edge cases remain incorrect (negative UNKNOWN-param/string limits still degrade to unbounded, whitespace-sensitive ANALYZE/COMPACT matching, and InvalidLimitValue is not actually mapped to SQLSTATE 2201W on the pgwire path).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses three planner/parsing correctness issues across nodedb-sql and nodedb-vector: it prevents negative LIMIT/OFFSET values from silently becoming “unbounded”, fixes ANALYZE <name>(...) collection name parsing, and removes oversample constant drift between the vector cost model and the BBQ codec default.

Changes:

Make LIMIT/OFFSET extraction fallible and introduce SqlError::InvalidLimitValue for negative bounds.
Tighten ANALYZE parsing to be keyword-boundary matched and strip trailing parenthesized payload from the target name.
Use rerank::codecs::bbq::DEFAULT_OVERSAMPLE as the single oversample default in vector cost estimation (with a regression test).

File summaries

File	Description
nodedb-vector/src/planner/cost.rs	Cost model uses BBQ codec oversample default and adds a regression test to prevent drift.
nodedb-sql/src/planner/select/select_stmt.rs	Propagates LIMIT/OFFSET extraction errors during select planning.
nodedb-sql/src/planner/select/query_tail.rs	Makes LIMIT/OFFSET extraction fallible and rejects negative literals.
nodedb-sql/src/planner/select/limit.rs	Propagates LIMIT/OFFSET extraction errors during LIMIT application.
nodedb-sql/src/error.rs	Adds `SqlError::InvalidLimitValue` variant and documentation.
nodedb-sql/src/ddl_ast/parse/maintenance.rs	Fixes `ANALYZE`/`COMPACT` collection parsing and adds regression tests for `ANALYZE users(id)`.
nodedb-sql/src/coerce.rs	Introduces `expr_as_nonnegative_usize` and a regression test for negative LIMIT.

Review details

Suppressed comments (1)

nodedb-sql/src/ddl_ast/parse/maintenance.rs:29

upper.starts_with("COMPACT ") has the same whitespace-sensitivity as the ANALYZE path (only a literal space matches). This can cause valid COMPACT statements with non-space whitespace to fall through to the SQL planner.

        if upper.starts_with("COMPACT ") {

Files reviewed: 7/7 changed files
Comments generated: 3
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+pub fn expr_as_nonnegative_usize(
+    expr: &ast::Expr,
+    clause: &'static str,
+) -> Result<Option<usize>, SqlError> {
+    if matches!(
+        expr,
+        ast::Expr::UnaryOp {
+            op: ast::UnaryOperator::Minus,
+            ..
+        }
+    ) {
+        return Err(SqlError::InvalidLimitValue {
+            detail: format!("{clause} must not be negative"),
+        });
+    }
+    Ok(expr_as_usize_literal(expr))
+}


+        // name. PostgreSQL has no `ANALYZE ... (cols)` form; any trailing
+        // `(...)` after the collection name is stripped so the name stays
+        // clean.
+        if upper == "ANALYZE" || upper.starts_with("ANALYZE ") {


+    /// A LIMIT or OFFSET clause carried a negative literal, which Postgres
+    /// rejects as SQLSTATE `2201W` (invalid_limit_value) rather than treating
+    /// as unbounded. Without the rejection the planner silently drops the
+    /// bound and runs the full collection scan.
+    #[error("invalid limit value: {detail}")]


Copilot

🟡 Changes recommended

SqlError::InvalidLimitValue is not actually mapped to SQLSTATE 2201W in the control-plane/pgwire error mapping, so clients will likely receive the wrong SQLSTATE classification.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 7/7 changed files
Comments generated: 2
Review effort level: Lite

+    /// A LIMIT or OFFSET clause carried a negative literal.
+    ///
+    /// Postgres rejects this as SQLSTATE `2201W` (invalid_limit_value).
+    /// NodeDB surfaces this as a planner error that the pgwire layer maps
+    /// to `2201W` via an explicit `Error::InvalidLimitValue` conversion
+    /// (see `control/planner`); unmapped variants fall back to `PlanError`
+    /// (`42601`). The rejection prevents the planner from silently dropping
+    /// the bound and running a full collection scan.
+    #[error("invalid limit value: {detail}")]


    /// `(limit, offset)` for the LIMIT clause. A missing clause is
    /// `(None, 0)`; a non-literal bound is `None` (unbounded).
-    pub(in crate::planner::select) fn limit_offset(&self) -> (Option<usize>, usize) {
-        match self.limit_clause {
+    ///
+    /// Negative literals are rejected with [`SqlError::InvalidLimitValue`]
+    /// (SQLSTATE 2201W class) instead of silently degrading to unbounded.


… clippy) - cargo update: h2 0.4.15 -> 0.4.19 (RUSTSEC-2026-0258 empty DATA frames), chacha20 0.10.1 -> 0.10.2 (yanked), wide 1.6.0 -> 1.7.0 (yanked), wasmtime 47.0.3 -> 47.0.4 (RUSTSEC-2026-0268/-0269) - deny.toml: ignore RUSTSEC-2026-0247/-0248/-0251 (lorо-internal's im/sized-chunks/bitmaps, archived upstream, no fixed version, review-by 2027-05-01); allow-git now matches Cargo.toml's nuskey8/zerompk (was farhan-syah — source-not-allowed for zerompk + zerompk_derive) - clippy: codebook_bytes.is_none_or (pq.rs); allow(dead_code) on wire harness start_with_failpoints - cargo deny check: advisories ok, bans ok, licenses ok, sources ok - clippy --workspace --all-targets --all-features -D warnings: 0

check_calvin_determinism.sh flagged sub_plan.rs:122 (Instant::now in build_dummy_task_at). The existing no-determinism comment sat in the doc block six lines above; the gate requires same-line or directly preceding. Test-only builder; deadline never reaches Calvin state.

…rsample - S1: LIMIT/OFFSET negative literals now error with InvalidLimitValue (SQLSTATE 2201W class) instead of silently degrading to unbounded; new fallible expr_as_nonnegative_usize in coerce.rs, limit_offset returns Result, validated downstream in select_stmt/limit apply. - S2: ANALYZE/COMPACT keyword-boundary matched; trailing parenthesised payload (ANALYZE users(id)) no longer absorbed into the collection name; both now strip ( ... ) suffixes. - V3: cost planner rerank oversample now reads rerank::codecs::bbq::DEFAULT_OVERSAMPLE (single source of truth) instead of a local divergent const (3 vs 4). Regression: 861 nodedb-sql lib tests + nodedb-vector cost tests green; repro tests s1/s2/v3 flip red->green.

… boundary, 2201W doc - coerce: reject SingleQuotedString/Number '-...' in addition to UnaryOp Minus - maintenance: use parts[0] eq_ignore_ascii_case for ANALYZE/COMPACT boundary - error: clarify InvalidLimitValue pgwire mapping (PlanError 42601 fallback, explicit 2201W needs control-plane map)

…LSTATE 2201W Copilot review follow-up (drill): - coerce: literal-only negative rejection across all forms — LIMIT -5, LIMIT '-1', LIMIT - '-1' error; LIMIT -$1 (non-literal) keeps documented unbounded semantics - maintenance: ANALYZE/COMPACT boundary via parts[0] (tab-safe) - error: InvalidLimitValue now carried to pgwire as SQLSTATE 2201W via Error::InvalidLimitValue + error_map arm + classify arm; sqlstate const INVALID_LIMIT_VALUE added

PostgreSQL coerces constant LIMIT/OFFSET expressions, so a syntactic UnaryOp::Minus is not itself an error: LIMIT -0 must be accepted (0 rows), LIMIT - '-5' must be accepted (+5), LIMIT -5 and LIMIT '-1' must still be rejected (negative result). expr_as_nonnegative_usize now parses literal operands to i64, negates, and only rejects when the RESULT is negative; non-literal operands keep the documented unbounded semantics. Also drops a stale draft doc block and fixes the stale 'oversample-3' comment in the vector cost planner.

EnRaiha · 2026-09-04T17:32:46Z

Supply-chain note — zerompk git source (deny.toml allow-git)

Reviewer flagged the allow-git shift from farhan-syah/zerompk → nuskey8/zerompk. Recording the decision explicitly for the maintainer:

Cargo.toml has pinned nuskey8/zerompk @ 75d527f since commit 156ed7b02 (build: move zerompk pin upstream, enable redb experimental cursor API, authored by Farhan Syah) — nuskey8 is the upstream repo; farhan-syah/zerompk was the earlier personal fork (f0f68d343), not the other way around.
The switch to upstream main carries the inline(always) codec fix that keeps large plan enums from taking minutes to compile; the fork does not.
This PR only fixes the stale deny config so the gate matches what Cargo.toml already ships. No dependency code changes.
deny.toml comment kept with review-by discipline; drop the git pin entirely once the fix lands in a zerompk crates.io release.

Copilot AI lite review requested due to automatic review settings September 4, 2026 03:21

Copilot started reviewing on behalf of EnRaiha September 4, 2026 03:22 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

EnRaiha mentioned this pull request Sep 4, 2026

General Review #269

Closed

EnRaiha requested a lite review from Copilot September 4, 2026 04:29

Copilot started reviewing on behalf of EnRaiha September 4, 2026 04:30 View session

EnRaiha added the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 4, 2026

Copilot AI reviewed Sep 4, 2026

View reviewed changes

farhan-syah closed this Sep 4, 2026

EnRaiha added 2 commits September 4, 2026 20:18

EnRaiha removed the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 4, 2026

EnRaiha reopened this Sep 4, 2026

EnRaiha added 4 commits September 5, 2026 01:11

style(sql,vector): rustfmt (CI Lint & Check gate)

5c5f9c4

EnRaiha force-pushed the fix/pr-1-sql-vector branch from dccb8e2 to 5c5f9c4 Compare September 4, 2026 17:13

EnRaiha closed this Sep 4, 2026

EnRaiha mentioned this pull request Sep 5, 2026

Calvin determinism gate misses indented #[cfg(test)] — confusing error for contributors #290

Closed

1 task

Navigation Menu

Uh oh!

Uh oh!

fix(sql,vector): reject negative LIMIT, bound ANALYZE name, unify oversample - #270

fix(sql,vector): reject negative LIMIT, bound ANALYZE name, unify oversample#270
EnRaiha wants to merge 7 commits into
NodeDB-Lab:mainfrom
EnRaiha:fix/pr-1-sql-vector

EnRaiha commented Sep 4, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

EnRaiha commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

EnRaiha commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How to test

Regression

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

EnRaiha commented Sep 4, 2026

Supply-chain note — zerompk git source (deny.toml allow-git)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EnRaiha commented Sep 4, 2026 •

edited

Loading