ClickHouse / ClickHouse/clickhouse-js

readVariant should return the active type discriminant, not just the value

Open
#910 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.