Odbc driver feature#596
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an initial YDB ODBC driver implementation (handles, connections, statements, attribute handling, basic metadata queries, and ODBC escape rewriting) along with unit/integration tests and CMake build integration so the driver can be built as part of the SDK.
Changes:
- Introduces
ydb-odbcshared library with core ODBC entrypoints and supporting utilities (conversion, diagnostics, cursors, attribute parsing, escape rewriting). - Adds ODBC unit tests (escape rewriting, SQL LIKE matching, param conversion) and integration tests (basic query flow, env/conn/stmt attrs).
- Integrates ODBC build/test targets into the SDK CMake configuration (option, preset, external ODBC dependency, test helper).
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/library/operation_id/CMakeLists.txt | Adds proto library dependency to operation_id unit test. |
| odbc/tests/unit/sql_like_ut.cpp | Unit tests for SQL LIKE pattern matching helper. |
| odbc/tests/unit/escape_ut.cpp | Unit tests for ODBC escape sequence rewriting. |
| odbc/tests/unit/convert_ut.cpp | Unit tests for ODBC parameter conversion to YDB params. |
| odbc/tests/unit/CMakeLists.txt | Defines ODBC unit test targets and include paths. |
| odbc/tests/integration/test_utils.h | Common ODBC integration test helpers (connect, error formatting). |
| odbc/tests/integration/stmt_attr_it.cpp | Integration tests for statement attributes + metadata-id behavior. |
| odbc/tests/integration/env_it.cpp | Integration tests for environment-level transactions (SQLEndTran). |
| odbc/tests/integration/CMakeLists.txt | Defines ODBC integration test targets. |
| odbc/tests/integration/basic_it.cpp | Integration tests for simple query, parameters, and column binding. |
| odbc/tests/integration/attr_it.cpp | Integration tests for env/connection attribute behavior. |
| odbc/tests/CMakeLists.txt | Adds ODBC tests subdirectories. |
| odbc/src/utils/util.h | Declares helper for converting ODBC strings to std::string. |
| odbc/src/utils/util.cpp | Implements ODBC string conversion helper. |
| odbc/src/utils/types.h | Declares YDB-type to ODBC-metadata helpers. |
| odbc/src/utils/types.cpp | Implements (partially) YDB-type to ODBC-metadata helpers. |
| odbc/src/utils/sql_like.h | Adds inline SQL LIKE matcher used by metadata filtering. |
| odbc/src/utils/escape.h | Declares ODBC escape rewrite function. |
| odbc/src/utils/escape.cpp | Implements ODBC escape rewriting (+ CONVERT-to-CAST rewriting). |
| odbc/src/utils/error_manager.h | Adds diagnostic record storage + exception-to-diagnostic helpers. |
| odbc/src/utils/error_manager.cpp | Implements diagnostic record mapping and retrieval. |
| odbc/src/utils/diag.h | Provides small helpers for standard SQLSTATE diagnostics. |
| odbc/src/utils/cursor.h | Declares cursor abstraction for exec/virtual cursors. |
| odbc/src/utils/cursor.cpp | Implements exec cursor (stream iterator) and virtual cursor (in-memory table). |
| odbc/src/utils/convert.h | Declares parameter and column conversion helpers. |
| odbc/src/utils/convert.cpp | Implements ODBC param conversion registry + YDB->ODBC column conversion. |
| odbc/src/utils/bindings.h | Defines bound parameter/column structs and binding filler interface. |
| odbc/src/utils/attr.h | Declares attribute read/write helpers (string + integer tokens). |
| odbc/src/utils/attr.cpp | Implements attribute string read/write and truncation diagnostics. |
| odbc/src/statement.h | Declares statement handle (prepare/execute/fetch/metadata/attrs). |
| odbc/src/statement.cpp | Implements statement execution, binding, metadata queries, and pattern scanning. |
| odbc/src/statement_attr.h | Declares statement attribute storage + get/set API. |
| odbc/src/statement_attr.cpp | Implements statement attribute get/set logic. |
| odbc/src/odbc_driver.cpp | Implements ODBC C entrypoints wiring to internal handle objects. |
| odbc/src/environment.h | Declares environment handle (attrs, connection registry, EndTran). |
| odbc/src/environment.cpp | Implements environment attrs and env-level EndTran across connections. |
| odbc/src/connection.h | Declares connection handle (connect/attrs/tx/session/client mgmt). |
| odbc/src/connection.cpp | Implements connection string parsing, driver pooling, tx control, catalog routing. |
| odbc/src/connection_attr.h | Declares connection attribute storage + catalog routing helpers. |
| odbc/src/connection_attr.cpp | Implements connection attributes (autocommit, access mode, isolation, catalog). |
| odbc/README.md | Adds build/config/usage documentation for the ODBC driver. |
| odbc/odbcinst.ini | Provides installed driver registration template. |
| odbc/odbc.ini | Provides sample DSN configuration template. |
| odbc/examples/scheme/main.cpp | Example program using SQLTables to enumerate tables. |
| odbc/examples/scheme/CMakeLists.txt | Builds scheme example and injects built driver path macro. |
| odbc/examples/CMakeLists.txt | Adds example subdirectories. |
| odbc/examples/basic/main.cpp | Example program using params and result fetching/binding. |
| odbc/examples/basic/CMakeLists.txt | Builds basic example and injects built driver path macro. |
| odbc/CMakeLists.txt | Builds/install ydb-odbc and adds tests/examples subdirs. |
| CMakePresets.json | Enables YDB_SDK_ODBC in presets. |
| CMakeLists.txt | Adds YDB_SDK_ODBC option and includes odbc/ when enabled. |
| cmake/testing.cmake | Adds add_odbc_test() helper when ODBC is enabled. |
| cmake/external_libs.cmake | Adds find_package(ODBC REQUIRED) under YDB_SDK_ODBC. |
| cmake/common.cmake | Extends _ydb_sdk_add_library options and changes global lib creation helper. |
Comments suppressed due to low confidence (1)
cmake/common.cmake:126
- add_global_library_for() now calls _ydb_sdk_add_library(${TgtName} STATIC ...), but _ydb_sdk_add_library() does not recognize a STATIC option. As a result, the created library type depends on CMake defaults/BUILD_SHARED_LIBS instead of being explicitly STATIC, which can break link behavior (whole-archive) and packaging. Please either teach _ydb_sdk_add_library() to accept STATIC, or change add_global_library_for() to call add_library(${TgtName} STATIC) (or _ydb_sdk_add_library with an explicit mode it supports).
function(add_global_library_for TgtName MainName)
_ydb_sdk_add_library(${TgtName} STATIC ${ARGN})
if(APPLE)
target_link_options(${MainName} INTERFACE "SHELL:-Wl,-force_load,$<TARGET_FILE:$<INSTALL_INTERFACE:YDB-CPP-SDK::>${TgtName}>")
else()
target_link_options(${MainName} INTERFACE "SHELL:-Wl,--whole-archive $<TARGET_FILE:$<INSTALL_INTERFACE:YDB-CPP-SDK::>${TgtName}> -Wl,--no-whole-archive")
endif()
add_dependencies(${MainName} ${TgtName})
target_link_libraries(${MainName} INTERFACE ${TgtName})
endfunction()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 63 changed files in this pull request and generated 16 comments.
Comments suppressed due to low confidence (1)
cmake/common.cmake:123
add_global_library_for()now calls_ydb_sdk_add_library(${TgtName} STATIC ${ARGN}), but_ydb_sdk_add_library()doesn’t handle theSTATICkeyword or forward${ARGN}as sources. This effectively creates an (empty) library target and drops the intended source list, which will break builds if/whenadd_global_library_foris used. Consider either restoringadd_library(${TgtName} STATIC ${ARGN})here, or extending_ydb_sdk_add_libraryto accept a library type + source list (or calltarget_sources()with the unparsed args).
function(add_global_library_for TgtName MainName)
_ydb_sdk_add_library(${TgtName} STATIC ${ARGN})
if(APPLE)
target_link_options(${MainName} INTERFACE "SHELL:-Wl,-force_load,$<TARGET_FILE:$<INSTALL_INTERFACE:YDB-CPP-SDK::>${TgtName}>")
else()
target_link_options(${MainName} INTERFACE "SHELL:-Wl,--whole-archive $<TARGET_FILE:$<INSTALL_INTERFACE:YDB-CPP-SDK::>${TgtName}> -Wl,--no-whole-archive")
endif()
90c479b to
835c5d1
Compare
| std::optional<NQuery::TExecuteQueryPart> prefetchedPart) | ||
| : BindingFiller_(bindingFiller) | ||
| , Iterator_(std::move(iterator)) | ||
| , PrefetchedPart_(std::move(prefetchedPart)) |
There was a problem hiding this comment.
А не проще вызвать 1 раз Fetch, а не передавать PrefetchedPart_
There was a problem hiding this comment.
Нет, fetch() не только читает стрим, но и переходит на первую строку (TryNextRow). Если вызвать его в конце Execute, приложение на первом SQLFetch получит вторую строку.
There was a problem hiding this comment.
Я не буквально имею в виду. Тут вот какие замечания:
- prefetched part частично дублирует логику fetch
- Мы размазываем зону логику вычитывания результата на вызывающую сторону
There was a problem hiding this comment.
Поняла. Убрала PrefetchedPart_: prefetch только в TryCreateExecCursor, первая result set сразу открывается в parser
| add_ydb_test(NAME odbc-escape_ut GTEST | ||
| SOURCES | ||
| escape_ut.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/../../src/utils/escape.cpp |
There was a problem hiding this comment.
А мы не можем просто позависить на ydb-odbc?
|
|
||
| For `SQLConnect("YDB", ...)`, `isql -v YDB`, or `Driver=YDB`. | ||
|
|
||
| **`odbcinst.ini`** — driver registration. Section `[YDB]` is the driver name used as `Driver=YDB` in connection strings and DSNs. `Driver` and `Setup` are the full path to `libydb-odbc.so`. Use `/etc/odbcinst.ini`, a file in `/etc/odbcinst.d/`, or set `ODBCSYSINI` to the directory that contains `odbcinst.ini`. |
There was a problem hiding this comment.
В моем понимании odbcinst.ini - генерится, odbc.ini - пишет пользователь
1b2a273 to
5e72317
Compare
Combine connection attribute routing and error-localization updates into one focused commit while keeping driver pool changes separate. Made-with: Cursor
5e72317 to
accfac3
Compare
f8942e4 to
b5ada5f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8829ad7511
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| NYdb::NRetry::TRetryOperationSettings TStatement::MakeAutocommitRetrySettings() { | ||
| NYdb::NRetry::TRetryOperationSettings settings; | ||
| settings.Idempotent(true); |
There was a problem hiding this comment.
Avoid retrying non-idempotent autocommit writes
When autocommit is on, every statement goes through RetryQuerySync with Idempotent(true), but ODBC autocommit also covers non-idempotent statements such as UPDATE t SET n = n + 1 or inserts with side effects. The SDK retry code only retries UNDETERMINED/transport statuses when this flag is set, so an ambiguous failure after the server applied the write can execute the same SQL again and corrupt user data; only read-only or otherwise proven-idempotent statements should be marked this way.
Useful? React with 👍 / 👎.
| AccessMode_ = *mode; | ||
| auto txMode = Tx::ResolveTxMode(AccessMode_, TxnIsolation_); | ||
| if (!txMode) { |
There was a problem hiding this comment.
Preserve access mode when validation fails
If a connection is in read-only mode with an isolation level that is only valid for read-only transactions, attempting to switch SQL_ATTR_ACCESS_MODE back to read-write correctly returns HYC00, but this assignment has already changed AccessMode_. The connection then reports read-write via SQLGetConnectAttr/SQLGetInfo while TxMode_ still contains the old read-only mode, so subsequent statements run with state that does not match the advertised attribute; validate txMode before mutating AccessMode_.
Useful? React with 👍 / 👎.
| SQLRETURN SQL_API SQLExecDirect(SQLHSTMT statementHandle, | ||
| SQLCHAR* statementText, | ||
| SQLINTEGER textLength) { | ||
| return NYdb::NOdbc::HandleOdbcExceptions<NYdb::NOdbc::TStatement>(statementHandle, [&](auto* stmt) { | ||
| auto ret = stmt->Prepare(NYdb::NOdbc::GetString(statementText, textLength)); |
There was a problem hiding this comment.
Accept full SQLINTEGER statement lengths
SQLExecDirect and SQLPrepare pass their SQLINTEGER textLength into GetString, but this helper takes SQLSMALLINT, so explicit statement lengths above 32767 bytes are truncated or become negative before the SQL is prepared. Applications that pass a non-SQL_NTS length for generated/bulk SQL will execute an empty or partial query even though ODBC defines these statement-text lengths as SQLINTEGER; make the helper accept the wider length used by these APIs.
Useful? React with 👍 / 👎.
No description provided.