Skip to content

Commit ff3ca55

Browse files
HyukjinKwonHyukjinKwon
authored
GH-48707: [C++][FlightRPC] Use IRD precision/scale defaults with ARD override in SQLGetData (#48708)
 ### Rationale for this change https://github.com/apache/arrow/blob/8fc54a35f7df672d416ebd12a9558d320fba9afe/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc#L746-L748 This was introduced by ed36107 The `GetData` function was using hardcoded precision (38) and scale (0) values instead of retrieving them from the IRD (Implementation Row Descriptor). This fix ensures that precision/scale are properly retrieved from the IRD as defaults, and can be overridden by ARD (Application Row Descriptor) values when specified, following ODBC specification behavior. ### What changes are included in this PR? - Modified `ODBCStatement::GetData()` to use IRD precision/scale as defaults instead of hardcoded values - ARD precision/scale now properly override IRD values when `SQL_ARD_TYPE` or `SQL_C_DEFAULT` is used - Added unit tests to verify IRD defaults and ARD override behavior ### Are these changes tested? Yes. Added two tests in `statement_test.cc`: - `TestGetDataPrecisionScaleUsesIRDAsDefault`: Verifies IRD precision/scale are used as defaults - `TestGetDataPrecisionScaleUsesARDWhenSet`: Verifies ARD precision/scale override IRD when set ### Are there any user-facing changes? I think it's fair to say a no. This is an internal fix that ensures correct ODBC behavior. * GitHub Issue: #48707 Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: David Li <li.davidm96@gmail.com>
1 parent 0c49503 commit ff3ca55

2 files changed

Lines changed: 103 additions & 4 deletions

File tree

‎cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "arrow/flight/sql/odbc/odbc_impl/spi/result_set_metadata.h"
2626
#include "arrow/flight/sql/odbc/odbc_impl/spi/statement.h"
2727
#include "arrow/flight/sql/odbc/odbc_impl/types.h"
28+
#include "arrow/type.h"
2829

2930
#include <sql.h>
3031
#include <sqlext.h>
@@ -743,9 +744,12 @@ SQLRETURN ODBCStatement::GetData(SQLSMALLINT record_number, SQLSMALLINT c_type,
743744

744745
SQLSMALLINT evaluated_c_type = c_type;
745746

746-
// TODO: Get proper default precision and scale from abstraction.
747-
int precision = 38; // arrow::Decimal128Type::kMaxPrecision;
748-
int scale = 0;
747+
// Get precision and scale from IRD (implementation row descriptor) as defaults.
748+
// These can be overridden by ARD (application row descriptor) if specified.
749+
const DescriptorRecord& ird_record = ird_->GetRecords()[record_number - 1];
750+
int precision =
751+
ird_record.precision > 0 ? ird_record.precision : Decimal128Type::kMaxPrecision;
752+
int scale = ird_record.scale;
749753

750754
if (c_type == SQL_ARD_TYPE) {
751755
if (record_number > current_ard_->GetRecords().size()) {
@@ -767,7 +771,6 @@ SQLRETURN ODBCStatement::GetData(SQLSMALLINT record_number, SQLSMALLINT c_type,
767771
scale = ard_record.scale;
768772
}
769773

770-
const DescriptorRecord& ird_record = ird_->GetRecords()[record_number - 1];
771774
evaluated_c_type = getc_typeForSQLType(ird_record);
772775
}
773776

‎cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -646,6 +646,102 @@ TEST_F(StatementRemoteTest, TestSQLExecDirectVarbinaryQueryDefaultType) {
646646
EXPECT_EQ('\xEF', varbinary_val[2]);
647647
}
648648

649+
// TODO(GH-48730): Enable this test when ARD/IRD descriptor support is fully implemented
650+
TYPED_TEST(StatementTest, DISABLED_TestGetDataPrecisionScaleUsesIRDAsDefault) {
651+
// Verify that SQLGetData uses IRD precision/scale as defaults when ARD values are unset
652+
std::wstring wsql = L"SELECT CAST('123.45' AS NUMERIC) as decimal_col;";
653+
std::vector<SQLWCHAR> sql(wsql.begin(), wsql.end());
654+
655+
ASSERT_EQ(SQL_SUCCESS,
656+
SQLExecDirect(this->stmt, &sql[0], static_cast<SQLINTEGER>(sql.size())));
657+
658+
ASSERT_EQ(SQL_SUCCESS, SQLFetch(this->stmt));
659+
660+
// Get precision and scale from IRD
661+
SQLLEN ird_precision = 0;
662+
SQLLEN ird_scale = 0;
663+
SQLHDESC ird = nullptr;
664+
ASSERT_EQ(SQL_SUCCESS,
665+
SQLGetStmtAttr(this->stmt, SQL_ATTR_IMP_ROW_DESC, &ird, 0, nullptr));
666+
ASSERT_EQ(SQL_SUCCESS,
667+
SQLGetDescField(ird, 1, SQL_DESC_PRECISION, &ird_precision, 0, nullptr));
668+
ASSERT_EQ(SQL_SUCCESS, SQLGetDescField(ird, 1, SQL_DESC_SCALE, &ird_scale, 0, nullptr));
669+
670+
// Test with SQL_C_NUMERIC - should use IRD precision/scale
671+
SQL_NUMERIC_STRUCT numeric_val;
672+
memset(&numeric_val, 0, sizeof(numeric_val));
673+
SQLLEN indicator;
674+
ASSERT_EQ(SQL_SUCCESS, SQLGetData(this->stmt, 1, SQL_C_NUMERIC, &numeric_val,
675+
sizeof(SQL_NUMERIC_STRUCT), &indicator));
676+
EXPECT_EQ(static_cast<SQLSMALLINT>(ird_precision), numeric_val.precision);
677+
EXPECT_EQ(static_cast<SQLSMALLINT>(ird_scale), numeric_val.scale);
678+
679+
// Test with SQL_C_DEFAULT when ARD is unset (0) - should fall back to IRD
680+
// precision/scale
681+
SQLHDESC ard = nullptr;
682+
ASSERT_EQ(SQL_SUCCESS,
683+
SQLGetStmtAttr(this->stmt, SQL_ATTR_APP_ROW_DESC, &ard, 0, nullptr));
684+
ASSERT_EQ(SQL_SUCCESS, SQLSetDescField(ard, 1, SQL_DESC_PRECISION,
685+
reinterpret_cast<SQLPOINTER>(0), 0));
686+
ASSERT_EQ(SQL_SUCCESS,
687+
SQLSetDescField(ard, 1, SQL_DESC_SCALE, reinterpret_cast<SQLPOINTER>(0), 0));
688+
689+
memset(&numeric_val, 0, sizeof(numeric_val));
690+
ASSERT_EQ(SQL_SUCCESS, SQLGetData(this->stmt, 1, SQL_C_DEFAULT, &numeric_val,
691+
sizeof(SQL_NUMERIC_STRUCT), &indicator));
692+
EXPECT_EQ(static_cast<SQLSMALLINT>(ird_precision), numeric_val.precision);
693+
EXPECT_EQ(static_cast<SQLSMALLINT>(ird_scale), numeric_val.scale);
694+
}
695+
696+
// TODO(GH-48730): Enable this test when ARD/IRD descriptor support is fully implemented
697+
TYPED_TEST(StatementTest, DISABLED_TestGetDataPrecisionScaleUsesARDWhenSet) {
698+
// Verify that SQLGetData uses ARD precision/scale when set, for both SQL_ARD_TYPE and
699+
// SQL_C_DEFAULT
700+
std::wstring wsql = L"SELECT CAST('123.45' AS NUMERIC) as decimal_col;";
701+
std::vector<SQLWCHAR> sql(wsql.begin(), wsql.end());
702+
703+
ASSERT_EQ(SQL_SUCCESS,
704+
SQLExecDirect(this->stmt, &sql[0], static_cast<SQLINTEGER>(sql.size())));
705+
706+
ASSERT_EQ(SQL_SUCCESS, SQLFetch(this->stmt));
707+
708+
SQLHDESC ard = nullptr;
709+
ASSERT_EQ(SQL_SUCCESS,
710+
SQLGetStmtAttr(this->stmt, SQL_ATTR_APP_ROW_DESC, &ard, 0, nullptr));
711+
712+
// Test with SQL_ARD_TYPE
713+
SQLSMALLINT ard_precision = 15;
714+
SQLSMALLINT ard_scale = 3;
715+
ASSERT_EQ(SQL_SUCCESS, SQLSetDescField(ard, 1, SQL_DESC_PRECISION,
716+
reinterpret_cast<SQLPOINTER>(ard_precision), 0));
717+
ASSERT_EQ(SQL_SUCCESS, SQLSetDescField(ard, 1, SQL_DESC_SCALE,
718+
reinterpret_cast<SQLPOINTER>(ard_scale), 0));
719+
ASSERT_EQ(SQL_SUCCESS, SQLSetDescField(ard, 1, SQL_DESC_TYPE,
720+
reinterpret_cast<SQLPOINTER>(SQL_C_NUMERIC), 0));
721+
722+
SQL_NUMERIC_STRUCT numeric_val;
723+
memset(&numeric_val, 0, sizeof(numeric_val));
724+
SQLLEN indicator;
725+
ASSERT_EQ(SQL_SUCCESS, SQLGetData(this->stmt, 1, SQL_ARD_TYPE, &numeric_val,
726+
sizeof(SQL_NUMERIC_STRUCT), &indicator));
727+
EXPECT_EQ(ard_precision, numeric_val.precision);
728+
EXPECT_EQ(ard_scale, numeric_val.scale);
729+
730+
// Test with SQL_C_DEFAULT
731+
ard_precision = 20;
732+
ard_scale = 4;
733+
ASSERT_EQ(SQL_SUCCESS, SQLSetDescField(ard, 1, SQL_DESC_PRECISION,
734+
reinterpret_cast<SQLPOINTER>(ard_precision), 0));
735+
ASSERT_EQ(SQL_SUCCESS, SQLSetDescField(ard, 1, SQL_DESC_SCALE,
736+
reinterpret_cast<SQLPOINTER>(ard_scale), 0));
737+
738+
memset(&numeric_val, 0, sizeof(numeric_val));
739+
ASSERT_EQ(SQL_SUCCESS, SQLGetData(this->stmt, 1, SQL_C_DEFAULT, &numeric_val,
740+
sizeof(SQL_NUMERIC_STRUCT), &indicator));
741+
EXPECT_EQ(ard_precision, numeric_val.precision);
742+
EXPECT_EQ(ard_scale, numeric_val.scale);
743+
}
744+
649745
TYPED_TEST(StatementTest, TestSQLExecDirectGuidQueryUnsupported) {
650746
// Query GUID as string as SQLite does not support GUID
651747
std::wstring wsql = L"SELECT 'C77313CF-4E08-47CE-B6DF-94DD2FCF3541' AS guid;";

0 commit comments

Comments
 (0)