#447 follow-ups: option-column type checking is Test-only, and dedup runs after the server LIMIT

[BorisTyshkevich]

Rewritten 2026-07-27 after a code review against main. The original body was
written during #447 phase 2 and is now wrong in both halves — item 2 is
implemented (only its regression test is missing), and item 1's citation names a
file that no longer exists, though the defect it describes is real and open.

1. The two-String-column rule is enforced on ONE path, and the batch still cannot check it — OPEN

Corrected citation. The original said the only caller is "the variable editor's
Test action (ui/variable-editor.ts)". That file was deleted by #457 along with
the option-SQL drawer and its Test action. The check was then re-hosted by #465
— shipped in PR #493 (fix(#465): validate a dashboard-variable tab's Run as a variable option query, merged 2026-07-27, a0417ed; #465 closed as completed) —
on the dashboard-variable tab's ordinary Run:

• src/ui/workbench/workbench-session.ts:441-448 — optionSqlDiagnostics(sql) runs
  first and sends nothing when the text is already knowably wrong;
• :475 — the SQL executes through compileOptionProbe(sql), not raw, so Run cannot
  pass for SQL the batch would reject (compileOptionProbe shares nestBounded with
  compileVariableOptionBatch);
• :487-489 — on a clean transport, validateOptionColumns(result.columns) and its
  message becomes the result's error.

The asymmetry is unchanged. readVariableOptionBatch
(src/core/variable-options.ts:413) checks only that the merged response has the
three columns the compiler projects, because UNION ALL reports ONE merged column
list for every branch — names from the first branch, types promoted to a supertype —
so per-branch column types are not visible in the response at all. The code now
documents this deliberately at variable-options.ts:351-361 and :310-322.

Consequence, still live: SELECT id, name FROM countries with id UInt64 runs fine
in the refresh batch and its options are offered (the transport stringifies every
cell), while Run correctly reports "Both option columns must be String". Run is
stricter than the runtime, and a user can "fix" SQL that was working.

What is missing

Nothing warns the user before they hit the difference. There is no proactive hint
about the contract anywhere in the variable tab — the strings "two columns" /
"both … String" exist only inside the two diagnostics, i.e. after a failure.

Options, unchanged from the original filing:

1. Accept the asymmetry and state it in the variable tab's hint — cheapest, and
   consistent with the spec designating the single-variable path as the diagnostic
   path. The code already documents the asymmetry; this only surfaces it.
2. Make Run as lenient as the runtime — weakens the contract Replace curated Dashboard filters with inferred Variables and batched option queries #447 states.
3. Project a per-branch type witness into the batch (an earlier draft used
   toTypeName(tuple(*))) — it works, but costs a fourth column on every row of
   every refresh, and was cut as engineering past the spec.

Recommended: option 1.

2. Duplicate collapsing vs. the server-side LIMIT — IMPLEMENTED, test missing

The original concern was that duplicates collapse client-side after each branch's
server LIMIT 1001, so SELECT country, country FROM visits could return an
arbitrary short list presented as complete, with no truncation warning because
the collapsed count lands under the cap.

That is fixed on main, by the detection route this issue proposed — but it
shipped under an unrelated issue number, which is why it looks untouched.

Provenance (why nobody could find this)

Commit	Merged as	Issue	What it did
16cabe7	#447 phase 2	#447	Introduced the truncation flag, derived from the kept count — the state this issue was filed against
249f7a0	PR #469 (feat(#468): restore the searchable multi-select for Array(scalar T) Dashboard variables, merged 2026-07-26)	#468	fix(#468): close three selection-destroying paths found in review — switched the flag to the raw count. This is item 2's actual fix.

So item 2 was resolved as collateral in a review of an unrelated issue — one of
#468's "three selection-destroying paths." #468's body never mentions #461, PR #469's
title does not either, and this issue was never updated. The only trace linking
the fix back here is the (#461) reference inside the code comment at
src/core/variable-options.ts:436.

That also explains the missing test below: the fix arrived under #468's
selection-preservation lens (don't prune a committed selection against a partial
list) rather than under this issue's completeness lens, so it got a test for #468's
concern and none for #461's own collapse case.
readVariableOptionBatch (src/core/variable-options.ts:432-441) counts RAW rows
before the blank/duplicate filters and flags from that count:

const raw = rawCount.get(name)! + 1;
rawCount.set(name, raw);
if (raw > VARIABLE_OPTION_CAP) truncated.add(name);

Its comment cites this issue by number and states the reasoning exactly: "a query
returning 1,001 rows of 500 distinct values is still an incomplete list".

It also reaches the user, which the original did not ask for:

• dashboard-viewer-session.ts:1061-1066 raises a variable-options-truncated
  diagnostic naming the affected variables — "Narrow the option SQL to see the rest."
• :1071 passes the flag into applyOptions, so a committed selection is not pruned
  against a list known to be incomplete.

The one thing left: make it falsifiable

No test covers the collapsing case, and this issue is cited in no test. The only
truncation test — tests/unit/variable-options.test.ts:427 "caps each variable
independently and reports which were truncated" — pushes VARIABLE_OPTION_CAP + 5
rows of distinct values (v${i}), so raw == kept and it cannot distinguish
raw-count logic from kept-count logic.

Concretely: swapping line 441 for a kept-count test such as
if (options.length >= VARIABLE_OPTION_CAP) still passes the whole existing suite,
while silently restoring the exact bug this issue reported.

Add one regression test: a branch returning VARIABLE_OPTION_CAP + 1 raw rows
that collapse to a handful of distinct values must still land in truncated.
Optionally assert the variable-options-truncated diagnostic in
dashboard-viewer-session.test.ts for the same shape. Cite #461 in the test
name, so the next reader does not have to reconstruct the provenance table above.

Scope

Two small, independent changes:

• a hint in the variable tab stating the two-String-column contract (item 1,
  option 1);
• the collapse-case regression test, and optionally its viewer-session counterpart
  (item 2).

Neither blocks #447 phase 3.

[BorisTyshkevich]

Body rewritten after a code review against main (2026-07-27). Recording what
changed and why, since the original text is no longer visible.

Item 2 (dedup after the server LIMIT) is implemented. It was fixed by the
detection route this issue proposed, and the fix cites this issue by number:
readVariableOptionBatch counts RAW rows before the blank/duplicate filters and
derives truncated from that count (src/core/variable-options.ts:432-441), so a
branch returning 1,001 rows that collapse to a few distinct values is still reported
as incomplete. It also surfaces — dashboard-viewer-session.ts:1061-1066 raises a
variable-options-truncated diagnostic naming the variables, and :1071 hands the
flag to applyOptions so a committed selection is not pruned against a list known to
be partial. That last part is more than this issue asked for.

One residual, and it is the actionable half: no test falsifies it. The only
truncation test (tests/unit/variable-options.test.ts:427) uses
VARIABLE_OPTION_CAP + 5 distinct values, so raw == kept there. Replacing the
raw-count check with a kept-count one still passes the entire suite while restoring
the reported bug. A regression test for the collapsing case is what closes item 2.

Item 1 (two-String-column rule) is still open, but its citation was wrong. The
original named ui/variable-editor.ts's Test action as the only caller; #457 deleted
that drawer and Test with it, and #465 re-hosted the check on the dashboard-variable
tab's ordinary Run (workbench-session.ts:441-448, :475, :487-489). The
defect is unchanged: the refresh batch still cannot check per-branch column types,
because UNION ALL reports one merged column list, so Run stays stricter than the
runtime and a user can "fix" SQL that was working. Nothing warns them in advance —
the contract's wording appears only inside post-failure diagnostics.

The three options are carried over verbatim, with option 1 (state the contract in
the variable tab's hint) recommended: the code already documents the asymmetry
deliberately (variable-options.ts:351-361), so this is about surfacing it, not
re-litigating it.

Net scope is now two small independent changes — a tab hint, and a regression test.
Neither blocks #447 phase 3.

Verified by reading the code, not by running the app.