Stringify numeric parameters bound to character columns (re: #163) by fdcastel · Pull Request #164 · duckdb/odbc-scanner

fdcastel · 2026-04-24T16:28:14Z

This is a first attempt at items 1 and 3 of the suggestions I filed
in #163. (Item 2 — bind-once-execute-many — is intentionally out of scope for
this PR; see notes at the end.)

⚠️ Full disclosure: this change was AI-assisted (Claude Code / Opus 4.7)
and I am very happy to iterate on it. If any of the choices below are wrong,
if the tests need more coverage, or if you would prefer a different shape
for the fix entirely — please say so and I will rework it.

What this changes

1. Stringify numeric → CHAR/VARCHAR in the scanner

Binding SQL_C_SLONG / SQL_C_SBIGINT / SQL_C_FLOAT / … to a SQL_VARCHAR
column forces the ODBC driver to convert numeric-C → character-SQL internally.
That conversion path is empirically one of the least-exercised corners of many
drivers: the Firebird ODBC driver ≤ 3.5.0 silently corrupted writes on it
(#161 / FirebirdSQL/firebird-odbc-driver#292), and older
MSSQL and MySQL ODBC releases have shipped similar defects.

Params::SetExpectedTypes now transforms TINYINT…BIGINT and FLOAT/DOUBLE
ScannerValues into TYPE_DECIMAL_AS_CHARS whenever the prepared parameter's
expected type is in the character family (SQL_CHAR / SQL_VARCHAR /
SQL_LONGVARCHAR / SQL_WCHAR / SQL_WVARCHAR / SQL_WLONGVARCHAR). The
scanner then binds SQL_C_CHAR → the actual expected character SQL type via
the existing DecimalChars path. This entirely bypasses the driver's
numeric-to-string coercion — no driver fix required.

BindOdbcParam<DecimalChars> also now honors the expected character SQL type
(it hard-coded SQL_VARCHAR before) so CHAR / WVARCHAR columns are not
silently re-coerced.

3. Regression test in `test_copy.cpp`

New TEST_CASE("Copy integer source into VARCHAR primary key", ...) copies
500 INTEGER rows through odbc_copy with batch_size=1 into a
VARCHAR(20) NOT NULL PRIMARY KEY column, then verifies both the row count
(via rows_processed) and the stored string values. Row-count alone would not
catch the original bug, which stored 500 corrupted (NUL-byte) values without
any driver error under one execute shape and collapsed to 11 distinct values
under another — so the test reads sampled values back and compares to the
source integers as decimal strings.

batch_size=1 deliberately forces the single-row bind path (SetExpectedTypes

BindToOdbc per row) which is the shape that exposed the original bug.
column_quotes='' keeps the INSERT column names unquoted so drivers that fold
unquoted identifiers to upper case (Oracle, DB2, Firebird) still match the
id column created in the DDL.

The test is skipped on:

FlightSQL / Spark / Snowflake / ClickHouse: no representative ad-hoc
DDL / INSERT path for the scenario.
MySQL / MariaDB: the CI connection strings do not select a default
database, so CREATE TABLE <name> fails at SQLPrepare with "No database
selected". The bug is not MySQL-specific and other drivers cover it.

CI

All driver jobs pass on my fork except for two pre-existing failures that
exist on main too and are unrelated to this diff:

MSSQL Linux: the MSSQL ODBC Lib step looks for
libmsodbcsql-18.6.so.1.1 but the currently-packaged driver installs
libmsodbcsql-18.6.so.2.1 (the ldd in the CI script then fails the job
at setup time, before any test runs).
Firebird Windows: test/sql/firebird/14_time_with_time_zone.test:30
asserts 12:45:00 for '15:45:00 Europe/London' but the CI is currently
returning 11:45:00 (looks DST-related).

Happy to file separate tickets / PRs for either of those if they are not
already on the radar.

What is not in this PR

Item 2 (bind-once-execute-many) is a real improvement idea, but my first
attempt broke test/sql/duckdb/duckdb_copy.test:46 on the DuckDB ODBC driver
(reusing bindings across executes without an intervening SQLFreeStmt(SQL_CLOSE)
raised an HY010 sequence error at rollback time). I reverted it from this PR
so the fix for item 1 is not blocked while I figure out a driver-aware
approach (probably an explicit SQLFreeStmt(SQL_CLOSE) between reused
executes, or a narrower opt-in per DbmsQuirks). Happy to open it as a
follow-up PR.
Items 4 (KNOWN_ISSUES.md) and 5 (post-commit row count validation):
intentionally left out of this PR. I can do them separately once the direction
here lands.

Thanks for looking — again, this is a starting point, please point at anything
you would like changed.

Binding SQL_C_SLONG/SQL_C_SBIGINT/SQL_C_FLOAT/... to a SQL_VARCHAR column forces the ODBC driver to convert numeric-C to character-SQL internally. That conversion path is one of the least-exercised corners of many drivers: the Firebird ODBC driver ≤ 3.5.0 silently corrupted writes on it (duckdb#161 / FirebirdSQL/firebird-odbc-driver#292), and older MSSQL and MySQL ODBC releases have shipped similar defects. Params::SetExpectedTypes now transforms TINYINT…BIGINT and FLOAT/DOUBLE ScannerValues into TYPE_DECIMAL_AS_CHARS whenever the prepared parameter's expected type is a character family (CHAR / VARCHAR / LONGVARCHAR / WCHAR / WVARCHAR / WLONGVARCHAR). The scanner then binds SQL_C_CHAR → the actual expected character SQL type via the existing DecimalChars path. This entirely bypasses the driver's numeric-to-string coercion — no driver bug required. BindOdbcParam<DecimalChars> also now honors the expected character SQL type (it hard-coded SQL_VARCHAR before) so CHAR / WVARCHAR columns are not silently re-coerced.

Guards against duckdb#161: copying integer source rows into a VARCHAR PK column silently dropped 489 / 500 rows on the Firebird ODBC driver because the scanner bound SQL_C_SLONG to the VARCHAR column and hit a driver bug in the numeric-C → character-SQL path. Every ODBC call returned SUCCESS, so a round-trip test that reads the stored VARCHAR back and compares to the source integer as a decimal string is the only way to spot a regression of this class. batch_size=1 forces the single-row bind path (the shape that exposed the original bug — per-row rebind amplified corruption into row loss). column_quotes='' keeps the INSERT column names unquoted so drivers that fold unquoted identifiers to upper case (Oracle, DB2, Firebird) still match the `id` column created in the DDL. The row-count assertion uses DECIMAL(18, 0) + DecimalValue<int64_t> — the one int64 accessor that works uniformly across drivers, since Result::Value<int64_t> routes through per-driver type expectations (Oracle's count(*) returns DECIMAL, DuckDB's BIGINT, etc.). Skips: - FlightSQL / Spark / Snowflake / ClickHouse: no representative ad-hoc DDL / INSERT path for the scenario. - MySQL / MariaDB: CI connection strings do not select a default database, so CREATE TABLE fails at SQLPrepare with "No database selected" — the bug is not MySQL-specific and other drivers cover it.

staticlibs

Thanks for the PR! In general it looks good to me, added one nit and one possible change.

1. Move Types::IsCharacterSQLType declaration out of the header — the definition lives in types.cpp alongside the other Types members. Also adds Types::IsWideCharacterSQLType to support (2). 2. For wide-character expected types (SQL_WCHAR / SQL_WVARCHAR / SQL_WLONGVARCHAR), stringify to UTF-16 and dispatch through the SQL_C_WCHAR binding path instead of SQL_C_CHAR. Some ODBC drivers do not reliably auto-convert SQL_C_CHAR into a wide-character column on bind, so we hand the driver wide bytes directly. ScannerValue::TransformNumericToChars gains a `wide` flag: the narrow path keeps the existing TYPE_DECIMAL_AS_CHARS representation; the wide path widens via WideChar::Widen and re-tags as DUCKDB_TYPE_VARCHAR so BindOdbcParam<std::string> picks it up and binds SQL_C_WCHAR. 3. Drop the C++ regression test in favor of SQLLogic tests added to each existing *_copy.test (db2, duckdb, firebird, mssql, oracle). Each test creates a VARCHAR primary-key column, copies 500 integers into it with batch_size=1 (the per-row rebind shape that exposed the original bug), and reads sampled values back to confirm they match the source integers as decimal strings. Per-driver DDL differences are folded into the respective test files rather than into C++ DBMSConfigured branches.

staticlibs · 2026-04-24T21:35:08Z

Thanks for the update! Added a comment there - sorry for being picky, just currently 2 code paths used for wide/narrow expected types look a bit confusing.

fdcastel · 2026-04-24T21:48:47Z

Not picky: thorough. Those are the most helpful kinds of reviews. Keep them coming. No worries at all! 🤗👍🏻

Drop the `bool wide` parameter from `ScannerValue::TransformNumericToChars` and always re-tag the value as TYPE_DECIMAL_AS_CHARS. Wide character targets (SQL_WCHAR / SQL_WVARCHAR / SQL_WLONGVARCHAR) are now handled inside `BindOdbcParam<DecimalChars>`, which widens the ASCII digits into a SQLWCHAR buffer stored on `DecimalChars` and binds `SQL_C_WCHAR` for the expected type. This keeps the integer/float binding dispatch on a single coalesced code path and validates the existing "if target is character, value has already been transformed to TYPE_DECIMAL_AS_CHARS" comments in integer_types.cpp and float_types.cpp for all character subtypes.

Applies the project's clang-format style to the new SQLBindParameter call added in the previous commit. No behavior change.

staticlibs

Thanks for the update! Apart from the 4 minor things commented above - it looks good to me!

Pulls the per-type stringification switch out of `TransformNumericToChars` into a private `ScannerValue::NumericToString()` returning `std::string`. Each case in the switch now returns directly, so a missed `break` / forgotten assignment can't silently leave the result empty.

…VARCHAR test - ScannerValue: add SetLengthBytes(SQLLEN) and use it in BindOdbcParam<DecimalChars> instead of assigning to the LengthBytes() reference (the getter-style assignment was confusing). - BindOdbcParam<DecimalChars>: pass dc.size<size_t>() directly to WideChar::Widen so the explicit static_cast<size_t> is no longer needed. - mssql_copy.test: extend the int-to-VARCHAR regression with an additional NVARCHAR(20) column so the wide (SQL_C_WCHAR) branch of BindOdbcParam<DecimalChars> is now covered by CI.

- BindSlotShape: drop the redundant `is_null` field — its value was always `type_id == DUCKDB_TYPE_SQLNULL`, so it added nothing to the equality check and one more thing to keep in sync. - IsFixedWidthShape: include `Params::TYPE_SQL_GUID` (raw `SQLGUID` in the union, address-stable across move-assignment) so GUID-keyed single-row inserts also hit the bind-once fast path. - Rename the helper to `BindToOdbcIfShapeUnchanged` and flip the return semantics to `bool reused` (true = cache hit, false = full rebind). This removes the awkward `bool did_bind = ...; reused_bindings = !did_bind;` inversion at the call site and matches the local `reused_bindings` flag. - Header comment: note that integer / float parameters whose target column is CHAR/VARCHAR/WCHAR are coalesced to TYPE_DECIMAL_AS_CHARS by SetExpectedTypes, so they intentionally stay on the rebind-per-row path. - duckdb_copy.test: bump the bind-once smoke test from 50 to 500 rows so the reuse path is exercised many times across multiple internal data chunks.

fdcastel added 2 commits April 24, 2026 13:27

staticlibs reviewed Apr 24, 2026

View reviewed changes

Comment thread src/include/types.hpp Outdated

Comment thread src/types/decimal_type.cpp

Comment thread test/test_copy.cpp Outdated

fdcastel marked this pull request as draft April 24, 2026 17:38

fdcastel marked this pull request as ready for review April 24, 2026 20:42

fdcastel mentioned this pull request Apr 24, 2026

odbc_copy: bind once per stable single-row shape (re: #163 item 2) #168

Merged

staticlibs reviewed Apr 24, 2026

View reviewed changes

Comment thread src/include/scanner_value.hpp Outdated

fdcastel added 2 commits April 24, 2026 19:09

format: clang-format wide-char BindOdbcParam branch

b8ffe8d

Applies the project's clang-format style to the new SQLBindParameter call added in the previous commit. No behavior change.