Skip to content

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

Closed
rendonvelez wants to merge 1 commit into
IBM:mainfrom
rendonvelez:fix/tinyint-negative-values
Closed

fix: bind SQL_TINYINT columns as SQL_C_SHORT to support signed values#479
rendonvelez wants to merge 1 commit into
IBM:mainfrom
rendonvelez:fix/tinyint-negative-values

Conversation

@rendonvelez

Copy link
Copy Markdown

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.

@kadler kadler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically I think we could handle this by binding to SQL_C_TINYINT and then calling SQLGetInfo(hstmt, SQL_TINYINT) to determine whether we should convert from a signed or unsigned char. But this solution is much simpler.

Comment thread src/odbc_connection.cpp Outdated
Comment on lines +3396 to +3399
// Bind SQL_TINYINT to SQL_C_SHORT: SQLSMALLINT can hold the full range
// of both signed (-128 to 127) and unsigned (0 to 255) TINYINT columns,
// so negative values don't fail conversion with SQLSTATE 22003 and
// unsigned values above 127 are preserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Bind SQL_TINYINT to SQL_C_SHORT: SQLSMALLINT can hold the full range
// of both signed (-128 to 127) and unsigned (0 to 255) TINYINT columns,
// so negative values don't fail conversion with SQLSTATE 22003 and
// unsigned values above 127 are preserved.
// Bind SQL_TINYINT to SQL_C_SHORT: SQLSMALLINT can hold the full range
// TINYINT columns, which could be signed (-128 to 127) or unsigned
// (0 to 255) depending on the DBMS. This ensures negative values don't
// fail conversion with SQLSTATE 22003 and unsigned values above 127 are
// preserved.

Reworded a bit to be more explicit that the signedness varies based on the DBMS in use.

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 rendonvelez closed this Jul 2, 2026
@rendonvelez rendonvelez force-pushed the fix/tinyint-negative-values branch from e5ce19f to c4a4c86 Compare July 2, 2026 18:45
@rendonvelez

Copy link
Copy Markdown
Author

@kadler Applied your suggested rewording in c4a4c86 (with a small grammar tweak: "the full range of TINYINT columns"). Thanks for the review! Agreed that querying the driver's signedness via SQLGetInfo would be the fully precise route, but binding to SQL_C_SHORT covers both cases without the extra round trip.

Note on CI: the windows-latest failure is the pre-existing node-gyp "find VS" issue on the runner image (macOS/Linux built fine) — looks like #480 addresses it by moving to windows-2022, so a re-run after that lands should go green.

@kadler

kadler commented Jul 2, 2026

Copy link
Copy Markdown
Member

Looks like you closed the PR before it was merged. Are you waiting for #480 to be merged before rebasing and re-opening?

@rendonvelez

Copy link
Copy Markdown
Author

@kadler Sorry for the confusion — the close wasn't intentional. When I applied your suggestion I amended and force-pushed from a shallow clone, which produced a parentless (orphan) commit; GitHub auto-closed the PR on that push and now refuses to reopen it ("the branch was force-pushed or recreated"), even with the branch restored.

So the plan is exactly what you suggested: I've rebuilt the commit with correct history (same tree, your rewording applied), and once #480 lands on main I'll open a fresh PR from this branch so the checks run green on windows-2022. I'll link it here. In the meantime I'm validating the patched build against a live Impala datasource and will include the results in the new PR.

@kadler

kadler commented Jul 2, 2026

Copy link
Copy Markdown
Member

Ahh, that makes sense. Learning multiple new things today (github closing PRs and TINYINT signedness varies).

@rendonvelez

Copy link
Copy Markdown
Author

#480 has landed, so as promised the fresh PR is up: #483 (same branch, review suggestion applied, proper history). Closing the loop here — thanks for the patience @kadler!

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.

2 participants