Skip to content

Commit 9ac6e57

Browse files
authored
Address code review comments for Arrow#40939
* Reword comment on null check Addresses comment apache#40939 (comment) * Use `std::memcpy` instead of `memcpy` Addresses comment apache#40939 (comment) * add include cstring * Remove warpdrive mentions * Fix lint * Address Justin's comments
1 parent 382e799 commit 9ac6e57

7 files changed

Lines changed: 16 additions & 13 deletions

File tree

cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/binary_array_accessor.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include <algorithm>
2121
#include <cstdint>
22+
#include <cstring>
2223
#include "arrow/array.h"
2324

2425
namespace driver {
@@ -44,7 +45,7 @@ inline RowStatus MoveSingleCellToBinaryBuffer(ColumnBinding* binding, BinaryArra
4445

4546
auto* byte_buffer =
4647
static_cast<unsigned char*>(binding->buffer) + i * binding->buffer_length;
47-
memcpy(byte_buffer, ((char*)value) + value_offset, value_length);
48+
std::memcpy(byte_buffer, ((char*)value) + value_offset, value_length);
4849

4950
if (remaining_length > binding->buffer_length) {
5051
result = odbcabstraction::RowStatus_SUCCESS_WITH_INFO;

cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/common.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include <algorithm>
2121
#include <cstdint>
22+
#include <cstring>
2223
#include "arrow/array.h"
2324
#include "arrow/flight/sql/odbc/flight_sql/accessors/types.h"
2425
#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/diagnostics.h"
@@ -43,7 +44,7 @@ inline size_t CopyFromArrayValuesToBinding(ARRAY_TYPE* array, ColumnBinding* bin
4344
}
4445
}
4546
} else {
46-
// Duplicate this loop to avoid null checks within the loop.
47+
// Duplicate above for-loop to exit early when null value is found
4748
for (int64_t i = starting_row; i < starting_row + cells; ++i) {
4849
if (array->IsNull(i)) {
4950
throw odbcabstraction::NullWithoutIndicatorException();
@@ -55,7 +56,7 @@ inline size_t CopyFromArrayValuesToBinding(ARRAY_TYPE* array, ColumnBinding* bin
5556
// Note that the array should already have been sliced down to the same number
5657
// of elements in the ODBC data array by the point in which this function is called.
5758
const auto* values = array->raw_values();
58-
memcpy(binding->buffer, &values[starting_row], element_size * cells);
59+
std::memcpy(binding->buffer, &values[starting_row], element_size * cells);
5960

6061
return cells;
6162
}

cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "arrow/flight/sql/odbc/flight_sql/accessors/string_array_accessor.h"
1919

2020
#include <boost/locale.hpp>
21+
#include <cstring>
2122
#include "arrow/array.h"
2223
#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/encoding.h"
2324

@@ -88,7 +89,7 @@ inline RowStatus MoveSingleCellToCharBuffer(std::vector<uint8_t>& buffer,
8889

8990
auto* byte_buffer = static_cast<char*>(binding->buffer) + i * binding->buffer_length;
9091
auto* char_buffer = (CHAR_TYPE*)byte_buffer;
91-
memcpy(char_buffer, ((char*)value) + value_offset, value_length);
92+
std::memcpy(char_buffer, ((char*)value) + value_offset, value_length);
9293

9394
// Write a NUL terminator
9495
if (binding->buffer_length >= remaining_length + sizeof(CHAR_TYPE)) {

cpp/src/arrow/flight/sql/odbc/flight_sql/get_info_cache.cc

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -954,21 +954,21 @@ bool GetInfoCache::LoadInfoFromServer() {
954954
break;
955955
}
956956
case SqlInfoOptions::SQL_SUPPORTED_RESULT_SET_TYPES:
957-
// Ignored. Warpdrive supports forward-only only.
957+
// Ignored. Arrow ODBC supports forward-only only.
958958
break;
959959
case SqlInfoOptions::SQL_SUPPORTED_CONCURRENCIES_FOR_RESULT_SET_UNSPECIFIED:
960-
// Ignored. Warpdrive supports forward-only only.
960+
// Ignored. Arrow ODBC supports forward-only only.
961961
break;
962962
case SqlInfoOptions::SQL_SUPPORTED_CONCURRENCIES_FOR_RESULT_SET_FORWARD_ONLY:
963-
// Ignored. Warpdrive supports forward-only only.
963+
// Ignored. Arrow ODBC supports forward-only only.
964964
break;
965965
case SqlInfoOptions::
966966
SQL_SUPPORTED_CONCURRENCIES_FOR_RESULT_SET_SCROLL_SENSITIVE:
967-
// Ignored. Warpdrive supports forward-only only.
967+
// Ignored. Arrow ODBC supports forward-only only.
968968
break;
969969
case SqlInfoOptions::
970970
SQL_SUPPORTED_CONCURRENCIES_FOR_RESULT_SET_SCROLL_INSENSITIVE:
971-
// Ignored. Warpdrive supports forward-only only.
971+
// Ignored. Arrow ODBC supports forward-only only.
972972
break;
973973

974974
// List<string> properties

cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/attribute_utils.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ inline SQLRETURN GetAttributeUTF8(const std::string_view& attribute_value,
5050
if (output) {
5151
size_t output_len_before_null =
5252
std::min(static_cast<O>(attribute_value.size()), static_cast<O>(output_size - 1));
53-
memcpy(output, attribute_value.data(), output_len_before_null);
53+
std::memcpy(output, attribute_value.data(), output_len_before_null);
5454
reinterpret_cast<char*>(output)[output_len_before_null] = '\0';
5555
}
5656

cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/encoding_utils.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,8 @@ inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer,
4545
SQLLEN value_length_in_bytes = wstr.size();
4646

4747
if (buffer) {
48-
memcpy(buffer, wstr.data(),
49-
std::min(static_cast<SQLLEN>(wstr.size()), buffer_size_in_bytes));
48+
std::memcpy(buffer, wstr.data(),
49+
std::min(static_cast<SQLLEN>(wstr.size()), buffer_size_in_bytes));
5050

5151
// Write a NUL terminator
5252
if (buffer_size_in_bytes >=

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ void ODBCStatement::CopyAttributesFromConnection(ODBCConnection& connection) {
251251
ODBCStatement& tracking_statement = connection.GetTrackingStatement();
252252

253253
// Get abstraction attributes and copy to this spi_statement_.
254-
// Possible ODBC attributes are below, but many of these are not supported by warpdrive
254+
// Possible ODBC attributes are below, but many of these are not supported by Arrow ODBC
255255
// or ODBCAbstaction:
256256
// SQL_ATTR_ASYNC_ENABLE:
257257
// SQL_ATTR_METADATA_ID:

0 commit comments

Comments
 (0)