Skip to content

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

Description

@peter-leonov-ch

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 Variants 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.

// today
export function readVariant(readers): Reader<T[number] | null>
// 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions