Skip to content

planner: write the vector-primary key guard as is_none_or - #385

Open
EnRaiha wants to merge 2 commits into
NodeDB-Lab:mainfrom
EnRaiha:fix/planner-nonminimal-bool
Open

EnRaiha wants to merge 2 commits into
NodeDB-Lab:mainfrom
EnRaiha:fix/planner-nonminimal-bool

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

planner: write the vector-primary key guard as is_none_or

Why

cargo clippy -p nodedb --lib --all-features -- -D warnings fails on main at
nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs:164:

error: this boolean expression can be simplified
   --> nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs:164:16
    |
164 |               && !fields
165 | |                 .get(key_column)
166 | |                 .is_some_and(|v| !matches!(v, SqlValue::Null))
    | |______________________________________________________________^
    |
    = note: `-D clippy::nonminimal-bool` implied by `-D warnings`

No in-flight branch touches that line, so the lint gate blocks every unrelated review. This
change applies clippy's own rewrite.

What changed

One file, +109/-2 against main:

  • nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs — the key-column guard
    written in clippy's suggested form, plus four tests in its mod tests that pin the
    key-column fill rule.

The new spelling is the negation identity is_none_or(f) == !is_some_and(!f), so the guard
selects the same rows: a row gets a minted identity exactly when the key column is absent or
NULL.

if !is_auto_rowid_pk(primary_key)
    && fields
        .get(key_column)
        .is_none_or(|v| matches!(v, SqlValue::Null))

Effect

No user-visible change. The planner builds the same plan for the same statement: no wire,
SQL, error-code, metric, config or on-disk change. is_none_or needs Rust 1.82 and the
workspace sets rust-version = "1.94", so the toolchain floor does not move either.

Steps to test

cargo clippy --workspace --all-targets --all-features --profile ci -- -D warnings   # exit 0
cargo nextest run -p nodedb --lib -E 'test(~key_column)'                            # 5 passed

The first command is the CI gate verbatim. The failing state reproduces at main: the same
clippy command exits 101 with the diagnostic quoted in Why.

Checks

step command exit at
red cargo clippy -p nodedb --lib --all-features -- -D warnings 101 main (f18b31edf)
green cargo clippy -p nodedb --lib --all-features -- -D warnings 0 77cb53893
green cargo nextest run -p nodedb --lib -E 'test(~key_column)' 0 · 5 passed 77cb53893
green cargo clippy --workspace --all-targets --all-features --profile ci -- -D warnings 0 77cb53893
assertion strength cargo nextest run -p nodedb --lib -E 'test(~key_column)', guard removed 100 · 3 passed, 2 failed 77cb53893 plus the guard removed

The last row is what makes the tests evidence: with the guard removed, the two fill cases
fail, so they cover the guard's behaviour and not only that the file compiles. The base
predicate passes the same five tests, so no case is red on main.

Copilot AI lite review requested due to automatic review settings September 27, 2026 11:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

`cargo clippy --profile ci -p nodedb --all-targets --all-features -- -D warnings`
aborts on this expression at `vector_primary.rs:161`:

    error: this boolean expression can be simplified
        = help: ... #nonminimal_bool

so the lint job is red on `main` for every branch, and the first thing it
reports is a file no branch has touched. Clippy's own suggested rewrite is this
one, and it is the negation identity `is_none_or(f) == !is_some_and(!f)`:
`!get(k).is_some_and(|v| !matches!(v, SqlValue::Null))` becomes
`get(k).is_none_or(|v| matches!(v, SqlValue::Null))`.

All three cases agree, including the one a reader might expect to differ — an
absent key inserts the key-column value under both spellings:
  present non-null -> false / false
  present Null     -> true  / true
  absent           -> true  / true

Verified: `main` exits 101 on this file, this branch exits 0.
@EnRaiha
EnRaiha force-pushed the fix/planner-nonminimal-bool branch 3 times, most recently from 5a19092 to 67b24a7 Compare September 29, 2026 00:50
The fill guard decides whether a synthesised identity is written back
under the key column. Its condition must mirror the identity extractor:
fill on an absent or NULL key, leave every present value alone.

Four cases pin that split. Two bind the fill direction: a missing key
column and an explicit NULL both end with the identity under the key. Two
cover the "left alone" direction: a present key keeps its bytes, and an
empty key stays empty. The empty case binds the identity extractor's
classification of `""` rather than the guard, because the guard writes
`doc_id` and `doc_id` already equals `""` there.

No case is red on `main`. The guard is behaviour-identical there, because
`!is_some_and(!p)` and `is_none_or(p)` agree on every input, so the
previous commit changes the form only. A mutation that disables the guard
fails the two fill cases (exit 100). The two "left alone" cases do not
discriminate that mutation, and they cannot discriminate an always-fill
mutation either: for a present String the identity the fill writes is
that value's own string form.
@EnRaiha
EnRaiha force-pushed the fix/planner-nonminimal-bool branch from 67b24a7 to 77cb538 Compare September 29, 2026 02:05

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants