fix(c/driver): support binding dictionary-encoded columns of any plainly-supported value type - #34
Open
fornwall wants to merge 4 commits into
Open
fix(c/driver): support binding dictionary-encoded columns of any plainly-supported value type#34fornwall wants to merge 4 commits into
fornwall wants to merge 4 commits into
Conversation
Dictionary encoding is an encoding of the same logical values, not a distinct logical type, so a driver that can bind a plain column of type T should accept dictionary<values=T> and decode it. pandas produces these routinely: pd.Series([1, 2, 1], dtype="category") is a dictionary<values=int64, indices=int8>. The binder had a parallel whitelist of supported dictionary value types and a special-cased dictionary bind that re-implemented value binding. The two disagreed: binary, large_binary, fixed_size_binary and binary_view were whitelisted as supported, but the dictionary bind called sqlite3_bind_text unconditionally regardless of value type. Binary-valued dictionaries therefore landed in SQLite with text affinity and read back as strings instead of blobs. Non-string/binary value types (such as int64 and double) were rejected with NOT_IMPLEMENTED even though the plain types bind fine. Rather than extend the whitelist and the special case (which is how the binary/text divergence arose), resolve the dictionary index up front and then dispatch on the *value* type through the same switch a plain column uses. Binary values now bind as blobs by construction, and every value type the binder already supports plainly - including int64 and double - works through a dictionary for free. SqliteDictionaryParamTest covers int64, double and string values, and is parameterized over binary, large_binary, binary_view and fixed_size_binary as a regression test for the bind_text bug. The tests exercise repeated indices, unreferenced dictionary values, and nulls within the dictionary. The dictionary-encoding test helper is added to adbc_validation so the postgresql tests can share it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The COPY writer supported dictionary-encoded columns only when the dictionary values were binary/string/large_binary/large_string. Everything else - including int64 and double, both of which are supported first-class as plain columns - fell through to "COPY Writer not implemented for type". Dictionary encoding is an encoding of the same logical values rather than a distinct logical type, so pandas categoricals (pd.Series([1, 2, 1], dtype="category") is dictionary<values=int64, indices=int8>) could not be bound or ingested. Replace the value-type whitelist and the hand-rolled binary dictionary writer with a generic writer that resolves the index and delegates to a writer built for the dictionary's value type, so every value type supported plainly is supported dictionary-encoded by construction. PostgresType::FromSchema already resolves a dictionary to its value type when picking the parameter OID, so no type-mapping change is needed; target_type is passed through, which also makes dictionary values land correctly in jsonb and numeric columns. This also fixes a latent bug on the prepared-statement bind path, which affected the already-supported string/binary dictionaries: a dictionary index that is itself non-null but points at a null value *within* the dictionary was written as an inline -1 field length. That encoding is correct for COPY, but the extended query protocol signals a null parameter via a null param_values entry, so PostgreSQL saw a zero-length value and rejected it with "insufficient data left in message" (SQLSTATE 08P01). Resolve dictionary nulls in BindAndExecuteCurrentRow instead. Tests cover the COPY writer for dictionary-encoded binary, int64 and double, asserting the output is byte-for-byte identical to the plain column, plus an end-to-end AdbcStatementBind test that binds dictionary int64 and binary parameters and reads them back. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Nightly clippy lints can break CI without any code changes on our side. Recently nightly clippy panicked: https://github.com/apache/arrow-adbc/actions/runs/29331390016/job/87079916638?pr=4514 Mark the step as `continue-on-error` so failures are visible but non-blocking. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
- postgresql: resolve dictionary encoding before the NUMERIC/JSONB target-type dispatch in MakeCopyFieldWriter, so a dictionary-encoded column also works when the COPY target column is NUMERIC - sqlite: store the resolved value type directly in binder->types and detect dictionary columns from the array view, dropping the extra dictionary_types allocation on every bind - sqlite: declare dictionary-encoded columns with their value type's affinity in bulk-ingest CREATE TABLE - validation: reuse DictionaryEncodeColumn in TestSqlIngestType instead of a duplicated inline implementation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012rvNGKLcvRAvTZsFq9NZZC
fornwall
added a commit
to fornwall/adbc-spanner
that referenced
this pull request
Jul 15, 2026
…t null-in-dictionary-values (#308) 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. Claude-Session: https://claude.ai/code/session_01BgsLAmXUPFThybc8nfF4Eh Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DISCLAIMER: This PR was mostly AI generated. I have reviewed and is ready to take on follow up work to change or investigate things.
Context: While looking at adding a test for bound dictionary encoded parameters in the python validation suite (adbc-drivers/validation#256), I encountered this issue.
Summary
Binding a dictionary-encoded column as a query parameter was under-supported in the C SQLite and PostgreSQL drivers. Dictionary encoding is an encoding of the same logical values, not a distinct logical type, so a driver that can bind a plain column of type
Tshould acceptdictionary<values=T>and decode it. pandas produces these routinely —pd.Series([1, 2, 1], dtype="category")is adictionary<values=int64, indices=int8>.Both drivers had the same pattern: a whitelist of supported dictionary value types plus a special-cased dictionary writer that re-implemented value binding. In both cases the fix takes the delegate-to-the-plain-path approach — resolve the index, then dispatch on the value type through the code the plain column already uses. The missing types then come for free, by construction, rather than by growing a second whitelist that can drift from the first.
One commit per driver, plus a follow-up commit applying review findings (allocation-free type bookkeeping in the SQLite binder,
NUMERIC-target andCREATE TABLEaffinity fixes, test-helper dedup). The branch also carries an unrelated CI commit letting the nightly clippy step fail without blocking (the 2026-07-15 nightly ICEs on this workspace).1. SQLite (
statement_reader.c)The whitelist and the bind implementation had already drifted, which is exactly the bug this restructuring prevents:
binary/large_binary/fixed_size_binary/binary_viewwere whitelisted as supported, but the dictionary case calledsqlite3_bind_textunconditionally, regardless of value type. Binary-valued dictionaries landed in SQLite with text affinity and read back as strings instead of blobs.int64/doubledictionaries were rejected withNOT_IMPLEMENTED, though the plain types bind fine.Bulk ingest now also declares dictionary-encoded columns with their value type's affinity in
CREATE TABLE: previously such a column got no declared type at all (BLOB affinity), so e.g. a dictionary-encoded int column compared differently against string literals than the same column ingested plain.2. PostgreSQL (
copy/writer.h,bind_stream.h)The COPY writer supported dictionaries only for
binary/string/large_binary/large_string; everything else fell through to "not implemented".PostgresType::FromSchemaalready resolves a dictionary to its value type when picking the parameter OID, so no type-mapping change was needed.Dictionary resolution happens at the top of
MakeCopyFieldWriter, before theNUMERIC/JSONBtarget-type dispatch, so ingesting a dictionary-encoded integer/decimal column into an existingNUMERICcolumn delegates to the same target-type-aware writer the plain column gets.While adding the end-to-end bind test I found a pre-existing latent bug that also affects the already-supported string/binary dictionaries: an index that is itself non-null but points at a null value within the dictionary was written as an inline
-1field length. That is correct for COPY, but the extended query protocol signals a null parameter via a nullparam_valuesentry, so PostgreSQL saw a zero-length value and rejected it:Dictionary nulls are now resolved in
BindAndExecuteCurrentRow.Changes in support
stringlarge_stringstring_viewbinarylarge_binaryfixed_size_binarybinary_viewboolint8/int16/int32/int64uint8/uint16/uint32/uint64half_float/float/doubledate32timestamptime64decimal128/decimal256interval_month_day_nanodurationPossible follow-up: cache converted dictionary values
Delegating to the plain path means a value is converted once per row, not once per distinct dictionary value — so for conversion-heavy types the repetition that dictionary encoding exists to exploit is currently thrown away. A per-column cache keyed by dictionary index would convert once per distinct value instead. This is left out for now.
Applicable to exactly the types whose plain writer does real per-value work:
date32,timestampArrowDate32ToIsoString/ArrowTimestampToIsoString:malloc+ format +strlen+SQLITE_TRANSIENTcopy +free, per rowdecimal128,decimal256raw_decimal_string, re-parse into digit groups, allocate astd::vector<int16_t>, per rowEvery other type is already effectively free per value and would gain nothing: the sqlite string/binary paths hand SQLite a pointer into the dictionary buffer with
SQLITE_STATIC(zero copy), the int/float paths are a single read, and the postgres timestamp/interval/ duration/binary writers are plain arithmetic plus amemcpywith no allocation.What it would save:
O(k)conversions instead ofO(n), forkdistinct values overnrows. Since dictionaries are used precisely whenk << n, the saving scales with the compression ratio. It would not save the bind/write itself — SQLite binds per row and PostgreSQL COPY writes bytes per row regardless — so the ceiling is the conversion and allocation only.Footnotes
Worse than unsupported: these were accepted by the whitelist and then bound with
sqlite3_bind_text, so the write succeeded silently and the data was stored as TEXT. Non-UTF-8 binary (most binary) became unreadable —typeof()returnedtext,length()counted malformed UTF-8 characters rather than bytes, and reading the value back raised a decode error. ↩ ↩2 ↩3 ↩4Not supported as a plain column by this driver, so there is no plain path to delegate to and no dictionary support is added. These are unchanged. ↩ ↩2 ↩3 ↩4