alinaliBQ commented on code in PR #47763:
URL: https://github.com/apache/arrow/pull/47763#discussion_r2449679408
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -65,6 +65,20 @@ SQLRETURN SQLFreeStmt(SQLHSTMT handle, SQLUSMALLINT option) {
return SQL_INVALID_HANDLE;
Review Comment:
TEMP
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -65,6 +65,20 @@ SQLRETURN SQLFreeStmt(SQLHSTMT handle, SQLUSMALLINT option) {
return SQL_INVALID_HANDLE;
}
+inline bool IsValidStringFieldArgs(SQLPOINTER diag_info_ptr, SQLSMALLINT
buffer_length,
+ SQLSMALLINT* string_length_ptr, bool
is_unicode) {
+ const SQLSMALLINT char_size = is_unicode ? GetSqlWCharSize() : sizeof(char);
+ const bool has_valid_buffer =
+ diag_info_ptr && buffer_length >= 0 && buffer_length % char_size == 0;
Review Comment:
Good catch, removed `diag_info_ptr` from `has_valid_buffer`
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -91,8 +356,84 @@ SQLRETURN SQLGetDiagRec(SQLSMALLINT handle_type, SQLHANDLE
handle, SQLSMALLINT r
<< ", message_text: " << static_cast<const
void*>(message_text)
<< ", buffer_length: " << buffer_length
<< ", text_length_ptr: " << static_cast<const
void*>(text_length_ptr);
- // GH-46575 TODO: Implement SQLGetDiagRec
- return SQL_INVALID_HANDLE;
+ // GH-46575 TODO: Add tests for SQLGetDiagRec
+ using arrow::flight::sql::odbc::Diagnostics;
+ using ODBC::GetStringAttribute;
+ using ODBC::ODBCConnection;
+ using ODBC::ODBCDescriptor;
+ using ODBC::ODBCEnvironment;
+ using ODBC::ODBCStatement;
+
+ if (!handle) {
+ return SQL_INVALID_HANDLE;
+ }
+
+ // Record number must be greater or equal to 1
+ if (rec_number < 1 || buffer_length < 0) {
+ return SQL_ERROR;
+ }
+
+ // Set character type to be Unicode by default
+ const bool is_unicode = true;
+ Diagnostics* diagnostics = nullptr;
+
+ switch (handle_type) {
+ case SQL_HANDLE_ENV: {
+ auto* environment = ODBCEnvironment::Of(handle);
+ diagnostics = &environment->GetDiagnostics();
+ break;
+ }
+
+ case SQL_HANDLE_DBC: {
+ auto* connection = ODBCConnection::Of(handle);
+ diagnostics = &connection->GetDiagnostics();
+ break;
+ }
+
+ case SQL_HANDLE_DESC: {
+ auto* descriptor = ODBCDescriptor::Of(handle);
+ diagnostics = &descriptor->GetDiagnostics();
+ break;
+ }
+
+ case SQL_HANDLE_STMT: {
+ auto* statement = ODBCStatement::Of(handle);
+ diagnostics = &statement->GetDiagnostics();
+ break;
+ }
+
+ default:
+ return SQL_INVALID_HANDLE;
+ }
+
+ if (!diagnostics) {
+ return SQL_ERROR;
+ }
+
+ // Convert from ODBC 1 based record number to internal diagnostics 0 indexed
storage
+ const size_t record_index = static_cast<size_t>(rec_number - 1);
+ if (!diagnostics->HasRecord(record_index)) {
+ return SQL_NO_DATA;
+ }
+
+ if (sql_state) {
+ // The length of the sql state is always 5 characters plus null
+ SQLSMALLINT size = 6;
+ const std::string& state = diagnostics->GetSQLState(record_index);
+ GetStringAttribute(is_unicode, state, false, sql_state, size, &size,
*diagnostics);
Review Comment:
According to Microsoft documentation
https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgetdiagrec-function?view=sql-server-ver17#diagnostics,
`SQL_SUCCESS_WITH_INFO` should be returned if `*message_text` buffer was too
small to hold the requested diagnostic message. But the doc does not mention
any `SQLGetDiagRec` return value that is associated with `sql_state` buffer, so
the return value for writing `sql_state` buffer is ignored by the driver. I
have added an in-line comment for this.
`sql_state` is meant to always contain a 5-character string, so error risk
for writing it is low.
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -65,6 +65,20 @@ SQLRETURN SQLFreeStmt(SQLHSTMT handle, SQLUSMALLINT option) {
return SQL_INVALID_HANDLE;
Review Comment:
TEMP
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
