Skip to content

Commit c2a93f4

Browse files
committed
Work on code review comments
- remove `diag_info_ptr` from `IsValidStringFieldArgs` checks
1 parent cb14b52 commit c2a93f4

File tree

1 file changed

+11
-4
lines changed

1 file changed

+11
-4
lines changed

cpp/src/arrow/flight/sql/odbc/odbc_api.cc

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ inline bool IsValidStringFieldArgs(SQLPOINTER diag_info_ptr, SQLSMALLINT buffer_
6969
SQLSMALLINT* string_length_ptr, bool is_unicode) {
7070
const SQLSMALLINT char_size = is_unicode ? GetSqlWCharSize() : sizeof(char);
7171
const bool has_valid_buffer =
72-
diag_info_ptr && buffer_length >= 0 && buffer_length % char_size == 0;
72+
buffer_length == SQL_NTS || (buffer_length >= 0 && buffer_length % char_size == 0);
7373

7474
// regardless of capacity return false if invalid
7575
if (diag_info_ptr && !has_valid_buffer) {
@@ -91,7 +91,6 @@ SQLRETURN SQLGetDiagField(SQLSMALLINT handle_type, SQLHANDLE handle,
9191
<< ", buffer_length: " << buffer_length << ", string_length_ptr: "
9292
<< static_cast<const void*>(string_length_ptr);
9393
// GH-46575 TODO: Add tests for SQLGetDiagField
94-
using arrow::flight::sql::odbc::Diagnostics;
9594
using ODBC::GetStringAttribute;
9695
using ODBC::ODBCConnection;
9796
using ODBC::ODBCDescriptor;
@@ -109,7 +108,7 @@ SQLRETURN SQLGetDiagField(SQLSMALLINT handle_type, SQLHANDLE handle,
109108
// If buffer length derived from null terminated string
110109
if (diag_info_ptr && buffer_length == SQL_NTS) {
111110
const wchar_t* str = reinterpret_cast<wchar_t*>(diag_info_ptr);
112-
buffer_length = wcslen(str) * arrow::flight::sql::odbc::GetSqlWCharSize();
111+
buffer_length = wcslen(str) * GetSqlWCharSize();
113112
}
114113

115114
// Set character type to be Unicode by default
@@ -164,7 +163,7 @@ SQLRETURN SQLGetDiagField(SQLSMALLINT handle_type, SQLHANDLE handle,
164163
return SQL_SUCCESS;
165164
}
166165

167-
// TODO implement return code function
166+
// Driver manager implements SQL_DIAG_RETURNCODE
168167
case SQL_DIAG_RETURNCODE: {
169168
return SQL_SUCCESS;
170169
}
@@ -420,6 +419,10 @@ SQLRETURN SQLGetDiagRec(SQLSMALLINT handle_type, SQLHANDLE handle, SQLSMALLINT r
420419
// The length of the sql state is always 5 characters plus null
421420
SQLSMALLINT size = 6;
422421
const std::string& state = diagnostics->GetSQLState(record_index);
422+
423+
// Microsoft documentation does not mention
424+
// any SQLGetDiagRec return value that is associated with `sql_state` buffer, so
425+
// the return value for writing `sql_state` buffer is ignored by the driver.
423426
GetStringAttribute(is_unicode, state, false, sql_state, size, &size, *diagnostics);
424427
}
425428

@@ -429,6 +432,10 @@ SQLRETURN SQLGetDiagRec(SQLSMALLINT handle_type, SQLHANDLE handle, SQLSMALLINT r
429432

430433
if (message_text || text_length_ptr) {
431434
const std::string& message = diagnostics->GetMessageText(record_index);
435+
436+
// According to Microsoft documentation,
437+
// SQL_SUCCESS_WITH_INFO should be returned if `*message_text` buffer was too
438+
// small to hold the requested diagnostic message.
432439
return GetStringAttribute(is_unicode, message, false, message_text, buffer_length,
433440
text_length_ptr, *diagnostics);
434441
}

0 commit comments

Comments
 (0)