Skip to content

fix(bind): honour the arrow.json tag through dictionary encoding; test null-in-dictionary-values - #308

Merged
fornwall merged 1 commit into
mainfrom
dictionary-json-and-null-value
Jul 15, 2026
Merged

fix(bind): honour the arrow.json tag through dictionary encoding; test null-in-dictionary-values#308
fornwall merged 1 commit into
mainfrom
dictionary-json-and-null-value

Conversation

@fornwall

Copy link
Copy Markdown
Owner

Two follow-ups from reviewing fornwall/arrow-adbc#34 (dictionary bind support in the C SQLite/PostgreSQL drivers) against this driver. Our bind.rs already uses the delegate-to-the-plain-path design that PR retrofits — the key selects the dictionary value, which re-enters the same cell_value mapping — so its whitelist-drift bugs can't occur here by construction. The comparison surfaced one genuine gap and one untested-but-correct edge case:

JSON through dictionary encoding

is_json_field required the field's own data type to be utf8-family, but the Arrow spec allows an extension array to be dictionary-encoded (the field's storage type is then dictionary<indices, utf8>). So a JSON-tagged Dictionary(Int32, Utf8) column — e.g. a pandas categorical of JSON strings — silently drifted from its plain form:

  • it bound as plain STRING, which Spanner refuses to coerce into a JSON column, and
  • ingest create modes made a STRING(MAX) column instead of JSON.

is_json_field now looks through Dictionary(_, storage) for the storage check. Present cells then get the explicit JSON param type for free via the existing plain-path delegation; null_dictionary_value now takes the field so a null cell keeps the type too (the plain path's typed-null rule). The tag stays ignored on non-string dictionary values (Dictionary(_, Int64)), matching the plain-storage rule.

Regression test: non-null key → null dictionary value

A dictionary cell can be null two ways: a null key, or a valid key selecting a null entry inside the values array. All existing dictionary tests only exercised the first. The second is handled correctly today, but only as a side effect of the delegation design (the delegated scalar binder's is_null check fires on the values array) — and it is exactly the case that stayed latent for years in the ADBC postgres driver's already-supported string dictionaries (insufficient data left in message, found in fornwall/arrow-adbc#34). The new test locks it in so a future restructuring of the dictionary arm fails the suite instead of reintroducing that bug.

No wire-format or option changes; cargo test, clippy -D warnings and fmt --check are clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BgsLAmXUPFThybc8nfF4Eh

…t null-in-dictionary-values

Two follow-ups from reviewing fornwall/arrow-adbc#34 (dictionary bind
support in the C SQLite/PostgreSQL drivers) against this driver's
delegate-to-the-plain-path dictionary binding:

- `is_json_field` now looks through `Dictionary(_, storage)` when
  checking for utf8-family storage: the Arrow spec allows an extension
  array to be dictionary-encoded, so a JSON-tagged
  `Dictionary(Int32, Utf8)` column previously bound as plain STRING
  (which Spanner refuses to coerce into a JSON column) and ingest
  create modes made a STRING(MAX) column instead of JSON. Present
  cells get the JSON param type via the existing plain-path
  delegation; `null_dictionary_value` now takes the field so a
  null cell keeps the type too (the plain path's typed-null rule).

- Regression test for a non-null key pointing at a null entry inside
  the dictionary values array — the second way a dictionary cell can
  be null. Handled correctly today only as a side effect of the
  delegation design; that exact case stayed latent for years in the
  ADBC postgres driver's already-supported string dictionaries.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BgsLAmXUPFThybc8nfF4Eh
@fornwall
fornwall merged commit 8305786 into main Jul 15, 2026
18 checks passed
@fornwall
fornwall deleted the dictionary-json-and-null-value branch July 15, 2026 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant