Uh oh!
There was an error while loading. Please reload this page.
GH-46575: [C++][FlightRPC] ODBC Diagnostics Report - #47763
Conversation
fe98d28 to
8d1b940Compare8d1b940 to
0c8fddcCompare@lidavidm@kou Please review this draft ODBC API PR. The test PR is at #47764.
|
| // 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); |
There was a problem hiding this comment.
Do we not need to check the return value?
There was a problem hiding this 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.
There was a problem hiding this comment.
Can we be explicit with ARROW_UNUSED? (Also, not necessarily for this PR, but if SQLRETURN is meant to be an error code, perhaps functions returning it should be annotated [[nodiscard]].)
There was a problem hiding this comment.
Sure, added ARROW_UNUSED here.
Regarding [[nodiscard]], we can add it in this PR for ODBC API internal headers (odbc_api_internal.h).
On the other hand, since BI applications (and ODBC tests) will use the ODBC API definition headers at sql.h which doesn't have [[nodiscard]], I think we could skip adding [[nodiscard]] to entry_points.cc as it won't make any impact.
There was a problem hiding this comment.
We can split it out too. [[nodiscard]] isn't a hard requirement, just a potential suggestion.
There was a problem hiding this comment.
yup. It's not a big change so we could do it
| 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; |
There was a problem hiding this comment.
Do we need diag_info_ptr && here?
| diag_info_ptr && buffer_length >= 0 && buffer_length % char_size == 0; | |
| buffer_length >= 0 && buffer_length % char_size == 0; |
There was a problem hiding this comment.
Good catch, removed diag_info_ptr from has_valid_buffer
0c8fddc to
c2a93f4Compare| // 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); |
There was a problem hiding this comment.
Can we be explicit with ARROW_UNUSED? (Also, not necessarily for this PR, but if SQLRETURN is meant to be an error code, perhaps functions returning it should be annotated [[nodiscard]].)
Work on code review comments - remove `diag_info_ptr` from `IsValidStringFieldArgs` checks Co-Authored-By: rscales <robscales@icloud.com>
Addresses community comment: https://github.com/apache/arrow/pull/47763/files#r2450014186
c2a93f4 to
4a0fb9dCompare| // 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); |
There was a problem hiding this comment.
Sure, added ARROW_UNUSED here.
Regarding [[nodiscard]], we can add it in this PR for ODBC API internal headers (odbc_api_internal.h).
On the other hand, since BI applications (and ODBC tests) will use the ODBC API definition headers at sql.h which doesn't have [[nodiscard]], I think we could skip adding [[nodiscard]] to entry_points.cc as it won't make any impact.
| SQLSMALLINT buffer_length, SQLSMALLINT* name_length_ptr, | ||
| SQLSMALLINT* data_type_ptr, SQLULEN* column_size_ptr, | ||
| SQLSMALLINT* decimal_digits_ptr, SQLSMALLINT* nullable_ptr); | ||
| [[nodiscard]] SQLRETURN SQLAllocHandle(SQLSMALLINT type, SQLHANDLE parent, |
There was a problem hiding this comment.
Added [[nodiscard]] here
| // 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); |
There was a problem hiding this comment.
yup. It's not a big change so we could do it
alinaliBQ
commented
Oct 28, 2025
@lidavidm Please let me know if there is any other feedback from your end, thanks! |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit d0e098f. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
### Rationale for this change Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on #47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: #46575 Co-authored-by: rscales <robscales@icloud.com>
### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see apache#47764) ### Are these changes tested? Tests will be in a separate PR (see apache#47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>
Rationale for this change
ODBC needs to provide diagnostic information so users can debug the error
What changes are included in this PR?
Tests are included in separate PR (see GH-46575: [C++][FlightRPC] Add Diagnostic tests #47764)
Are these changes tested?
Tests will be in a separate PR (see #47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors.
Changes tested locally
Are there any user-facing changes?
No