GH-50560: [C++][FlightRPC][ODBC] Fix SQLDescribeCol column_size/decimal_digits width#50562
Conversation
|
|
github-actions
Bot
added
Component: C++
Component: FlightRPC
awaiting review
I think we reserve this for things that would be a release blocker, FWIW. CC @justing-bq for review |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect SQLDescribeCol output widths in the Flight SQL ODBC driver so callers receive fully-initialized column_size for numeric/decimal types (and correct buffer-size arguments for decimal_digits), addressing GH-50560 and preventing nondeterministic garbage in upper bytes.
Changes:
- In
SQLDescribeCol, readsSQL_DESC_PRECISIONinto aSQLSMALLINTand widens intoSQLULENto avoid short writes intocolumn_size. - Corrects
decimal_digitsGetFieldbuffer sizes tosizeof(SQLSMALLINT)for scale / datetime precision paths. - Adds a new gtest file to validate
column_sizeis fully overwritten (and wires it into the ODBC test CMake target).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/src/arrow/flight/sql/odbc/tests/describe_col_test.cc | Adds regression tests intended to catch short writes into SQLULEN column_size and validate decimal_digits. |
| cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt | Includes the new describe_col_test.cc in the ODBC test sources. |
| cpp/src/arrow/flight/sql/odbc/odbc_api.cc | Fixes write-width mismatch for numeric column_size and corrects buffer-size arguments for decimal_digits GetField calls. |
…/decimal_digits width SQLDescribeCol read SQL_DESC_PRECISION (a 2-byte SQLSMALLINT) directly into the caller's 8-byte SQLULEN* column_size output for numeric/decimal columns. The GetAttribute template writes exactly sizeof(T) bytes and ignores the claimed buffer_length, so only the low 2 bytes were written and the upper 6 bytes were left uninitialized garbage. Read the precision into a local SQLSMALLINT and widen-assign it into *column_size_ptr so all 8 bytes are written. Also fix the two decimal_digits sites (SQL_DESC_SCALE and datetime SQL_DESC_PRECISION) to pass sizeof(SQLSMALLINT) -- the true width of the SQLSMALLINT* decimal_digits output -- instead of sizeof(SQLULEN). Add describe_col_test.cc, which pre-fills column_size with an all-ones sentinel and asserts SQLDescribeCol fully overwrites it for DECIMAL and TIMESTAMP columns. Introduced by apacheGH-47724 (apache#48052). Signed-off-by: Vikrant Puppala <vikrantpuppala@gmail.com>
5b1944f to
1691e5c
Compare
|
@lidavidm Dropped the "Critical Fix" wording from the description |
|
@justing-bq can I get a review here please? |
| // for a DECIMAL column. Prior to GH-50560 the numeric path read | ||
| // SQL_DESC_PRECISION (a SQLSMALLINT) straight into the SQLULEN* output, writing | ||
| // only 2 of the 8 bytes and leaving the upper 6 bytes as uninitialized garbage. | ||
| TEST_F(DescribeColRemoteTest, TestSQLDescribeColDecimalColumnSizeIsFullyWritten) { |
There was a problem hiding this comment.
I think it would be better for these tests to be defined in columns_test.cc where there are many other SQLDescribeCol tests.
| case SQL_DECIMAL: | ||
| case SQL_NUMERIC: { | ||
| // decimal_digits_ptr is a SQLSMALLINT*; SQL_DESC_SCALE is stored as a | ||
| // SQLSMALLINT. Pass the matching buffer size. |
There was a problem hiding this comment.
While this comment helps explain the reasoning behind the change to anyone looking at this PR, I don't think this comment is particularly helpful after the change gets merged.
| case SQL_INTERVAL_HOUR_TO_SECOND: | ||
| case SQL_INTERVAL_DAY_TO_SECOND: { | ||
| // decimal_digits_ptr is a SQLSMALLINT*; SQL_DESC_PRECISION is stored as | ||
| // a SQLSMALLINT. Pass the matching buffer size. |
There was a problem hiding this comment.
Likewise for this comment.
Rationale for this change
SQLDescribeColreturned uninitialized garbage in the upper bytes of itscolumn_size(ColumnSizePtr) output for numeric / decimal / integer / floatcolumns.
The numeric path read
SQL_DESC_PRECISION— stored internally as aSQLSMALLINT(2 bytes) — directly into the caller'sSQLULEN* column_size_ptr(8 bytes). The
GetAttributetemplate that ultimately performs the write copiesexactly
sizeof(T)bytes (hereT = SQLSMALLINT, so 2 bytes) and ignores theclaimed
buffer_length. The result: only the low 2 bytes of the 8-byteSQLULENwere written and the upper 6 bytes were left uninitialized. Anyapplication reading the full
column_sizesaw a wildly incorrect value.Two adjacent
decimal_digitssites (SQL_DESC_SCALEfor exact-numeric columns,SQL_DESC_PRECISIONfor datetime/interval columns) passedsizeof(SQLULEN)asthe buffer size even though
decimal_digits_ptris aSQLSMALLINT*. Those didnot corrupt memory (the write width matched the 2-byte target), but the size
argument was wrong and misleading.
Introduced by GH-47724 (#48052).
What changes are included in this PR?
column_sizepath: readSQL_DESC_PRECISIONinto a localSQLSMALLINTand widen-assign it into*column_size_ptr, so all 8 bytes ofthe
SQLULENoutput are written.decimal_digitsscale and datetime-precision paths: passsizeof(SQLSMALLINT)(the true target width) instead ofsizeof(SQLULEN).describe_col_test.ccwith parameterized (mock + remote) tests thatpre-fill
column_sizewith an all-ones sentinel, callSQLDescribeColon aDECIMALcolumn and aTIMESTAMPcolumn, and assert the full 8-bytecolumn_sizeis a sane small value (i.e. the upper bytes were overwritten),plus sane
decimal_digits. Without the fix these fail because the sentinel'supper bytes survive the short write.
Are these changes tested?
Yes — new gtest cases in
cpp/src/arrow/flight/sql/odbc/tests/describe_col_test.cc, wired into the ODBCtest
CMakeLists.txt.Are there any user-facing changes?
Yes.
SQLDescribeColnow returns a correctcolumn_sizefor numeric/decimalcolumns instead of a value with uninitialized high bytes. This is a bug fix in
externally-visible ODBC behavior; no API signatures change.