Skip to content

fix(sql): keep one cell per duplicate constant column - #327

Merged
farhan-syah merged 1 commit into
NodeDB-Lab:mainfrom
EnRaiha:fix/constant-row-cell-keys
Sep 17, 2026
Merged

farhan-syah merged 1 commit into
NodeDB-Lab:mainfrom
EnRaiha:fix/constant-row-cell-keys

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Summary

SELECT nextval('a'), nextval('a') legally repeats an output name. The constant-result row is a JSON object keyed by column name, so the second cell overwrote the first and both wire columns rendered the last value. The payload and the output schema's lookup keys now use the same unique per-column keys every response encoder derives.

Red proof (the test fails on main without the fix)

────────────
     Summary [  10.891s] 1 test run: 0 passed, 1 failed, 10256 skipped
  TRY 2 FAIL [   4.311s] (1/1) nodedb::wire cases::sql_sequences::duplicate_constant_column_names_keep_their_own_cells
error: test run failed

Validation (local, on this branch)

Check Result
the new test, with the fix 1 passed
related suites: sql_sequences + cell_keys + output_schema 53 passed
cargo fmt --all no change
cargo clippy -p nodedb --all-targets --all-features --profile ci -- -D warnings clean
repository hygiene gate (test registry, comment and commit hygiene, module contracts) pass

One commit; touches sql_plan_convert/output_schema/build.rs, sql_plan_convert/set_ops.rs, and the new wire test only.

Copilot AI lite review requested due to automatic review settings September 16, 2026 06:42

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.

@farhan-syah farhan-syah added the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 17, 2026
`SELECT nextval('s'), nextval('s')` legally repeats an output name. The
constant-result row is a JSON object keyed by column name, so the second
cell overwrote the first and both wire columns rendered the last value.

Key the payload and the output schema's lookup keys by the same unique
per-column keys every response encoder derives, so each column keeps its
own cell.
@EnRaiha EnRaiha added run-ci Opt this PR into the full test suite; re-add to force a re-run and removed run-ci Opt this PR into the full test suite; re-add to force a re-run labels Sep 17, 2026
@EnRaiha
EnRaiha force-pushed the fix/constant-row-cell-keys branch from 55a42e7 to 22a0f42 Compare September 17, 2026 06:10

@farhan-syah farhan-syah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approve.

Verified statically against the branch ref (no checkout):

Claim Result
cargo fmt --all no change Confirmed. rustfmt --check clean on all three files.
One commit, three files Confirmed. 22a0f42, +59 / −13.
Test fails on main Consistent with the code. On main both cells land under nextval, so the row holds one value and both columns read it.
Comment hygiene Clean. No issue numbers, links, or pre-fix narration.

Layer is correct. Both producers of the constant-result shape (set_ops.rs payload, build.rs schema) now derive keys from cell_keys, the one function every encoder already reads through. No fallback, no second accepted shape.

Out of scope for this PR, tracked separately: the extended-query path (pgwire/handler/prepared/execute.rs) rebuilds its projection from the Describe fields with lookup_key = field name, and that projection overrides the planner schema (routing/execute.rs). SELECT nextval('s'), nextval('s') over Bind/Execute still collapses, as does SELECT w.id, b.id. That defect predates this PR.

@farhan-syah
farhan-syah merged commit 6b22531 into NodeDB-Lab:main Sep 17, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-ci Opt this PR into the full test suite; re-add to force a re-run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants