From 5ed464423bd46d008e2520d2bd7212be1756e572 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Tue, 23 Sep 2025 15:21:39 -0700 Subject: [PATCH 1/7] Reword comment on null check Addresses comment https://github.com/apache/arrow/pull/40939#discussion_r2099120394 --- cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h index 8b74028a9c86..b1e5b427b98b 100644 --- a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h +++ b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h @@ -43,7 +43,7 @@ inline size_t CopyFromArrayValuesToBinding(ARRAY_TYPE* array, ColumnBinding* bin } } } else { - // Duplicate this loop to avoid null checks within the loop. + // Duplicate above for loop to exit early when null value is found for (int64_t i = starting_row; i < starting_row + cells; ++i) { if (array->IsNull(i)) { throw odbcabstraction::NullWithoutIndicatorException(); From 0b431930f700d837cdf453776df395eac088cda8 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Tue, 23 Sep 2025 15:28:37 -0700 Subject: [PATCH 2/7] Use `std::memcpy` instead of `memcpy` Addresses comment https://github.com/apache/arrow/pull/40939#discussion_r2099120764 * add include cstring --- .../sql/odbc/flight_sql/accessors/binary_array_accessor.cc | 3 ++- cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h | 3 ++- .../sql/odbc/flight_sql/accessors/string_array_accessor.cc | 3 ++- .../include/odbcabstraction/odbc_impl/attribute_utils.h | 2 +- .../include/odbcabstraction/odbc_impl/encoding_utils.h | 2 +- 5 files changed, 8 insertions(+), 5 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/binary_array_accessor.cc b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/binary_array_accessor.cc index 659b7638cd33..071f9ef799f7 100644 --- a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/binary_array_accessor.cc +++ b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/binary_array_accessor.cc @@ -19,6 +19,7 @@ #include #include +#include #include "arrow/array.h" namespace driver { @@ -44,7 +45,7 @@ inline RowStatus MoveSingleCellToBinaryBuffer(ColumnBinding* binding, BinaryArra auto* byte_buffer = static_cast(binding->buffer) + i * binding->buffer_length; - memcpy(byte_buffer, ((char*)value) + value_offset, value_length); + std::memcpy(byte_buffer, ((char*)value) + value_offset, value_length); if (remaining_length > binding->buffer_length) { result = odbcabstraction::RowStatus_SUCCESS_WITH_INFO; diff --git a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h index b1e5b427b98b..1727589d88cc 100644 --- a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h +++ b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h @@ -19,6 +19,7 @@ #include #include +#include #include "arrow/array.h" #include "arrow/flight/sql/odbc/flight_sql/accessors/types.h" #include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h" @@ -55,7 +56,7 @@ inline size_t CopyFromArrayValuesToBinding(ARRAY_TYPE* array, ColumnBinding* bin // Note that the array should already have been sliced down to the same number // of elements in the ODBC data array by the point in which this function is called. const auto* values = array->raw_values(); - memcpy(binding->buffer, &values[starting_row], element_size * cells); + std::memcpy(binding->buffer, &values[starting_row], element_size * cells); return cells; } diff --git a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.cc b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.cc index fc1e97a4765b..93a3dfd22951 100644 --- a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.cc +++ b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.cc @@ -18,6 +18,7 @@ #include "arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.h" #include +#include #include "arrow/array.h" #include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/encoding.h" @@ -88,7 +89,7 @@ inline RowStatus MoveSingleCellToCharBuffer(std::vector& buffer, auto* byte_buffer = static_cast(binding->buffer) + i * binding->buffer_length; auto* char_buffer = (CHAR_TYPE*)byte_buffer; - memcpy(char_buffer, ((char*)value) + value_offset, value_length); + std::memcpy(char_buffer, ((char*)value) + value_offset, value_length); // Write a NUL terminator if (binding->buffer_length >= remaining_length + sizeof(CHAR_TYPE)) { diff --git a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/attribute_utils.h b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/attribute_utils.h index 24bb3fe2f6db..1bebf96f0ab1 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/attribute_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/attribute_utils.h @@ -50,7 +50,7 @@ inline SQLRETURN GetAttributeUTF8(const std::string_view& attribute_value, if (output) { size_t output_len_before_null = std::min(static_cast(attribute_value.size()), static_cast(output_size - 1)); - memcpy(output, attribute_value.data(), output_len_before_null); + std::memcpy(output, attribute_value.data(), output_len_before_null); reinterpret_cast(output)[output_len_before_null] = '\0'; } diff --git a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h index 4f489a8d1f0a..1939013c6739 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h @@ -45,7 +45,7 @@ inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer, SQLLEN value_length_in_bytes = wstr.size(); if (buffer) { - memcpy(buffer, wstr.data(), + std::memcpy(buffer, wstr.data(), std::min(static_cast(wstr.size()), buffer_size_in_bytes)); // Write a NUL terminator From 8d678a53a94dd69502995549abfffbef149a1d0a Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Thu, 25 Sep 2025 16:37:07 -0700 Subject: [PATCH 3/7] Remove warpdrive mentions --- .../arrow/flight/sql/odbc/flight_sql/get_info_cache.cc | 10 +++++----- .../odbc/odbcabstraction/odbc_impl/odbc_statement.cc | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/flight_sql/get_info_cache.cc b/cpp/src/arrow/flight/sql/odbc/flight_sql/get_info_cache.cc index 1e4cdbeb65de..fddc62dc2f1c 100644 --- a/cpp/src/arrow/flight/sql/odbc/flight_sql/get_info_cache.cc +++ b/cpp/src/arrow/flight/sql/odbc/flight_sql/get_info_cache.cc @@ -954,21 +954,21 @@ bool GetInfoCache::LoadInfoFromServer() { break; } case SqlInfoOptions::SQL_SUPPORTED_RESULT_SET_TYPES: - // Ignored. Warpdrive supports forward-only only. + // Ignored. Arrow ODBC supports forward-only only. break; case SqlInfoOptions::SQL_SUPPORTED_CONCURRENCIES_FOR_RESULT_SET_UNSPECIFIED: - // Ignored. Warpdrive supports forward-only only. + // Ignored. Arrow ODBC supports forward-only only. break; case SqlInfoOptions::SQL_SUPPORTED_CONCURRENCIES_FOR_RESULT_SET_FORWARD_ONLY: - // Ignored. Warpdrive supports forward-only only. + // Ignored. Arrow ODBC supports forward-only only. break; case SqlInfoOptions:: SQL_SUPPORTED_CONCURRENCIES_FOR_RESULT_SET_SCROLL_SENSITIVE: - // Ignored. Warpdrive supports forward-only only. + // Ignored. Arrow ODBC supports forward-only only. break; case SqlInfoOptions:: SQL_SUPPORTED_CONCURRENCIES_FOR_RESULT_SET_SCROLL_INSENSITIVE: - // Ignored. Warpdrive supports forward-only only. + // Ignored. Arrow ODBC supports forward-only only. break; // List properties diff --git a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc index 1b88988b1f3c..5965d4add2f4 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc @@ -251,7 +251,7 @@ void ODBCStatement::CopyAttributesFromConnection(ODBCConnection& connection) { ODBCStatement& tracking_statement = connection.GetTrackingStatement(); // Get abstraction attributes and copy to this spi_statement_. - // Possible ODBC attributes are below, but many of these are not supported by warpdrive + // Possible ODBC attributes are below, but many of these are not supported by Arrow ODBC // or ODBCAbstaction: // SQL_ATTR_ASYNC_ENABLE: // SQL_ATTR_METADATA_ID: From 28615417c229694e869259756282246a8313bd4a Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Thu, 25 Sep 2025 17:11:14 -0700 Subject: [PATCH 4/7] Fix lint --- .../include/odbcabstraction/odbc_impl/encoding_utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h index 1939013c6739..f804514860a0 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h +++ b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h @@ -46,7 +46,7 @@ inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer, if (buffer) { std::memcpy(buffer, wstr.data(), - std::min(static_cast(wstr.size()), buffer_size_in_bytes)); + std::min(static_cast(wstr.size()), buffer_size_in_bytes)); // Write a NUL terminator if (buffer_size_in_bytes >= From f6a5d8270675859df2c86614b6502d61e97a518b Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Fri, 26 Sep 2025 09:54:24 -0700 Subject: [PATCH 5/7] Address Justin's comments --- cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h index 1727589d88cc..47ff47b2c2f5 100644 --- a/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h +++ b/cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h @@ -44,7 +44,7 @@ inline size_t CopyFromArrayValuesToBinding(ARRAY_TYPE* array, ColumnBinding* bin } } } else { - // Duplicate above for loop to exit early when null value is found + // Duplicate above for-loop to exit early when null value is found for (int64_t i = starting_row; i < starting_row + cells; ++i) { if (array->IsNull(i)) { throw odbcabstraction::NullWithoutIndicatorException(); From e2dcb19dae7bd11c33ac71d12ddd26cac0ebdde3 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Fri, 26 Sep 2025 12:09:17 -0700 Subject: [PATCH 6/7] Attempt to resolve random segfault error --- cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt b/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt index c024d7678745..2dc719fa05ec 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt +++ b/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt @@ -32,7 +32,6 @@ set(ARROW_FLIGHT_SQL_MOCK_SERVER_SRCS add_arrow_test(flight_sql_odbc_test SOURCES columns_test.cc - connection_test.cc connection_attr_test.cc connection_info_test.cc errors_test.cc @@ -41,6 +40,8 @@ add_arrow_test(flight_sql_odbc_test statement_test.cc tables_test.cc type_info_test.cc + # Connection test needs to be put last to resolve segfault issue + connection_test.cc odbc_test_suite.cc odbc_test_suite.h # Enable Protobuf cleanup after test execution From a41acdf4e685f5a1c66257283dbc9750955583a3 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Fri, 26 Sep 2025 12:20:45 -0700 Subject: [PATCH 7/7] Revert "Attempt to resolve random segfault error" This reverts commit e2dcb19dae7bd11c33ac71d12ddd26cac0ebdde3. --- cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt b/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt index 2dc719fa05ec..c024d7678745 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt +++ b/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt @@ -32,6 +32,7 @@ set(ARROW_FLIGHT_SQL_MOCK_SERVER_SRCS add_arrow_test(flight_sql_odbc_test SOURCES columns_test.cc + connection_test.cc connection_attr_test.cc connection_info_test.cc errors_test.cc @@ -40,8 +41,6 @@ add_arrow_test(flight_sql_odbc_test statement_test.cc tables_test.cc type_info_test.cc - # Connection test needs to be put last to resolve segfault issue - connection_test.cc odbc_test_suite.cc odbc_test_suite.h # Enable Protobuf cleanup after test execution