Skip to content

fix: bind SQL_TINYINT columns as SQL_C_SHORT to support signed values#483

Open
rendonvelez wants to merge 2 commits into
IBM:mainfrom
rendonvelez:fix/tinyint-negative-values
Open

fix: bind SQL_TINYINT columns as SQL_C_SHORT to support signed values#483
rendonvelez wants to merge 2 commits into
IBM:mainfrom
rendonvelez:fix/tinyint-negative-values

Conversation

@rendonvelez

Copy link
Copy Markdown

Note

Supersedes #479, which GitHub auto-closed when I force-pushed the amended commit and now refuses to reopen ("the branch was force-pushed or recreated"). Same branch and content; @kadler's review suggestion from #479 is already applied here (1fec882).

Problem

SQL_TINYINT result columns are bound as SQL_C_UTINYINT (unsigned char). When a driver exposes TINYINT as a signed type (MySQL, Impala, Hive, among others) and a row contains a negative value such as -1, the driver must convert a negative signed value into an unsigned C type, which fails with SQLSTATE 22003 (numeric value out of range). Any SELECT over such a column errors out in node-odbc, while the same query succeeds in other ODBC clients that bind signed or default C types.

Reproduction:

CREATE TABLE t (c TINYINT);
INSERT INTO t VALUES (-1);
const conn = await odbc.connect(connectionString);
await conn.query('SELECT c FROM t'); // → Error: [SQLSTATE 22003] on drivers with signed TINYINT

Why not just switch to SQL_C_STINYINT?

TINYINT signedness is driver-specific: SQL Server exposes TINYINT as unsigned (0–255), while MySQL/Impala/Hive expose it as signed (−128–127). Binding unconditionally signed would break values above 127 on SQL Server, exactly mirroring the current bug.

Instead, this PR binds SQL_TINYINT to SQL_C_SHORT — the same path already used for SQL_SMALLINT. SQLSMALLINT holds the full range of both signed and unsigned TINYINT, so both families of drivers work correctly with no data loss. Per the ODBC conversion rules, converting any TINYINT to a wider signed C integer type is always valid. This follows the same spirit as #164, which fixed the analogous signedness problem for SQL_BIGINT.

Additional fix included

TINYINT output parameters in callProcedure were bound as SQL_C_UTINYINT, but the result-conversion switch has no case for that C type, so they fell through to the SQL_C_CHAR default and were misread as strings. They now go through the existing SQL_C_SSHORT path, which is handled.

Changes

  • SQL_TINYINT columns: bind as SQL_C_SHORT with an SQLSMALLINT buffer (merged with the SQL_SMALLINT case).
  • SQL_TINYINT procedure parameters: bind as SQL_C_SSHORT (merged with the SQL_SMALLINT case).
  • Removed the now-unused SQL_C_UTINYINT fetch/convert/cleanup paths and the tinyint_data union member.

Net: −26/+4 lines. All translation units compile cleanly (clang++ -fsyntax-only).

The original failure was observed through the Cloudera Impala ODBC driver (signed TINYINT column containing -1, failing with SQLSTATE 22003 on every SELECT). I'll report back with results from a patched build against that environment.

SQL_TINYINT columns and procedure parameters were bound as
SQL_C_UTINYINT. When a driver returns a signed TINYINT with a negative
value (e.g. MySQL, Impala, Hive), the conversion to an unsigned C type
fails with SQLSTATE 22003 (numeric value out of range), so any SELECT
over such a column errors out.

Bind TINYINT to SQL_C_SHORT/SQL_C_SSHORT instead, the same path already
used for SQL_SMALLINT. SQLSMALLINT covers the full range of both signed
(-128..127) and unsigned (0..255) TINYINT, so drivers that expose
TINYINT as unsigned (e.g. SQL Server) keep returning values above 127
correctly.

This also fixes TINYINT output parameters in callProcedure, which had
no SQL_C_UTINYINT case in the result conversion and fell through to
the SQL_C_CHAR default.

Signed-off-by: Gustavo Rendón Vélez <22383219+rendonvelez@users.noreply.github.com>
@rendonvelez

Copy link
Copy Markdown
Author

Validation results, as promised. Environment: Cloudera Impala ODBC driver (64-bit) on Windows x64, Node 20, against a production Impala cluster.

CREATE TABLE t (c TINYINT);
INSERT INTO t VALUES (-1), (0), (1);
SELECT c FROM t;

With this branch:

[
  { c: -1 },
  { c: 0 },
  { c: 1 },
  ...
  columns: [ { name: 'c', dataType: -6, dataTypeName: 'SQL_TINYINT', columnSize: 3, decimalDigits: 0, nullable: true } ]
]

Also verified SELECT CAST(-1 AS TINYINT) returns -1 on every row. The same queries on the released package fail with SQLSTATE 22003 before returning any row. Sign preserved, no conversion errors — from my side this is ready.

@abmusse abmusse added the enhancement New feature or request label Jul 2, 2026
@kadler

kadler commented Jul 2, 2026

Copy link
Copy Markdown
Member

Ok, I checked the code for other references to TINYINT and I noticed this reference was not removed for the INOUT case: https://github.com/rendonvelez/node-odbc/blob/1fec882e7c343371d3d828ab13db2c19686b6d87/src/odbc_connection.cpp#L1516-L1526

I think this should go with the SQLSMALLINT code path, but TBH I have a hard time following the INOUT codepath. It looks like it's completely hosed for INOUT. AFAICT the input value will be converted in ODBC::StoreBindValues to SQLBIGINT or SQLDOUBLE depending on the number (or something else if they pass in a string, bool, etc). Then the INOUT TINYINT code path will override the buffer size to 1, but didn't change the bind type. I think the only saving grace here is that we're not binding as character or binary types, so the ODBC spec says the buffer size is ignored. With that logic, I think we can just combine it with the SQLSMALLINT codepath there as well.

@kadler

kadler commented Jul 2, 2026

Copy link
Copy Markdown
Member

There is also extraneous TINYINT references in a comment here that can be cleaned up: https://github.com/IBM/node-odbc/blob/main/src/odbc_connection.cpp#L3905-L3906

…comments

For SQL_PARAM_INPUT_OUTPUT parameters, the TINYINT case overrode
BufferLength to sizeof(SQLCHAR) without changing the value type that
ODBC::StoreBindValues had already chosen (SQL_C_SBIGINT/SQL_C_DOUBLE for
JS numbers). Route it through the same path as SQL_SMALLINT, which
handles the numeric binding and the character-buffer resize.

Also remove the stale SQL_C_STINYINT/SQL_C_TINYINT entries (listed
twice) from the unhandled-C-types comment, since TINYINT columns no
longer bind to those types.

Addresses review feedback.

Signed-off-by: Gustavo Rendón Vélez <22383219+rendonvelez@users.noreply.github.com>
@rendonvelez

Copy link
Copy Markdown
Author

Both addressed in 4931ccf:

  • INOUT case: merged SQL_TINYINT into the SQL_SMALLINT codepath. Your reading matches mine — StoreBindValues has already picked SQL_C_SBIGINT/SQL_C_DOUBLE for JS numbers by that point, and the old TINYINT case only overrode BufferLength to 1 without touching the bind type, so for numeric C types that override was ignored anyway. Going through the SMALLINT path also gives the SQL_C_CHAR fallback a properly sized buffer (7 bytes covers -128\0) instead of a 1-byte one, which looks like it was a latent overflow for string-typed INOUT tinyints.
  • Comment cleanup: removed the stale SQL_C_STINYINT/SQL_C_TINYINT entries from the unhandled-types TODO (they were listed twice, and TINYINT columns no longer bind to those types at all).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants