ClickHouse / ClickHouse/clickhouse-js
readVariant should return the active type discriminant, not just the value
- Dominant language
- TypeScript
- Stars
- 331
- Forks
- 74
- PR merge metrics
- No merged PRs in 30d
Description
## Summary
`readVariant` in `@clickhouse/rowbinary` (`skills/clickhouse-js-node-rowbinary-parser/src/composite.ts`) reads the 1-byte discriminant to pick which alternative reader to run, then returns **only the decoded value** and discards the discriminant. For most `Variant`s the active alternative can be inferred from the JS value's shape, but for some it cannot — two alternatives can decode to an identical JS value yet correspond to different ClickHouse types, so a consumer cannot tell which one it got.
## Why it matters
A consumer (e.g. a text serializer, or any user inspecting decoded values) needs the active type to format/interpret the value correctly. Verified against a live server (26.1):
| Variant | value | decoded JS (both alternatives) | correct ClickHouse text |
| --- | --- | --- | --- |
| `Variant(UInt8, Enum8('a'=1,'b'=2))` | `1` | `number 1` (identical) | `1` (UInt8) vs `a` (Enum8) |
| `Variant(Date, DateTime)` | midnight | `Date(…T00:00:00)` (identical) | `2020-01-02` vs `2020-01-02 00:00:00` |
In both rows the decoded value is indistinguishable, so without the discriminant there is no way to recover the right interpretation. (Cases like `Variant(String, UInt64)` or `Variant(Int32, Int64)` are unaffected — the shapes differ or the text is identical.)
These specific collision patterns (a numeric type beside an `Enum`, or `Date` beside `DateTime`/`DateTime64`) are **rare** — they don't appear in any of the upstream `0_stateless` tests that use `Variant` — so this is a **correctness/robustness** fix for arbitrary user input, not a coverage blocker.
## Proposed change
Have `readVariant` surface the active alternative index alongside the value, e.g.
```ts
// today
export function readVariant(readers): Reader
// proposed (shape TBD)
export function readVariant(readers): Reader<{ typeIndex: number; value: unknown } | null>
// (NULL discriminant 0xFF -> null, as today)
```
The reader already knows the index (it dispatches on the discriminant), so this is just threading it through. Consumers that don't care can ignore `typeIndex`.
## Notes / considerations
- **Breaking change** to `readVariant`'s return type — should land with a minor/major bump and a changelog note.
- The type-AST → reader fold (`astToReader` in `compile.ts`) maps the discriminant index to `node.arguments[typeIndex]`; a renderer can then format with the exact subtype.
- Discriminant order: alternatives are sorted by type name (ClickHouse's global ordering), which matches the header's argument order — see the existing note in `variantReader` (`compile.ts`).
Found while scoping a `Variant` renderer for the upstream-SQL test harness (`tests/clickhouse-test-runner`), where it surfaced as the one case a value-only decode can't render correctly.
Contributor guide
Research direction
Start in skills/clickhouse-js-node-rowbinary-parser/src/composite.ts at readVariant, then read variantReader and astToReader in compile.ts to understand discriminant ordering. Inspect tests/clickhouse-test-runner, then define and implement the return shape so non-NULL results retain the active index while NULL remains null, updating the breaking-change documentation and relevant coverage.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- nodejs, typescript
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100