-
Notifications
You must be signed in to change notification settings - Fork 4.2k
GH-50560: [C++][FlightRPC][ODBC] Fix SQLDescribeCol column_size/decimal_digits width #50562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1581,8 +1581,14 @@ SQLRETURN SQLDescribeCol(SQLHSTMT stmt, SQLUSMALLINT column_number, SQLWCHAR* co | |
| case SQL_REAL: | ||
| case SQL_FLOAT: | ||
| case SQL_DOUBLE: { | ||
| ird->GetField(column_number, SQL_DESC_PRECISION, column_size_ptr, | ||
| sizeof(SQLULEN), nullptr); | ||
| // SQL_DESC_PRECISION is stored as a SQLSMALLINT. Read it into a | ||
| // correctly-sized local and widen into the SQLULEN output. Passing the | ||
| // SQLULEN* directly would only write the low 2 bytes and leave the | ||
| // upper 6 bytes uninitialized. | ||
| SQLSMALLINT precision = 0; | ||
| ird->GetField(column_number, SQL_DESC_PRECISION, &precision, sizeof(precision), | ||
| nullptr); | ||
| *column_size_ptr = static_cast<SQLULEN>(precision); | ||
| break; | ||
| } | ||
|
|
||
|
|
@@ -1603,8 +1609,10 @@ SQLRETURN SQLDescribeCol(SQLHSTMT stmt, SQLUSMALLINT column_number, SQLWCHAR* co | |
| case SQL_BIGINT: | ||
| 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. | ||
| ird->GetField(column_number, SQL_DESC_SCALE, decimal_digits_ptr, | ||
| sizeof(SQLULEN), nullptr); | ||
| sizeof(SQLSMALLINT), nullptr); | ||
| break; | ||
| } | ||
|
|
||
|
|
@@ -1621,8 +1629,10 @@ SQLRETURN SQLDescribeCol(SQLHSTMT stmt, SQLUSMALLINT column_number, SQLWCHAR* co | |
| case SQL_INTERVAL_MINUTE_TO_SECOND: | ||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise for this comment. |
||
| ird->GetField(column_number, SQL_DESC_PRECISION, decimal_digits_ptr, | ||
| sizeof(SQLULEN), nullptr); | ||
| sizeof(SQLSMALLINT), nullptr); | ||
| break; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| // Licensed to the Apache Software Foundation (ASF) under one | ||
| // or more contributor license agreements. See the NOTICE file | ||
| // distributed with this work for additional information | ||
| // regarding copyright ownership. The ASF licenses this file | ||
| // to you under the Apache License, Version 2.0 (the | ||
| // "License"); you may not use this file except in compliance | ||
| // with the License. You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, | ||
| // software distributed under the License is distributed on an | ||
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| // KIND, either express or implied. See the License for the | ||
| // specific language governing permissions and limitations | ||
| // under the License. | ||
| #include "arrow/flight/sql/odbc/tests/odbc_test_suite.h" | ||
|
|
||
| #include "arrow/flight/sql/odbc/odbc_impl/platform.h" | ||
|
|
||
| #include <sql.h> | ||
| #include <sqltypes.h> | ||
| #include <sqlucode.h> | ||
|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| namespace arrow::flight::sql::odbc { | ||
|
|
||
| // These tests run against the remote server only. The mock SQLite server | ||
| // reports SQL_WVARCHAR metadata for `SELECT ... AS` result columns (see | ||
| // ColumnsMockTest in columns_test.cc), so it cannot exercise the numeric or | ||
| // datetime branches of SQLDescribeCol that this fix touches. | ||
| class DescribeColRemoteTest : public FlightSQLODBCRemoteTestBase {}; | ||
|
|
||
| namespace { | ||
| // Sentinel used to pre-fill the SQLULEN column_size output. If SQLDescribeCol | ||
| // only writes the low bytes (the width bug this test guards against), the upper | ||
| // bytes remain set to this pattern and the value is far outside any legal | ||
| // column size. | ||
| constexpr SQLULEN kColumnSizeSentinel = static_cast<SQLULEN>(0xFFFFFFFFFFFFFFFFULL); | ||
|
|
||
| // Column ordinals in the all-data-types query (see | ||
| // ODBCTestBase::GetQueryAllDataTypes). Decimal and timestamp exercise the | ||
| // numeric-precision and datetime-precision code paths in SQLDescribeCol. | ||
| constexpr SQLUSMALLINT kDecimalColumn = 17; | ||
| constexpr SQLUSMALLINT kTimestampColumn = 31; | ||
| } // namespace | ||
|
|
||
| // Verify that SQLDescribeCol fully initializes the SQLULEN column_size output | ||
| // 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be better for these tests to be defined in |
||
| std::wstring wsql = this->GetQueryAllDataTypes(); | ||
| std::vector<SQLWCHAR> sql0(wsql.begin(), wsql.end()); | ||
|
|
||
| ASSERT_EQ(SQL_SUCCESS, | ||
| SQLExecDirect(this->stmt, &sql0[0], static_cast<SQLINTEGER>(sql0.size()))); | ||
|
|
||
| SQLWCHAR column_name[kOdbcBufferSize]; | ||
| SQLSMALLINT name_length = 0; | ||
| SQLSMALLINT data_type = 0; | ||
| SQLULEN column_size = kColumnSizeSentinel; | ||
| SQLSMALLINT decimal_digits = -1; | ||
| SQLSMALLINT nullable = 0; | ||
|
|
||
| ASSERT_EQ(SQL_SUCCESS, | ||
| SQLDescribeCol(this->stmt, kDecimalColumn, column_name, | ||
| sizeof(column_name) / sizeof(SQLWCHAR), &name_length, | ||
| &data_type, &column_size, &decimal_digits, &nullable)); | ||
|
|
||
| EXPECT_EQ(SQL_DECIMAL, data_type); | ||
|
|
||
| // The precision (column size) is a small positive integer. If only the low | ||
| // bytes were written, the sentinel's upper bytes would remain set and the | ||
| // full 8-byte value would be enormous. Checking the whole SQLULEN (not just a | ||
| // truncated view of it) is what catches the short write. | ||
| EXPECT_NE(kColumnSizeSentinel, column_size); | ||
| EXPECT_GT(column_size, 0u); | ||
| EXPECT_LT(column_size, 100u) << "column_size upper bytes look uninitialized: 0x" | ||
| << std::hex << column_size; | ||
|
|
||
| // decimal_digits (scale) is a SQLSMALLINT output; it must be a sane scale. | ||
| EXPECT_GE(decimal_digits, 0); | ||
| EXPECT_LE(static_cast<SQLULEN>(decimal_digits), column_size); | ||
| } | ||
|
|
||
| // Verify SQLDescribeCol reports a fully-written column_size for a TIMESTAMP | ||
| // column. This exercises the datetime branch, which reports the display size | ||
| // via SQL_DESC_LENGTH (a SQLULEN) and the seconds precision via | ||
| // SQL_DESC_PRECISION into the SQLSMALLINT decimal_digits output. | ||
| TEST_F(DescribeColRemoteTest, TestSQLDescribeColTimestampColumnSizeIsFullyWritten) { | ||
| std::wstring wsql = this->GetQueryAllDataTypes(); | ||
| std::vector<SQLWCHAR> sql0(wsql.begin(), wsql.end()); | ||
|
|
||
| ASSERT_EQ(SQL_SUCCESS, | ||
| SQLExecDirect(this->stmt, &sql0[0], static_cast<SQLINTEGER>(sql0.size()))); | ||
|
|
||
| SQLWCHAR column_name[kOdbcBufferSize]; | ||
| SQLSMALLINT name_length = 0; | ||
| SQLSMALLINT data_type = 0; | ||
| SQLULEN column_size = kColumnSizeSentinel; | ||
| SQLSMALLINT decimal_digits = -1; | ||
| SQLSMALLINT nullable = 0; | ||
|
|
||
| ASSERT_EQ(SQL_SUCCESS, | ||
| SQLDescribeCol(this->stmt, kTimestampColumn, column_name, | ||
| sizeof(column_name) / sizeof(SQLWCHAR), &name_length, | ||
| &data_type, &column_size, &decimal_digits, &nullable)); | ||
|
|
||
| EXPECT_EQ(SQL_TYPE_TIMESTAMP, data_type); | ||
|
|
||
| // The timestamp display size is a small integer (e.g. 19 or 23). A short | ||
| // write would leave the sentinel's upper bytes intact. | ||
| EXPECT_NE(kColumnSizeSentinel, column_size); | ||
| EXPECT_GT(column_size, 0u); | ||
| EXPECT_LT(column_size, 100u) << "column_size upper bytes look uninitialized: 0x" | ||
| << std::hex << column_size; | ||
|
|
||
| // Seconds precision is a small non-negative scale. | ||
| EXPECT_GE(decimal_digits, 0); | ||
| EXPECT_LE(decimal_digits, 9); | ||
| } | ||
|
|
||
| } // namespace arrow::flight::sql::odbc | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.