Uh oh!
There was an error while loading. Please reload this page.
GH-50852: [C++][FlightRPC][ODBC] Decode wide connection/descriptor string attributes with the wide decoder - #50853
Conversation
e57ce97 to
e51c04fCompareThere was a problem hiding this comment.
Pull request overview
Corrects wide-string decoding for ODBC catalog and descriptor attributes.
Changes:
- Uses the wide decoder for Unicode catalog and descriptor names.
- Preserves UTF-8 decoding for non-Unicode catalog values.
- Adds catalog round-trip coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
connection_attr_test.cc | Tests wide catalog round-tripping. |
odbc_descriptor.cc | Corrects descriptor-name decoding. |
odbc_connection.cc | Corrects catalog decoder selection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| break; | ||
| case SQL_DESC_NAME: | ||
| SetAttributeUTF8(value, buffer_length, record.name); | ||
| SetAttributeSQLWCHAR(value, buffer_length, record.name); |
There was a problem hiding this comment.
Good catch — added ODBCDescriptorTest.SetGetNameWideRoundTrips in odbc_descriptor_test.cc, a direct SetField/GetField round-trip on SQL_DESC_NAME with a multi-character wide name. It passes with the fix and fails without it (the byte-wise decode corrupts the stored UTF-16, which the wide getter then rejects). A black-box test isn't possible here since there's no SQLSetDescField entry point wired up, so the unit test on ODBCDescriptor covers it directly.
…tor string attributes with the wide decoder SetConnectAttr(SQL_ATTR_CURRENT_CATALOG) had its is_unicode decode branches swapped, and SetField(SQL_DESC_NAME) unconditionally used the byte-wise SetAttributeUTF8. In both cases a wide SQLWCHAR buffer was misread, corrupting the stored value. Decode wide buffers with SetAttributeSQLWCHAR, matching the mapping the getters already use (GetStringAttribute / GetAttributeSQLWCHAR). Add a round-trip TYPED_TEST for SQL_ATTR_CURRENT_CATALOG through the wide entry point (connection_attr_test.cc) and a SetField/GetField round-trip unit test for SQL_DESC_NAME (odbc_descriptor_test.cc). Co-authored-by: Isaac
e51c04f to
98f81ccCompareThere was a problem hiding this comment.
Can we consolidate into an existing file?
There was a problem hiding this comment.
The tricky part is SQL_DESC_NAME can't be exercised through a public entry point. I had tried a black-box test via SQLGetStmtAttr(SQL_ATTR_APP_ROW_DESC) + SQLSetDescField(ard, 1, SQL_DESC_NAME, …), but that path returns SQL_ERROR (the field isn't settable on the ARD through the DM), so the only way to reach ODBCDescriptor::SetField(SQL_DESC_NAME) is a direct unit test on ODBCDescriptor.
There's no existing descriptor unit test in odbc_impl/ to merge into, and the unit tests there follow a 1:1 source↔test naming convention (util.cc↔util_test.cc, json_converter.cc↔json_converter_test.cc, etc.), so odbc_descriptor_test.cc fit that pattern. Happy to fold it elsewhere if you'd prefer though, e.g. into util_test.cc.
Rationale for this change
Two string attributes in the ODBC driver are decoded with the byte-wise
decoder even when the value arrives through a wide (Unicode /
*W) entry point,so the wide buffer is misread and corrupted.
ODBCConnection::SetConnectAttr(SQL_ATTR_CURRENT_CATALOG)had its two decodebranches swapped:
is_unicodeselects the buffer width: a unicode (*W) call passes a wideSQLWCHARbuffer that must be decoded withSetAttributeSQLWCHAR; a non-unicodecall passes a byte string decoded with
SetAttributeUTF8. With the branchesswapped, a wide catalog name (e.g. UTF-16
"my_catalog") is reinterpretedbyte-wise and stored with the wide encoding's embedded NULs
(
"m\0y\0_\0c\0a\0t\0a\0l\0o\0g\0"), so catalog scoping operates on a corruptname. This is the inverse of the mapping the getter already uses:
GetStringAttributemapsis_unicode -> GetAttributeSQLWCHAR.ODBCDescriptor::SetField(SQL_DESC_NAME)is the same class of bug: itunconditionally used the byte-wise
SetAttributeUTF8, even though the matchinggetter (
GetField/SQL_DESC_NAME) reads the field back withGetAttributeSQLWCHAR.How this happened (history):
SQL_ATTR_CURRENT_CATALOGbranches were swapped from the start, in theoriginal driver import ([C++][FlightRPC] Accept donation of ODBC driver #46522, GH-46522: [C++][FlightRPC] Add Arrow Flight SQL ODBC driver #40939). Note the getter side was written
against the shared
GetStringAttributehelper (which decides width in oneplace and got it right), while the setter open-coded the branch inline and
inverted the polarity — there is no matching
SetStringAttributehelper.SQL_DESC_NAMEcase was originally consistent (getter andsetter both byte-wise). [C++][FlightRPC][ODBC] SQLColAttribute implementation #47721 (GH-47721: [C++][FlightRPC] Return ODBC Column Attribute from result set #48050) later migrated the descriptor
string getters to the wide
GetAttributeSQLWCHARfor correct Unicode columnattributes, but left the
SQL_DESC_NAMEsetter on the byte-wise decoder,creating the asymmetry.
What changes are included in this PR?
odbc_connection.cc: swap theSQL_ATTR_CURRENT_CATALOGdecode branches so aunicode call uses
SetAttributeSQLWCHARand a non-unicode call usesSetAttributeUTF8, matchingGetStringAttribute.odbc_descriptor.cc: decodeSQL_DESC_NAMEwithSetAttributeSQLWCHARtomatch its getter.
connection_attr_test.cc: addTestSQLSetGetConnectAttrCurrentCatalogWide, aTYPED_TEST(mock + remote fixtures) that sets a multi-character catalogthrough the wide entry point and asserts the full name round-trips back.
odbc_descriptor_test.cc(new): addODBCDescriptorTest.SetGetNameWideRoundTrips,a
SetField/GetFieldround-trip onSQL_DESC_NAMEwith a multi-characterwide name. There is no
SQLSetDescFieldentry point in this tree, so thiscovers the descriptor change with a direct unit test on
ODBCDescriptor.Are these changes tested?
Yes. Each change has a round-trip test that distinguishes the fixed and unfixed
code:
out_catalog == "my_catalog"out_catalog == "m\0y\0_\0c\0a\0t\0a\0l\0o\0g\0""my_column"arrow-odbc-spi-impl-test(unit suite)ConnectionAttributeTest/0.*(mock suite)The connection catalog test uses the mock fixture (
FlightSQLODBCMockTestBase,an in-process SQLite Flight SQL server); its remote variant runs when
ARROW_FLIGHT_SQL_ODBC_CONNis set. The descriptor test is a standalone unittest. Verified locally against the mock fixture and the unit suite.
Are there any user-facing changes?
Yes. A catalog name (and descriptor
SQL_DESC_NAME) set through the wide entrypoint is now decoded correctly instead of being corrupted. Applications that set
a multi-character catalog through
SQLSetConnectAttrWnow scope to the intendedcatalog.
AI usage
Per the AI-generated code guidance:
the diagnosis, the fix, and the test were produced with the assistance of an AI
coding agent and reviewed and verified by me. Correctness was checked by
(1) confirming the getter path (
GetStringAttribute) mapsis_unicodeto thewide decoder, establishing the intended mapping the setters violated, and
(2) running the new round-trip test against both the fixed and unfixed driver to
confirm it distinguishes them (clean name vs. embedded-NUL corruption).
🤖 Drafted by Claude Code (an AI agent) and reviewed & verified by vikrantpuppala.