chore: extract the Trino ODBC driver from the stackable-odbc-rs workspace - #1
Merged
Conversation
Extracted from stackable-odbc-rs, which held core, trino and sqlite in one workspace. The manifest is de-inherited from that workspace and gains the metadata crates.io requires. Version resets to 0.0.1. Does not compile yet: stackable-odbc-core is ahead of the copy this driver last built against. Adapted in the next commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
default_get_info and common_get_info_raw are now generic over the backend. SQL_CURSOR_COMMIT_BEHAVIOR is now derived from Backend::cursor_commit_behavior, which defaults to SQL_CB_PRESERVE (2); this driver reports SQL_TC_NONE, so the default is correct and the snapshot expectation was the stale part. Also corrects two comments that justified SQL_TC_NONE with "Trino has no explicit transactions". Trino does support transactions over the X-Trino-Transaction-Id headers; it is this driver that does not implement them. The reported capability is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pre-commit configuration is stackable-odbc-core's extended set -- check-yaml, check-toml, check-merge-conflict, mixed-line-ending and cargo-sort -- with shellcheck re-added for the nine shell scripts this repository ships, and cargo test in place of cargo test --workspace. deny.toml keeps stackable-odbc-rs's Windows target triple, since this repository ships a DLL, and its RUSTSEC-2024-0436 ignore for the unmaintained paste crate that trino-rust-client pulls in. Its ring and OpenSSL licence allowances are dropped: rustls 0.23 resolves to aws-lc-rs, so ring is absent from the tree entirely and no OpenSSL-licensed crate is present. Verified by removing them and re-running cargo deny check against all three targets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paths collapse now that the crate is the repository root: CRATE_DIR and REPO_ROOT become one variable, and the dist directory moves from crates/stackable-odbc-trino/packaging/dist to packaging/dist. The -p package selector goes with the workspace it selected from. The support link pointed at stackable-odbc-rs and now points here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build.sh resolves paths from its own location, so it needed no edits; the README's four nested crates/stackable-odbc-trino/connector/ paths did. The .pq file's [Version = "1.0.0"] is the Power BI connector's own version, which Power BI reads, not a Cargo version -- deliberately not reset to 0.0.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test/trino/ flattens to test/: there is no sibling suite here to
disambiguate from. Three consequences of the shallower path:
- the test/trino/ usage strings and comments become test/
- PROJECT_DIR was $SCRIPT_DIR/../.., which now overshoots the repository
root. setup.sh and run-tests.sh would have built in the wrong
directory, and windows_test.py would have looked for
windows/openssl_legacy.cnf and the built DLL outside the repository.
- cargo's -p selector goes with the workspace it selected from
Generated artefacts are left behind rather than copied: setup.sh
regenerates them, and the old odbc.ini held an absolute path into
stackable-odbc-rs. .gitignore also learns about Python bytecode, which
the five test scripts produce and nothing previously ignored.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WINDOWS.md documented both drivers; the SQLite half goes, along with its
build lines, driver and DSN registration, registry cleanup and the
SQLite-specific parameter table.
Two things are corrected rather than merely copied:
- The PowerShell smoke test created and dropped a table, which assumes
a writable catalog. Trino's DDL support depends on the connector
backing the catalog, so it now selects from an inline VALUES list --
self-contained, as the original intended to be.
- The connection-string table omitted AccessToken/Token and
QueryTimeout/LoginTimeout. Both are accepted by connect_params.rs.
The virtio-win ISO is copied across but stays untracked under the
target/ rule; start.yaml re-fetches it from a pinned, checksummed URL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build.yaml is adapted from stackable-odbc-core's, minus the Miri and fuzz jobs: Miri cannot execute the OpenSSL and aws-lc-rs code trino-rust-client links in, and both fuzz targets exercise core APIs. It gains the Windows cross-compile job from stackable-odbc-rs, narrowed to this one DLL. release.yaml triggers on v* rather than trino-v*, which is the tag cargo-release produces, reads the root Cargo.toml, and drops the workspace-era -p selector and nested packaging paths. Its commented-out integration-test block is gone; that TODO now lives in build.yaml. Note that CI cannot pass until stackable-odbc-core is published: the path dependency does not resolve on a runner. This is deliberate -- the workflows are written for the published world, so swapping the path dep for a version dep is the only change needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AGENTS.md is driver-focused and self-contained: an agent working here cannot read stackable-odbc-core's copy. The framework-internal sections core owns -- adding an ODBC function, forward_ffi internals, Miri, fuzzing -- are cross-linked rather than duplicated, to avoid drift. Content is verified against source rather than carried over on trust: the isql DSN names against test/setup.sh, and every connection-string key against connect_params.rs. That last check is what surfaced the AccessToken/Token and QueryTimeout/LoginTimeout keys missing from the Windows documentation, now recorded in the changelog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mirrors stackable-odbc-core's, except for what gets version-bumped: this
crate is a loadable driver rather than a Rust dependency, so there is no
dep snippet to rewrite and the version strings kept current are the
packaging examples instead.
The v{version} tag it produces is what release.yaml triggers on.
Validated with a dry run: all four replacements fire, the changelog
footer's first-release rule emits the tag link exactly once (the
compare-form rule correctly no-ops via min = 0), the pre-release
pre-commit hook passes, and the allow-branch guard refuses to release
from a branch other than main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The repository-wide sweep caught what the per-directory sweeps missed: the Rust sources still carried 56 test/trino/ paths in #[ignore] reasons and doc comments, seven cargo -p / --workspace invocations that no longer have a workspace to select from, and one crates/stackable-odbc-trino/ path in escape_dialect.rs. test/windows_test.py's HTTP_PORT comment said it avoided "SQLite's 8080". The port is unchanged and still correct -- 8080 is Trino's port here -- but the reason it gives now matches the repository it lives in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stackable-odbc-core made fourteen capability facts required `Backend` methods, on the rule that a value whose zero is an answer rather than "unknown" must come from the backend. Implementing them corrected five values this driver had been reporting wrongly, each confirmed against a live Trino 467 coordinator rather than the documentation: - SQL_NULL_COLLATION was SQL_NC_HIGH, meaning NULL position follows ASC/DESC. Trino orders NULLS LAST in both directions, which is SQL_NC_END. - SQL_GROUP_BY was SQL_GB_NO_RELATION, which tells an application it may put arbitrary non-grouped columns in the select list. Trino rejects that with EXPRESSION_NOT_AGGREGATE while allowing GROUP BY columns absent from the select list -- SQL_GB_GROUP_BY_CONTAINS_SELECT. - SQL_DEFAULT_TXN_ISOLATION was SQL_TXN_READ_COMMITTED while SQL_TXN_CAPABLE reported SQL_TC_NONE and SQL_TXN_ISOLATION_OPTION reported 0, so it named a level absent from its own supported set. The spec's value for a data source without transactions is 0. - SQL_ALTER_TABLE and SQL_SQL_CONFORMANCE were core's defaults: 0, which claims Trino cannot ALTER TABLE at all, and SQL_SC_SQL92_ENTRY, which Trino does not reach -- its CREATE TABLE rejects PRIMARY KEY, UNIQUE, CHECK and REFERENCES, and entry level requires both referential integrity and COMMIT/ROLLBACK. SQL_CORRELATION_NAME, SQL_NON_NULLABLE_COLUMNS and SQL_EXPRESSIONS_IN_ORDERBY had been reaching applications as SQL_CN_NONE, SQL_NNC_NULL and the empty string, none of which Trino warrants and the last of which is not one of that info type's two values. The SQL_CATALOG_NAME, SQL_NULL_COLLATION, SQL_OJ_CAPABILITIES, SQL_DEFAULT_TXN_ISOLATION and SQL_TXN_ISOLATION_OPTION arms are removed from trino_get_info. Core derives all five from a hook, and an arm here would answer SQLGetInfo while the hook still drove SQLGetConnectAttr and the HY024 validation in sql_set_connect_attr -- the two could then report different things for one connection. timedate_add_intervals and timedate_diff_intervals stay 0. The spec defines them as the intervals TIMESTAMPADD/TIMESTAMPDIFF accept, and those escapes do not yet survive translation; the doc comment records the value to use once they do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `SQL_*_FUNCTIONS` bitmaps are defined by the spec in terms of the
ODBC escape -- "an application can determine which string functions are
supported by a driver by calling SQLGetInfo with an information type of
SQL_STRING_FUNCTIONS", and what it emits next is `{fn NAME(...)}`. This
driver read them instead as "does Trino have an equivalent", and
advertised twelve functions whose escape a client could not actually
use. Each was checked against a live coordinator:
{fn LOCATE('b','ab')} FUNCTION_NOT_FOUND: 'locate'
{fn POSITION('b','ab')} FUNCTION_NOT_FOUND: 'position'
{fn CURDATE()} FUNCTION_NOT_FOUND: 'curdate'
{fn CURTIME()} FUNCTION_NOT_FOUND: 'curtime'
{fn CURRENT_DATE()} SYNTAX_ERROR
{fn USERNAME()} FUNCTION_NOT_FOUND: 'username'
{fn DBNAME()} FUNCTION_NOT_FOUND: 'dbname'
{fn DAYOFWEEK(d)} FUNCTION_NOT_FOUND: 'dayofweek'
{fn TIMESTAMPADD(SQL_TSI_DAY, 1, t)} COLUMN_NOT_FOUND: 'sql_tsi_day'
Trino can do all of them, but only through `position(sub IN str)`, the
bare `current_date`/`current_user`/`current_catalog` keywords without
parentheses, or `date_add('day', 1, t)` with a quoted unit. None is a
rename, and `EscapeDialect::remap_scalar_fn` only swaps the identifier
in front of the parentheses -- it never sees the arguments. Restoring a
bit therefore means teaching core's translator to rewrite the whole
call, which is why the bits are dropped rather than the dialect
extended here.
`DAYOFWEEK` is dropped for a second reason: renaming it to Trino's
`day_of_week()` would succeed and return an ISO-numbered day (1 =
Monday) where ODBC specifies 1 = Sunday -- a silently wrong answer
rather than a loud failure.
The three snapshot tests over these bitmaps only restated the constants
they guarded, so they could not have caught the overclaim; they are
joined by untranslatable_escapes_are_never_advertised, which asserts
both that each name's bit is clear and that the dialect has no remap arm
for it. Verified by re-adding SQL_FN_STR_LOCATE and confirming the test
fails.
Also answers SQL_MULT_RESULT_SETS, SQL_NEED_LONG_DATA_LEN and
SQL_MAX_ROW_SIZE_INCLUDES_LONG with "N". All three are Y/N strings with
no arm in core's default_get_info, so they were reaching applications as
the empty string, which is not one of the two values the spec defines
for any of them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…re capabilities
Two rounds of core changes land here.
--- The escape rewrites ---
EscapeDialect::rewrite_scalar_fn receives a whole {fn NAME(args)} call, so
the twelve functions this driver had to stop advertising can be translated
rather than dropped. Each is verified end to end against a live coordinator
by every_advertised_scalar_function_escape_runs_on_trino, which executes
all 40 advertised escapes -- the check the bitmaps actually need, since the
unit tests can only say what text a rewrite produces, not whether it runs:
{fn LOCATE('b', 'ab')} position('b' IN 'ab')
{fn CURDATE()} and the CUR*/CURRENT_* the bare keyword, () removed
{fn USERNAME()} / {fn DBNAME()} current_user / current_catalog
{fn TIMESTAMPADD(SQL_TSI_DAY, 1, t)} date_add('day', 1, t)
{fn DAYOFWEEK(d)} ((day_of_week(d) % 7) + 1)
DAYOFWEEK is the one where the rewrite earns its keep. Trino has
day_of_week(), so a rename would have succeeded -- and returned an
ISO-numbered day where ODBC specifies 1 = Sunday. The conversion is asserted
against a known Monday rather than trusted.
POSITION needed nothing at all: ODBC spells it POSITION(exp IN exp), which
is already Trino's syntax. It was dropped for a calling convention it never
had. LOCATE comes back as SQL_FN_STR_LOCATE_2, the two-argument flag, since
ODBC's third argument is a start offset and strpos()'s is an occurrence
index.
split_args is the one piece of parsing this crate does that core declined
to, because only the dialect knows each function's arity. It is
literal-aware for the same reason core's translator is: a comma inside a
string, a quoted identifier, a comment or a nested call is not a separator.
Core's own worst case, {fn LOCATE(',', x)}, is a test case.
SQL_TIMEDATE_ADD_INTERVALS and SQL_TIMEDATE_DIFF_INTERVALS follow from
TIMESTAMPADD/TIMESTAMPDIFF working, and are held to the units the dialect
can rewrite. SQL_FN_TSI_FRAC_SECOND stays out: ODBC defines it as
billionths of a second and Trino's finest unit is millisecond.
--- The nine new capability hooks ---
Core's default_get_info_answers_are_backend_derived_or_declared_core_facts
turned nine more values into required methods. Each was measured:
- accessible_tables is now "N", changed from the "Y" core used to supply.
"Y" guarantees the connected user has SELECT on every table SQLTables
returns, which depends on the deployment's access control -- Trino filters
information_schema only when one is configured.
- concat_null_behavior is SQL_CB_NULL: concat('a', NULL) is NULL.
- subqueries claims every SQL_SQ_* bit, each executed.
- convert_functions is CAST only; CONVERT('1', INTEGER) does not resolve.
- The rest -- column_alias, union_support, order_by_columns_in_select,
data_source_read_only, search_pattern_escape -- keep the values the driver
already reported, now stated rather than inherited.
Also answers SQL_DATABASE_NAME with the connection's catalog, which core
cannot know: common_get_info_raw takes no connection, and its shared empty
string is only right for a backend that cannot name its current database.
TrinoConnection gains the catalog for it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Core's `Backend::keywords` hook replaces the shared empty string, which claimed this data source reserves nothing beyond ODBC's own list. It reserves 22 words ODBC does not -- UNNEST, LISTAGG, ROLLUP, the JSON_* family, both LOCAL* forms and the rest -- and applications read SQL_KEYWORDS to decide which identifiers need quoting, so the empty list could leave a generated identifier unquoted where it collides. Verified against a live coordinator: `SELECT 1 AS unnest` is a syntax error and `SELECT 1 AS "unnest"` is not. The list is static, unlike the SQLite driver's, which reads its keywords out of the linked library through sqlite3_keyword_name. Trino has no equivalent: system.jdbc -- the schema backing the JDBC driver's DatabaseMetaData, and so the one place such a list would live -- has no keywords table, nor does system.metadata, and this driver speaks HTTP so there is no library to ask. Trino's own JDBC driver hardcodes getSQLKeywords() for the same reason. The hook returns the raw 83 reserved words and core subtracts ODBC's 234, which is where the spec puts the rule: "this list does not contain keywords specific to ODBC or keywords used by both the data source and ODBC". Deliberately not gated on server_major, unlike SQL_SQL92_PREDICATES and SQL_SQL92_RELATIONAL_JOIN_OPERATORS. The safe direction is inverted here: over-reporting a keyword only makes an application quote an identifier it need not have, while under-reporting leaves a genuinely reserved word unquoted and the statement fails to parse. So it tracks the newest list. The drift that costs is small and additive -- of twelve words sampled against a live 467, eleven were already reserved and only AUTO was newer. The FFI test asserts the value through sql_get_info_w rather than only recomputing the subtraction in a unit test. Redoing the subtraction proves the list is right but not that core is asking this backend for it: before the hook existed core answered every backend with "" and SQLGetInfo still returned SUCCESS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…onestly The .mez overrode six SQLGetInfo values, and a Power Query override wins over SQLGetInfoW silently -- it cannot be corrected by fixing the driver. Two of them contradicted what the coordinator will actually accept. SQL_SQL92_PREDICATES was forced to 0x3FFF and SQL_SQL92_RELATIONAL_JOIN_OPERATORS to 0x3FF, every bit set. The driver gates both on the coordinator's version, because MATCH and UNIQUE arrived in Trino 482, OVERLAPS in 483 and CORRESPONDING in 475; the flat values re-asserted all of them on every server, including the 467 in test/docker-compose.yml. 0x3FF additionally claimed NATURAL JOIN, which a live 467 rejects with "NOT_SUPPORTED: Natural join not supported", and UNION JOIN, which has no production in Trino's grammar at any version. Power BI folding any of those emits SQL the coordinator refuses. Nothing Power Query actually generates is lost by removing them: comparison, IN, LIKE, BETWEEN, IS NULL, EXISTS and the four join types are all in the driver's ungated set. SQL_AGGREGATE_FUNCTIONS, SQL_SQL92_VALUE_EXPRESSIONS and SQL_IDENTIFIER_QUOTE_CHAR were overridden to exactly the values the driver already reports. Harmless today, duplicated state tomorrow. GroupByCapabilities is not a documented SqlCapabilities field. The GROUP BY relationship belongs in SQL_GROUP_BY, which the driver now answers with SQL_GB_GROUP_BY_CONTAINS_SELECT -- the same value that was sitting in the wrong record. Its comment claimed "Trino supports GROUP BY without restrictions", which is wrong either way: a non-aggregated select column absent from GROUP BY fails with EXPRESSION_NOT_AGGREGATE. SQL_SQL_CONFORMANCE stays overridden to SQL_SC_SQL92_FULL. The driver's own 0 and this 8 are both right for their audience: to an ODBC application the info type is a conformance claim Trino does not meet, while to Power Query it is the knob that unlocks SQL generation, and Microsoft's guidance is explicit -- "most drivers will want to report a SQL_SC_SQL92_FULL compliance level, and override specific SQL generation behavior using the SQLGetInfo and SQLGetFunctions properties". SupportsDerivedTable also defaults to false below that level, and derived tables are required for many DirectQuery scenarios. The stale comment claiming the driver reports "SQL_SC_MINIMUM (1)" is replaced with that reasoning. Not verified in Power BI: this repo has no way to drive it. Every claim about what Trino accepts was checked against a live 467 coordinator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Still needed, and load-bearing: 56 `#[serial]` attributes across `ffi_integration_tests.rs` plus the backend tests. The FFI tests share one ODBC connection through a `OnceLock` and the backend tests open their own `TrinoConnection`, so without serialisation two independent reqwest connection pools hit the same coordinator and intermittently corrupt a TCP socket -- the reason AGENTS.md tells you not to run the two suites together. No source changes: 4.0.1 needed no API adjustment here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Core gives every `Backend` and `StatementBackend` method one associated error type, requires `identifier_case`, and makes the descriptor and type-info structs non-exhaustive with builders. Adapt to all of it. `TrinoError` gains an `Odbc` variant and `From<OdbcError>`, which is what lets a defaulted trait body construct an error and still name `Self::Error`. It wraps rather than flattens, and the reverse direction unwraps it, so the SQLSTATE core chose survives the round trip instead of degrading to HY000. The hand-built internal-invariant errors keep building an `OdbcError` and converting: that preserves their exact message and SQLSTATE, including the 24000 on an abandoned result set, which no `TrinoError` variant expresses. `SQLDescribeCol` reports SQL_NULLABLE_UNKNOWN rather than SQL_NULLABLE. Trino's REST protocol describes a result column with a name and a type and nothing else, so neither definite answer has a basis, and the spec defines the third value for that case. `column_count` narrows to i16 here, where the real number is known, rather than leaving core to clamp a value it cannot interpret. `describe_col` hands back a borrow, which `SQLColAttribute` reads once per column per attribute. `close_cursor` reports a failed page drain after completing teardown, so a dirty pooled socket reaches the application without stranding the statement. The FFI tests move onto `test_support::attach_connection`, core's supported replacement for reaching into the private `handles` module, and detach before freeing because `SQLFreeHandle` refuses a connection handle that still holds a connection. Drop the `odbc-sys` dev-dependency in favour of core's re-export. The only use is `Timestamp`, read back out of a buffer core wrote, and nothing pinned the two declarations to one version -- two versions of a `#[repr(C)]` struct are two different layouts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`SQLGetDiagRec` reports the native error through `NativeErrorPtr`, and Trino's error taxonomy is the only thing that can meaningfully go there. Every failure reported `0` with the client error flattened into a message string, which is the one form neither the code nor the causal chain survives. `map_trino_error` gains a `Query` variant for everything it does not classify into a specific SQLSTATE, keeping the client error as the cause and lifting `QueryError::error_code` into the native error. The classified variants are untouched on purpose: `validate_connection` matches on them to keep an auth failure at 28000 rather than reclassifying it as 08001, so collapsing them would have moved that silently. Two limits are documented rather than worked around. PERMISSION_DENIED never arrives with a code, because the client turns it into `Error::Forbidden` and drops the code on the way; and a transport failure has no Trino code at all, so both report `0` -- the spec's value for "no native code". `sensitive_connect_keywords` declares the two bearer-token spellings. Core's substring heuristic already covers them, so nothing that leaked before stops leaking; it states this driver's own vocabulary instead of depending on another crate's pattern list to keep matching it. `get_functions` stays an explicit list rather than becoming `CORE_EXPORTED_FUNCTIONS`: core exporting a symbol says the entry point exists, not that this driver implements the function behind it, and 16 of the 69 are ODBC 2.x functions superseded in 3.x or descriptor-field functions with no handle to work on. Both directions are now pinned by tests, so neither the claim nor the withholding can drift unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pre-commit ran in its own workflow, and `finished` -- the single check branch protection requires -- did not list it in `needs`. Formatting, clippy and cargo-deny could therefore all fail while the required check reported success. Fold the job into this workflow, because `needs:` cannot cross workflows, and add it to `needs`. `finished` now derives its verdict from `join(needs.*.result, ' ')` instead of comparing two job names by hand, so a job added to `needs` and forgotten in the script cannot be silently non-blocking. That is the same shape as the bug above. Also: a concurrency group that supersedes in-flight PR runs but never cancels a merge_group run, since a cancelled one reports failure and evicts the PR from the queue; `timeout-minutes` on every job; `--locked` on the cargo invocations in CI and in the hooks, so a Cargo.toml change without a matching lockfile update fails here rather than drifting; and the redundant `token:` on checkout dropped, GITHUB_TOKEN being its default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AGENTS.md told a contributor to write `.map_err(|e| OdbcError::from(map_trino_error(e)))?` in functions returning `OdbcError`. Both traits give every method one associated error type, so no such function exists and the idiom is just `.map_err(map_trino_error)?`. Record what replaces it: when a hand-built `OdbcError` is still right (a SQLSTATE no `TrinoError` variant carries), and why `map_trino_error`'s classified arms cannot be collapsed -- `validate_connection` matches on two of them. Also document the things the compiler enforces but does not explain: the non-exhaustive types from core and their builders, why `TypeInfoRow`'s are `const`, that `odbc-sys` comes from core's re-export rather than a dependency of this crate, and that `test_support` is how a test puts a connection into a handle now that `handles` is private -- including the HY010 teardown that catches out the obvious spelling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`actions/checkout` defaults its `token` to `secrets.GITHUB_TOKEN`, so passing it explicitly states the default and reads as though some elevated credential were in play. The `token` on `rustsec/audit-check` stays: that one is the action's own input, which it needs to post advisory annotations onto the run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hheld `get_functions` was a hand-written list, with the sixteen functions it omitted recorded only as a set pinned inside a test and justified by one prose comment covering all of them. Split it into `TRINO_ADVERTISED_FUNCTIONS` and `TRINO_WITHHELD_FUNCTIONS`, the latter carrying a reason per entry, and assert that the two partition `CORE_EXPORTED_FUNCTIONS` exactly. The advertised set is unchanged: the same 53 functions, so `SQLGetFunctions` answers exactly as before. It stays opt-in rather than becoming `CORE_EXPORTED_FUNCTIONS` minus the withheld set, because the two directions fail differently. Deriving it means a function core exports later is advertised without anyone deciding it works, and the Windows Driver Manager builds its dispatch table from that answer. Listing it means such a function is merely not advertised -- and now lands in neither list, so the partition test names it and someone has to classify it. Mirrors core's own `CORE_EXPORTED_FUNCTIONS` / `CORE_UNEXPORTED_FUNCTIONS` pair and its `every_function_id_is_declared_exported_or_not`, one level up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adapts to five breaking changes in stackable-odbc-core. Four are mechanical; the cancel token is not. Backend::cancel took &mut Self::Statement, which SQLCancel cannot produce from a thread holding no lock on the connection while another thread executes on the same statement -- the case the ODBC spec singles out. It now receives a TrinoCancelToken built once per statement from the connection, carrying the client, the runtime and a shared CancelState. That state is load-bearing beyond naming the query. The old cancel also cleared next_uri, because polling get_next after a server-side cancel fails and leaves the pooled TCP socket carrying residual bytes, which surfaces later as an unrelated query failing. With no statement to mutate, cancel publishes a flag instead and fetch/close_cursor read it. exec_direct fills the query-id slot as soon as the coordinator accepts the query rather than after the metadata-polling loop, so a query cancelled while still queued is now reachable -- it was not before. begin_query clears the flag as well as setting the id, since core never replaces a statement's token across a re-execute. The six catalog functions take the token but record nothing in it: four do no I/O, and tables/columns page inside Client::get_all, which never surfaces a query id. They were not cancellable before either. The 25 required capability declarations, plus get_type_info and escape_dialect, take &Self::Connection. This driver reads it from none of them -- every value is a fact about Trino-the-engine or about this driver's own SQL generation -- but it is what a version-gated declaration would use, which is why TrinoConnection::server_major exists. Pre-connect, core skips every declaration needing a connection and substitutes its own benign default, so 22 of this driver's documented SQLGetInfo answers no longer appear before SQLDriverConnectW. None becomes SQL_ERROR; the connected answers are unchanged. get_info_snapshot now asserts the connected path, which is what that table has always been about. TypeInfoRow's four string-setting builders take impl Into<Cow<'static, str>>, and Into cannot run in a const context, so TRINO_TYPE_INFO moves from a static to info::trino_type_info() behind a OnceLock. TRINO_RESERVED_KEYWORDS stays a plain &[&str] so the list remains readable, with reserved_keywords() lifting it into Cows once. Adds cancel_from_another_thread_while_fetching, covering the scenario the whole token design exists for and the one place it could plausibly deadlock rather than cancel: two threads inside block_on on the same current-thread runtime. It needs a live Trino and has not yet been run. Lifts disconnected_trino_conn out of ffi_integration_tests into backend.rs; the unit tests need a connection now too, and it was about to be duplicated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three changes, landing together because core moved mid-task and the tree does
not compile without all of them.
1. A SQLFetch interrupted by a concurrent SQLCancel now reports HY008.
Running the cross-thread cancel test against a live coordinator for the first
time found a real gap. The cancel flag is set by the cancelling thread only
after its DELETE returns, by which point Trino may already have failed the
in-flight page request -- so fetch's flag check, which sits at the top of the
loop, never saw it, and the request surfaced as a raw USER_CANCELED under
HY000.
map_trino_error now recognises Trino's USER_CANCELED code and yields
TrinoError::OperationCancelled, which carries HY008. The spec gives that state
to a function interrupted by SQLCancel from another thread and does not mark
the clause (DM), so it is the driver's to report. Reading the server's verdict
rather than the local flag needs no cross-thread ordering and also catches a
query killed by something else, e.g. CALL system.runtime.kill_query.
end_page_fetch keeps that case off abandon_result_set: a cancellation the
application asked for leaves a finished result set, not the undefined cursor
position 24000 describes.
Core has no named constructor for HY008 because it documents HY008 as never
returned by a driver ("not applicable; the Backend trait is synchronous").
Cross-thread SQLCancel made that false, so this builds it with SqlState::new.
Worth raising against core.
2. string_parameter_with_quotes_is_not_injected was reading out of bounds.
It bound payload.as_bytes().to_vec() -- exactly 23 bytes, no terminator --
while passing a NULL StrLen_or_IndPtr, which SQLBindParameter defines as "the
data is null-terminated". Core therefore scanned for a NUL past the end of the
allocation and returned the payload plus whatever the heap held next, which
read as "payload was altered in transit". It passed alone and failed after 53
other tests had dirtied the allocator, which is why the handoff recorded one
sighting and dismissed it as an artefact; it reproduced 5/5.
The binding now passes an explicit indicator. The NULL-indicator form was
untested for strings, which is what let the bad binding look like a driver
bug, so string_parameter_bound_as_nts_is_not_injected covers it with a
properly terminated buffer.
3. Adaptation to core's sealing and constant export.
EscapeDialect, ColumnDescriptor and TypeInfoRow have pub(crate) fields and
accessors, so 51 field reads become calls. TypeInfoRow::type_name returns &str
rather than Cow, dropping the as_ref() the previous commit needed. SQL_ADD and
SQL_DIAG_MESSAGE_TEXT are gone from types, restated as
BulkOperation::Add as i16 and HeaderDiagnosticIdentifier::MessageText as i16,
which is what core's own note at the removal site directs.
Backend::describe_param is additive and defaulted, and this driver keeps the
default: Trino has no wire-level parameter binding, so it cannot describe a
parameter without parsing the SQL, and core's documented uniform fallback beats
a specific guess indistinguishable from a real answer.
Verified against a live coordinator: 244 offline, 8 backend, 55 FFI, with the
FFI suite run three times and the cancel tests eight.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SQL_CONVERT_FUNCTIONS has always reported SQL_FN_CVT_CAST, which advertises
the ODBC conversion escape, but escape_dialect.rs had no CONVERT arm. The
target type reached Trino as a bare identifier in an argument position, so
SELECT {fn CONVERT('1', SQL_INTEGER)} failed with COLUMN_NOT_FOUND on
sql_integer. Same class as the twelve scalar functions fixed earlier: a client
reading the bitmap is entitled to send this.
rewrite_scalar_fn is the hook that can express it -- the fix is not a rename
but a change of shape, from a two-argument call to a CAST -- and
trino_convert_target maps all 26 ODBC type keywords that have a Trino
equivalent.
Two mappings were measured against a live coordinator rather than read off the
documentation, and both would have been wrong the obvious way:
- SQL_CHAR maps to VARCHAR, not CHAR. A bare CHAR in Trino is CHAR(1), so
CAST('hello world' AS CHAR) returns "h". The escape carries no length to
give CHAR(n) instead, so mapping it to CHAR would have silently truncated
every conversion to one character -- worse than not translating at all.
convert_to_char_does_not_map_to_trinos_truncating_char pins this.
- SQL_FLOAT maps to DOUBLE. ODBC's FLOAT is double precision; Trino's REAL is
the single-precision type, which is SQL_REAL.
The SQL_INTERVAL_* keywords are left unmapped: no bare CAST reaches Trino's
interval types, so declining leaves the call on the fallback path rather than
casting to something the application did not ask for. A warn! names the
keyword when that happens, per the degraded-behaviour rule in AGENTS.md.
every_convert_escape_target_runs_on_trino covers the live side. It is a
separate test from every_advertised_scalar_function_escape_runs_on_trino
because CONVERT is advertised through SQL_CONVERT_FUNCTIONS, not through the
SQL_*_FUNCTIONS bitmaps that test walks -- which is precisely how the escape
stayed advertised and untranslated with a green suite. The binary targets read
back through to_hex, since Trino has no VARBINARY -> VARCHAR cast, which also
makes them assert a value instead of merely running.
Verified end to end through unixODBC: isql now answers
SELECT {fn CONVERT('1', SQL_INTEGER)} AS n with 1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… text JSON has no literal for NaN or the infinities, so Trino sends them as strings: a DOUBLE or REAL column carrying one arrives as "NaN", "Infinity" or "-Infinity". Confirmed off the wire rather than from the documentation -- a direct /v1/statement call returns [["NaN", "Infinity", "-Infinity", "NaN"]]. json_to_column_value had no arm for a string-valued float column, so the value fell through as ColumnValue::String and core refused String -> Double with 22018: the three values were simply unreadable as numbers. Both float arms now try trino_special_float when as_f64 declines, which covers REAL as well as DOUBLE -- both reached the fallback through the same as_f64. The spellings are matched exactly, not case-insensitively. These are the three Trino emits, and looser matching would silently turn some other data source's text into a number. The quoting the task brief flagged as a second, possibly wider problem turns out to be exactly that. Every fallback in this function rendered its value with Value::to_string(), which re-encodes a string as JSON, so any value that failed to convert reached the application carrying two quote characters it never sent -- "NaN" was merely the reachable instance. json_as_text yields the text Trino actually sent. The twelve remaining to_string() calls are in `else` branches where the value is provably not a string, so they already produce the same thing and are left alone. Verified through unixODBC: isql now answers CAST(nan() AS DOUBLE) with NaN rather than "NaN", and ieee_special_floats_are_readable_as_c_double reads all six DOUBLE/REAL cases as SQL_C_DOUBLE. One consequence worth recording rather than fixing here: read as text, an infinity now renders as "inf", which is Rust's Display. Trino and Java's Double.toString both spell it "Infinity". That conversion is core's F64 -> text path, not this driver's, and no ODBC text form is specified, so it is a consistency question for core rather than part of this fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AGENTS.md gains a section for the version resource, recording the three decisions that are easy to undo by accident: every value coming from cargo's environment so it cannot disagree with the package, release.toml deliberately having no rule because CARGO_PKG_VERSION already tracks the bump, and the gate being CARGO_CFG_TARGET_OS rather than cfg!(windows), which is false on the Linux host that cross-compiles the shipped DLL. It also records where the Administrator's buttons are tested, and why Cancel and Remove are asserted rather than photographed. WINDOWS.md gains the command, and says what the screenshots are for. The CHANGELOG entry states what an application sees: the driver lists with its version and Stackable GmbH instead of "Not marked" twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
System.Data.Odbc calls SQLDriverConnectW with SQL_DRIVER_NOPROMPT, the
same as pyodbc, so a connection made from the dialog's Test button may
never show a login URL. Measured through the Windows Driver Manager
against a live Keycloak, testing an ExternalAuthentication data source
returns
[28000] ExternalAuthentication needs to show a login URL, and this
connection was made with SQL_DRIVER_NOPROMPT; supply
AccessToken instead
which is correct, and a good diagnostic, and still arrives under a
"Connection failed" heading that reads as the settings being wrong. They
are not: the same data source connects and opens a browser from any
application that permits prompting.
The button now checks the keyword before connecting and reports that the
login cannot be driven from here, as information rather than a failure,
naming the reason and what to do instead.
Testing it for real would mean replacing System.Data.Odbc with a direct
SQLDriverConnectW at SQL_DRIVER_COMPLETE, which the script could do --
it already P/Invokes odbccp32 for ConfigDSN. That also means reading the
result columns through raw ODBC in PowerShell, so it is left until an
OAuth user asks for it, and AGENTS.md records the option.
Verified on the VM: the OAuth data source now reports "Cannot be tested
here", and dsn_dialog_test.py's ordinary password path still passes
every check, Test connection included.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Core is its own repository, so depend on it as one. A path dependency on `../stackable-odbc-core` required every contributor to clone a second repository by hand and could not resolve on a CI runner at all. `Cargo.lock` pins the resolved commit, so the build stays reproducible even though the dependency tracks a branch. `deny.toml` allows the repository by name, leaving `unknown-git = "deny"` to reject any other git source. To build against a local core checkout, use a `[patch]` in a gitignored `.cargo/config.toml` instead of editing `Cargo.toml`, so the override cannot be committed. Note that cargo rewrites core's `Cargo.lock` entry to a path while that patch is active. `packaging/test-sbom.sh` expected two path-sourced components. There is now one, the root package, and the assertion states the invariant it was really protecting: a path-sourced component other than the root package means a developer's local override reached a release artifact. A new check requires core's purl to carry a resolved 40 character commit rather than a branch name. `cargo publish` stays blocked until core reaches crates.io, which is what the remaining TODO tracks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The documentation was accurate but uniformly machine shaped, and much of it narrated a history no reader has: nothing has been released, so a before and after contrast describes a state nobody saw. Prose rules applied throughout: no em dashes, present tense describing present state, fact before reason, no "deliberately" or "load-bearing" or "worth knowing", and prose matched to its reader. SQLSTATEs and spec citations belong in AGENTS.md and in code comments, not in README.md. The measured facts are the value of this documentation and every one is kept; only the prose around them is compressed. CHANGELOG.md drops 974 lines of pre-release diff history for a short entry describing what the driver offers, plus its known limitations. README.md loses the internal reasoning and gains compatibility, troubleshooting and support sections. Building and testing move to a new CONTRIBUTING.md, which GitHub also links from the issue and pull request UI. AGENTS.md puts the architecture first and splits a 768 line "Conventions" section that was mostly design rationale. connector/README.md keeps the install path and moves 113 lines of maintainer notes to connector/DEVELOPING.md. packaging/README.md ships inside both release archives, so installation now precedes the build tooling. Nine factual errors found and fixed along the way: - integration-tests/README.md documented a --force-recreate flag that makes setup.sh exit 2 - WINDOWS.md expected ODBC_ERROR_INVALID_KEYWORD_VALUE, which predates the DSN dialog and contradicted the same file - connector/build.sh printed the wrong Get Data entry name - the CI TODO named a hive compose profile that does not exist - test_integration.py claimed SQLNumParams is unimplemented; it is implemented in core - UID and PWD are accepted but were documented nowhere - PARAM_EXTERNAL_AUTHENTICATION claimed User is required, where the code, the tests and AGENTS.md all make it optional - test_c_abi.py claimed the query timeout needs Threading = 2, where AGENTS.md records the opposite as measured - metadata.rs claimed both test catalogs return no table privileges; there are three, and hive returns rows Three doc comments were attached to the wrong item, which is why map_trino_error and cancel appeared to be undocumented. connector/StackableTrinoODBC80.png is removed. Power Query defines two icon groups, 16/20/24/32 and 32/40/48/64, and every connector in Microsoft's DataConnectors samples ships exactly those seven sizes. Nothing referenced the 80, and the Driver Manager never sees these icons: they are read from inside the .mez and the driver is listed from the VERSIONINFO resource instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
release.yaml called build-archives.sh but built with plain cargo, so the first tag push would have died in packaging: sbom.sh refuses a binary carrying no .dep-v0 section, and syft was not on the runner at all. Both tools are now installed at pinned versions, named once in the workflow's env, and both targets build with `cargo auditable build --locked --release`. --locked matters here beyond the usual reason: the SBOM describes what was linked, so an unlocked build would produce an accurate document about an unintended dependency set. The six SBOMs and sha256sums.txt were being generated and thrown away; they are release assets now. actions/attest-build-provenance signs where the archives came from and actions/attest-sbom binds each artifact to its CycloneDX document, which is what the job-level id-token and attestations permissions are for. The top level drops to contents: read, leaving publish-release as the only writer. Code signing stays a TODO: attestation proves where a binary was built, not who vouches for it. On the build side, windows-cross-compile becomes release-artifacts, which also builds the Linux release .so and runs sbom.sh --check-native on both. That fragment is hand-maintained, so nothing else keeps it true, and a pull request is where a dependency change can still be reverted cheaply. Runner images are pinned across all four workflows. windows-latest resolves to Windows Server 2025, while the driver's Windows behaviour was measured on a 2022 VM, so the suite now runs on windows-2022. The label cannot be lifted into a variable: runs-on accepts no env context, and vars would move the pin out of the repository. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A refused port, a certificate signed by an authority the client does not trust and a host that does not resolve all set reqwest's is_connect(), so one arm of map_trino_error classifies all three and the message is the only thing that separates them. It separated nothing: reqwest::Error's Display names only its own layer, so every one of them read "unable to reach Trino server: error sending request for url (...)", and CommunicationLinkFailure carries a String rather than a source, so core's Diagnostics had no chain left to walk. The information was discarded at the format!, not lost upstream. flatten_causes walks the chain into the message first. A refused port now ends in "tcp connect error: Connection refused (os error 111)", measured, and a rejected certificate carries whatever rustls says instead of vanishing. Segments already present are dropped, since the layers quote each other. This came from a report that a TLS misconfiguration and a dead server are indistinguishable in the Windows installer UI, which is true of anything holding only the diagnostic record. Two of the three cases stay indistinguishable and that is not fixable here: Jetty serves a different certificate for an SNI it cannot match, so connecting by IP fails as an unknown issuer rather than a name mismatch. The certificate wording is unverified against a live coordinator; only the refused-connection path is measured. The flattening itself is asserted on a synthetic chain, so it holds whatever rustls decides to say. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more sites stringified a reqwest error with its shallow Display, the same way the connect arm did. `connect`'s client build is the one that matters. reqwest::ClientBuilder::build reports a TLS trust store it cannot assemble as a bare "builder error", the client wraps that as Error::HttpError, and the reason a certificate was refused sits one source() down. Someone with a bad Certificate= therefore read "failed to build Trino client: http error, reason: builder error", which says less than the message that prompted this work. The timeout arm beside the connect arm has the same shape. Its detail is smaller, "operation timed out" against reqwest's "error sending request for url", but it separates a timeout waiting for a connection from one waiting for a coordinator that took the request and is still thinking. Its test builds a genuine reqwest timeout from a loopback listener that accepts and then answers nothing, rather than a synthetic error: a refused port yields a connect error instead, and no coordinator is needed to make a request outlive its own timeout. It costs under a second and reaches no network. The client build has no test. Both errors that reach it need crafted TLS material, since a certificate that fails to parse is rejected earlier by Ssl::read_pem, and the flattening itself is already covered. Arms that carry a source are untouched and need nothing: QueryCause::Transport holds the client error whole, so core walks that chain already. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verified against the compose stack rather than reasoned about. Each failure now ends in its own sentence, and the table names all four. One of them corrects what was written when the fix landed. The claim was that the two certificate cases collapse into one, on the grounds that an unmatched SNI reaches Trino's internal certificate rather than a mismatched name. The first half is right and the conclusion is not: reaching a different certificate is not the same failure as reaching an untrusted one, and rustls says so, rejecting the internal certificate as a CA presented as an end entity while an anchor that signed nothing is UnknownIssuer. What does stay indistinguishable is a Certificate= file that is not a certificate. It reads UnknownIssuer like an untrusted anchor, because reqwest defers parsing to the handshake instead of failing in Ssl::read_pem. The TLS suite passes unchanged against the live stack, 19 checks and 1 note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
run-tests.sh decided the stack was up from the presence of generated/stack.env. That directory survives a teardown, so after any earlier session the file was there and setup.sh was skipped with nothing running. The file header already claimed the behaviour this now implements. The failure mode is worse than a red run. Every check needing the coordinator fails with "unable to reach Trino server", but the checks asserting a refusal pass, because a refusal is what they assert and a dead port supplies one. The TLS suite reported 7 passed, 12 failed against an empty Docker, and two of those passes were the ones verifying that a bad trust anchor is rejected. Both conditions are now required, since neither implies the other: stack.env is what the suites read, and a running coordinator is what they talk to. The check reuses lib.sh's service_running rather than adding one. Verified both branches. With stack.env present and nothing running it prints "Stack configured but not running", calls setup.sh and the TLS suite passes 19/0; with the stack up it skips setup and passes 19/0 again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…elds Two defects in the options record introduced by 495fc4d, either of which made the connector unusable. asked M to construct a type from a type, so the section failed to compile and Power BI registered nothing -- the connector was absent from Get Data and any saved query reported that StackableTrinoODBC.Contents was not recognised. Naming a type-valued section member needs no prefix. The dialog supplies every declared field, null for those left blank, and Text.From(null) is null rather than , so every unset option reached Odbc.DataSource as a null-valued connection-string property and it refused the connection. Blank fields are now removed before the string is built; the unknown-key check still runs on the full supplied record so a misspelled option still errors.
The Windows runner ran test_integration.py and nothing else, while the Linux runner grew to eleven entries. Neither file said that was a decision rather than an oversight, and adding a suite meant editing one of them. suites/registry.py is now the only list. scripts/run-tests.sh reads it through --bash and windows/windows_test.py imports it, so a suite is added in one place. Each entry states its script, profile, how it takes its configuration, whether it needs pyodbc, and whether it runs on Windows; a suite that does not carries the reason, which the runner prints as a SKIP. The command stays with each runner. The two invoke Python differently, and the four-configuration connect matrix is not the same on both: Linux crosses DSN names out of the generated odbc.ini, Windows crosses a registry-registered DSN and connects by address for the unverified cases so that no SNI is sent. What kept the other suites off Windows was stack.env alone: the host's names the driver's .so, a localhost the VM is not, and certificate paths under the user's home directory. windows_test.py writes the VM its own, and mirrors the repository layout under C:\odbc_test_trino so that test_folding_contract.py's ../../connector/... and Stack's ../generated/stack.env resolve there as they do here. Stack gains DRIVER_NAME, because the Windows Driver Manager loads by registered name while the ctypes suites want the DLL's path. Windows now runs eight of the ten suites rather than one. test_harness.py is excluded as platform-independent Python, test_oauth.py until it has an odbc32.dll branch and an issuer the VM resolves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Picks up trino-rust-client 98b44b01, where a root certificate replaces the platform's roots instead of joining them. TlsVerify=full previously left the machine's CA store in the trust store alongside Certificate, so a chain that store trusted was accepted even though the connection had named a different anchor. Pinning a private CA therefore still accepted whatever any public CA issued for the same host. The Windows suite is what found it: the VM carries the test CA in Cert:\LocalMachine\Root for the OAuth2 browser login, and test_tls.py's "full refuses a trust anchor that did not sign the chain" passed on Linux and failed there. It passes on both now. README says what Certificate means, in the parameter table and in the TLS troubleshooting section: it names the anchor, so the machine's CA store stops applying to that connection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`parse_trino_date` strips a leading `-` before splitting, with a comment saying the driver has to be able to read back what it emits. `parse_trino_timestamp` did a bare `splitn(3, '-')`, so the first field of `-0001-01-01 00:00:00` was empty, the parse failed, and the value degraded to text. The driver emits exactly that form: `backend::params` renders a bound `SQL_TIMESTAMP_STRUCT` through the same `year4` it uses for a `SQL_DATE_STRUCT`. So a bound BC timestamp did not round-trip, while a bound BC date did, which is what kept it invisible. Both now go through one `parse_ymd`, so the two cannot disagree again, and `dates_and_timestamps_read_the_same_years` asserts that directly. The sign is applied with `checked_neg`, because `i16::MIN` has no positive counterpart. Also corrects the `TrinoTypeName::Char` comment, which claimed the `TrinoTy::Char -> EXT_W_VARCHAR` mapping was the query path. `execute.rs` prefers `TrinoTypeName::parse`, which answers `EXT_W_CHAR` for a `char(n)`; the `TrinoTy` arm is the fallback for a signature the parser cannot read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…seed
ODBC's `RAND` takes an optional *seed* and returns a float in `[0, 1)`.
Trino's `rand(n)` takes a *bound* and returns an integer in `[0, n)`, so
`{fn RAND(5)}` passed through verbatim answered a different type over a
different range, and no error revealed it.
Trino has no seeded generator to rewrite the seed onto, so the seed is
dropped and a bare `random()` emitted. That keeps ODBC's type and range
and loses only reproducibility, which is the better of the two failures.
The zero-argument form still passes through untouched.
Also documents `SQL_FN_STR_SOUNDEX`, which was the one name in
`TRINO_STRING_FUNCTIONS` with no justification beside it. Trino gained
`soundex()` in 356 (April 2021, trinodb/trino#4022), so unlike
`CORRESPONDING` (475), `MATCH` and `UNIQUE` (482) and `OVERLAPS` (483) it
needs no `server_major` gate: no coordinator this driver supports predates
it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`TransactionState::aborted` is meant to say "the transaction open now has been aborted". It was set whenever autocommit was off, whether or not one was open, and only `ended()` cleared it, which runs from `end_tran` and returns early when nothing is open. Manual-commit mode is not the same as having a transaction: the mode is set by `SQLSetConnectAttr` and a transaction opens at the first statement that needs one, so there is a window with the mode on and nothing begun. A failing catalog lookup or `DESCRIBE INPUT` in that window set the flag, and the next transaction was born aborted: its statements all succeeded and `SQLEndTran(SQL_COMMIT)` rolled them back with 25S03. `begun()` now clears it wherever a transaction opens, and `end_tran` clears it on the path where none was, so the flag is unobservable outside the life of a transaction. Three further fixes in the same area: - `set_autocommit` records the mode even when the commit it makes first fails. `end_tran` has already moved the epoch, cleared the flag and, in the case that fails most often, sent the rollback, so an early return left the connection in manual-commit mode with nothing open while the application had been told the switch failed. - Trino carries the transaction id in a session header, so the driver's own `information_schema` queries and its `PREPARE` / `DESCRIBE INPUT` round trip join whatever the application has open, and a failure in either aborts it. Both used to substitute a success -- an empty result set, and core's uniform `VARCHAR` guess -- which reported success over a transaction they had just killed. Both substitutions are now conditional on no transaction being open. Suppressing the abort instead was rejected: Trino really did abort it. - `describe_param` strips the trailing statement terminator, as `exec_direct` already did. Trino's grammar has none, so `PREPARE x FROM SELECT ? ;` is a syntax error and a statement an application could prepare and run was one `SQLDescribeParam` could not describe. Two connection-string keys are tightened to match the policy the rest of the parser already follows: - `Certificate` and `ClientCertificate` are refused with `Protocol=http`. `connect` applies the TLS group only when the transport is secure, so neither file was opened and an unreadable or wrong-CA path went unreported, while the operator believed the connection was verified against it. `TlsVerify` and `SSLVerification` are deliberately tolerated: they are equally inert but harmlessly so, and `configure-dsn.ps1` writes one into every data source it produces, so refusing them would refuse plaintext data sources the driver's own dialog wrote. The dialog mirrors the rule so it is reported while the fields are on screen. - `QueryTimeout` refuses a value it cannot parse instead of logging a `warn!` no application sees and silently using 30s, which is what `MaxAttempts` and `ExternalAuthenticationTimeout` already do. Blank is unset rather than invalid, for the ODBC-standard `LoginTimeout` spelling that a DSN editor may write empty, and `0` stays legal as the spec's "no timeout". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`StackableTrinoODBCImpl` fell back to `User=trino` for every credential
kind but UsernamePassword. `ExternalAuthentication` is offered in
`Config_AdvancedOptions`, and `connect_params.rs` documents why a name
must not be invented there: Trino reads a `User` that disagrees with the
token's identity as an impersonation request and refuses it. So the only
Power BI route to OAuth 2.0 failed every time, with the exact error the
driver goes out of its way to avoid. An absent user is now left absent.
Three values were wrong in ways nothing would report:
- The `Roles` sample was `hive:ROLE{admin}`. `selected_role` renders
Trino's `ROLE{...}` wire form itself, so the sample double-wrapped, and
the braces collide with connection-string syntax besides.
- The TIMESTAMP and TIME `Constant` entries rendered through
`.sssssss`. In a .NET custom format string `s` is the second and `f` is
the fractional second, so those two of the twelve folding entries built
a literal with no fraction in it. `test_folding_contract.py` now
translates each temporal format and sends the rendered literal to Trino,
rejecting a run of `s` where a fraction belongs.
- The conformance-override comment justified itself with "the driver
reports SQL_TC_NONE where Entry level requires COMMIT/ROLLBACK". It
reports `SQL_TC_DML`, and `sql_conformance` says the opposite: the
COMMIT/ROLLBACK requirement is met and the constraint grammar is the
whole argument. Stale from before transactions landed, and load-bearing,
since it justifies the one override that silently outranks SQLGetInfo.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three checks reported success regardless of the outcome:
- `install.bat` graded `odbcconf.exe /A {INSTALLDRIVER ...}` on its exit
code. odbcconf exits 0 whether or not the action succeeded, so a failed
registration printed "installed successfully" and the driver was then
simply absent from the ODBC Administrator with no explanation, which is
the first symptom README.md's troubleshooting section covers. It now
reads the registry: the driver's own key, and its entry in the `ODBC
Drivers` listing the Drivers tab is populated from. `windows_test.py`
works around the same unreliability by force-writing those values after
its own odbcconf call, which is what prompted the check.
- `uninstall.bat` deleted only the DLL, while `install.bat` places two
files and refuses to run without both. It now removes `configure-dsn.ps1`
and the directories it created, when empty.
- build.yaml's "Verify DLL exports" piped a symbol count into `xargs
echo`, and the step was graded on `xargs`. A DLL exporting nothing at all
passed. It now asserts 23 named entry points, `ConfigDSNW` among them,
plus a floor of 55; the Linux .so has 60 and the DLL 61.
The Windows VM harness never ran the shipped installers -- it registers
the driver its own way -- so `check_installers` now stages the four files
the release zip puts side by side, runs install.bat and uninstall.bat, and
reads the state each left. It runs before `register_driver`, because the
uninstaller deregisters the driver every later suite depends on.
release.yaml's archive check now asserts the Linux tarball carries no
`.mez`, rather than only looking for files it expects to find. README.md
spent this branch claiming it did; a check that only detects absences
cannot catch a promise about a file that was never there. `configure-dsn.ps1`
is added to the Windows list, since install.bat hard-requires it.
`build-archives.sh` clears `packaging/dist` before assembling. The
checksum step globs the whole directory, so a local re-run at a different
$VERSION checksummed both versions into one manifest. CI never saw it;
the person following packaging/README.md does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three suites, for three surfaces that had no live coverage at all.
**Escape sequences.** `info.rs` states the rule the capability bitmaps
follow: a bit is set only when the escape becomes Trino SQL that runs.
Nothing checked that against a coordinator. The Rust side compares a
bitmap against a list of names and the rewriter's output against expected
strings, so an argument order, a unit spelling or a value conversion could
be wrong in both places and agree with itself. `test_escapes.py` parses
the bitmaps out of the driver's own source and executes one `{fn ...}` per
bit, asserting the value wherever the dialect transforms rather than
renames: `DAYOFWEEK` renumbering Sunday to 1, `TIMESTAMPDIFF` counting
b - a, `LOG` being `ln`, `LOCATE`'s argument order, and `RAND` dropping its
seed. It also covers every advertised `SQL_TSI_*` unit in both directions,
all 26 `{fn CONVERT}` targets, and the `{d}` `{t}` `{ts}` literals. A bit
added without a probe fails the suite rather than shipping unchecked.
114 assertions.
**Session keys.** Seventeen connection-string keys were parsed, unit-tested
and never once shown to reach Trino. `test_session_keys.py` covers each by
the strongest assertion Trino allows: observed for `TimeZone`, `Path`,
`Source`, `Locale`, `SessionProperties` and `Roles`; refused-on-a-bad-value
for `ResourceEstimates` and `ExtraHeaders`, where a rejection proves the
good value travelled too; and accepted, recorded as a NOTE rather than a
PASS, for the seven Trino exposes nothing for. `AccessToken` and the proxy
keys are explicit SKIPs with reasons.
**SQLDescribeParam.** ctypes, because pyodbc exposes it not at all. Covers
the `PREPARE` / `DESCRIBE INPUT` / `DEALLOCATE` round trip, the
per-connection cache, the trailing terminator, and both failure paths --
falling back outside a transaction and reporting inside one. Each type
probe also asserts the answer is *not* core's uniform `VARCHAR` fallback,
which is what the round trip exists to avoid: with the terminator strip
reverted, the semicolon probe returns `SQL_VARCHAR(4000)` instead of
`SQL_BIGINT`, so it is a real regression guard.
`test_sql_surface.py` gains batched parameters, which the changelog claims
and nothing covered: every set inserted, values kept with their own row,
and quotes and NULLs escaped per set.
integration-tests/README.md records that `Protocol=http` stays uncovered
by decision rather than oversight.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
README.md said "Both archives contain StackableTrinoODBC.mez". `build-archives.sh` puts it in the Windows zip and publishes it standalone; the Linux tarball has never carried it, and packaging/README.md and AGENTS.md both say so correctly. The top-level README was the outlier, and it is the one a Linux user reads before going looking for a file that is not there. It now states where the connector ships and that Power BI Desktop is Windows-only. CONTRIBUTING.md called pre-commit "the single source of truth for what must pass" and then listed five of its sixteen hooks. `cargo doc` with warnings denied, `cargo sort` and `cargo deny` are the three most likely to surprise a first contributor, and two of them need installing, which nothing said. The `Certificate`, `ClientCertificate` and `QueryTimeout` rows in the connection-option table are updated for the validation those keys now carry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four advertised scalar functions did not do what the ODBC appendix
specifies, and each failed quietly rather than visibly.
TRUNCATE was not merely inexact, it did not resolve. Trino declares the
two-argument `truncate` over `decimal` alone, so `{fn TRUNCATE(x, 1)}`
over a double or real column fails FUNCTION_NOT_FOUND, while ODBC's
`numeric_exp` covers SQL_FLOAT, SQL_REAL and SQL_DOUBLE. The escape now
scales into the single-argument form, which Trino does define over every
numeric type. The scale factor is an integer literal rather than
`power(10, d)`, and that choice is what keeps ODBC's "returns values of
the same data type as the input parameters": Trino promotes
`decimal * bigint` to decimal and `real * bigint` to real, where a
double-valued factor drags both to double. A digit count that cannot be
folded, a column or a parameter marker, and any `|d| > 18`, still take
the `power` path and widen, which remains better than not resolving.
LENGTH, LTRIM and RTRIM all turn on ODBC's word "blanks", which means the
space character alone. Trino reads all three as whitespace-wide, so
`length()` counted trailing spaces the spec excludes and the one-argument
trims ate tabs and newlines the spec keeps. Measured against a
coordinator, `length(CAST('ab' AS char(5)))` answered 5 where ODBC asks
for 2, and `length('abc ')` answered 6 where it asks for 3, so the gap
was never confined to padded char(n). Each now goes through the
two-argument trim with an explicit space.
The suites gain cases that tell the readings apart: the trims carry a tab
that has to survive alongside spaces that have to go, LENGTH carries
trailing blanks, and TRUNCATE asserts `typeof` across decimal, double and
real at three digit counts, because the value alone cannot distinguish an
integer scale factor from a double one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ings An audit of all 54 advertised scalar functions against the ODBC appendices left three names where the driver's answer is a decision rather than a match, and none of them said so. ATAN2 is the one place this driver knowingly departs from the appendix text. That text reads `ATAN2(float_exp1, float_exp2)` as "the arctangent of the x and y coordinates ... respectively", putting x first, where Trino's `atan2(y, x)` puts y first. No implementation honours the sentence: PostgreSQL, MySQL, Oracle, C, Java, Python and SQL Server's own ATN2 all take (y, x), and ATN2's documented example evaluates `ATN2(129.44, 35.175643)` to 1.30545, which is atan(129.44/35.175643). Microsoft's engine therefore contradicts Microsoft's appendix. psqlodbc, which remaps LOG, LENGTH and DAYOFWEEK exactly as this module does, carries ATAN2 only as a commented-out `built_in`. Swapping would make this the single driver in the ecosystem answering the complementary angle, so the call passes through and the reasoning now sits next to the arm it deliberately does not have, with unit tests asserting both hooks decline it. ROUND looked like the same trap as TRUNCATE, since ODBC mandates rounding to the left of the point for a negative digit count and Trino's math reference documents that for `truncate` alone. Measured, `round` accepts one too, over double, real, decimal and bigint, and returns the input's type. Scaling it the way TRUNCATE is scaled would break that type preservation for no gain, so it stays unrewritten and the test now pins the negative case the docs omit. WEEK is ISO-numbered, and the existing note called the divergence "off by one". At a year boundary it is a whole year: `week(DATE '2021-01-01')` is 53 where a convention starting week 1 on January 1 answers 1. ODBC fixes only the 1-53 range, which ISO numbering stays inside, and never names a convention, so the behaviour stands and the note now says what it does. The ATAN2 suite case moves off `(0, 1)`, whose two readings differ by an answer of 0 that many wrong paths produce, onto `(1, 2)`, where they differ by 0.64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The interval variants of `ColumnValue` changed shape: `IntervalDayTime` counts signed `i128` nanoseconds rather than `i64` milliseconds, and both variants now carry the `Interval` precision the spec's two *SQL to C: Intervals* tables turn on. Trino has exactly one type per family and each spans its family's full field range, so the precision is `YearToMonth` and `DayToSecond` and nothing narrower has a Trino column type to come from. The day-time parser no longer truncates at three fractional digits. It did so because the variant held milliseconds, under a comment claiming ODBC's `SQL_INTERVAL_DAY_TO_SECOND` could carry no more; `SQL_INTERVAL_STRUCT`'s `fraction` field counts billionths of a second, so that was never true. Trino renders this type with three digits, its own storage being a millisecond count, so nothing observable changes today, and `parse_fraction_nanos` now reads the fragment on the same rule the temporal parsers use rather than a second padding rule that could drift from it. One rendering does change, in core rather than here: an `INTERVAL YEAR TO MONTH` reads as `3-07` where it read as `3-7`, core having moved to the spec's *Interval Literals* rule of an unpadded leading field and two-digit trailing fields. Day-time is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Trino's temporal types reach twelve fractional digits and the client always
advertises `PARAMETRIC_DATETIME`, so a `timestamp(12)` column arrives on the
wire with all twelve:
columns: [('t12', 'timestamp(12)'), ('ti12', 'time(12)')]
data : [['2020-01-02 03:04:05.123456789012', '03:04:05.123456789012']]
`ColumnValue::Timestamp::fraction` counts nanoseconds, so `parse_fraction_nanos`
keeps nine of them, while `type_name_scale` reports the column's scale as the
declared twelve. The read answered `SQL_SUCCESS` with three digits silently
gone. The loss happens inside this driver's own conversion, before a
`ColumnValue` exists, which is exactly what core cannot see and what
`StatementBackend::take_value_warning` was added for.
Reported per value, not per column. Asking the declared scale instead would
put `01S07` on every row of a `timestamp(12)` column, including the values
whose discarded digits were zeros and which therefore lost nothing; core's own
`01S07` is value-precise for the same reason. So `convert_rows` records the
cells whose wire text carried a non-zero digit past the ninth, that being the
only point the text still exists, `get_data` arms the warning from the record
and `take_value_warning` hands it back.
The record is a set rather than a flag per cell because only a `time` or
`timestamp` column declared beyond nine digits can put anything in it: for
every other result set it stays empty, allocating nothing, and `get_data` pays
one length check per value rather than a hash. The composite types are walked,
since a `timestamp(12)` inside a `ROW` reaches the same parser.
Both halves are pinned against a live coordinator: a value with digits past
the ninth answers `SQL_SUCCESS_WITH_INFO` with `01S07` and still delivers its
nine, and one padded with zeros answers a plain `SQL_SUCCESS` with an empty
diagnostic queue.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The repository is public now, and its `scaffolding` branch is gone: the remote has only `main`, at the commit `v0.1.0` points to. The branch dependency therefore no longer resolves at all. A tag rather than `main`, so which core a build takes is stated in Cargo.toml and moving to a newer one is a reviewable edit, instead of depending on where a branch happened to point when the runner cloned it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…Cargo.toml Four defects, all of which would have surfaced on the first release. `cargo release` aborted. A pre-release-replacement searched packaging/README.md for a literal `stackable-odbc-trino-<n.n.n>-linux-x64 .tar.gz`, but that file names artefacts with a `<version>` placeholder because it documents a naming scheme rather than one release, so the rule matched nothing and `exactly = 1` failed the run. Both README rules are gone: the build command now takes its version from Cargo.toml, so the file holds no release version to rewrite. build-archives.sh trusted $VERSION and never compared it to the crate version, so it would name an archive 0.1.0 around a DLL whose VERSIONINFO resource said 0.0.1. The crate version is now the default, since it is the only value that can be correct, and a $VERSION that disagrees is refused. release.yaml still passes one explicitly, from a tag verify-version has already checked, and that agreeing value is now verified rather than assumed. test-sbom.sh hardcoded the trino-rust-client commit and went stale when the fork was bumped, reporting a defect that was only a stale expectation. Both purl checks derive the expected value from Cargo.lock, which still asserts the real property because the commit comes from the lockfile rather than from the SBOM under test. `--locked` was in release.yaml alone. Every local instruction and script omitted it, so a local build could resolve a different dependency set than the released one while test-sbom.sh asserts an exact component count. The flag, and the pinned syft and cargo-auditable versions, now match CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Extracts
stackable-odbc-trinofrom the three-cratestackable-odbc-rsworkspace (core + trino + sqlite) into this standalone, releasable repository,
and adapts it to the newer
stackable-odbc-coreAPI.SQLite follows later, separately.
Layout
Flat single-crate, mirroring
stackable-odbc-core'sscaffoldingbranch — no[workspace]table, nocrates/nesting.test/trino/flattens totest/,since there is no sibling suite here to disambiguate from.
Moved across: the driver sources, the Criterion benchmark, the Power Query
connector, the release packaging scripts, the Docker integration suite and the
Windows VM harness. Left behind deliberately:
fuzz/(both targets exercisecore APIs), the other two crates, and the workspace tables.
Reviewer notes
CI will be red, by design.
stackable-odbc-coreis still private, so it isa path dependency on a sibling checkout and does not resolve on a runner.
cargo publishis blocked for the same reason. The workflows are written forthe published world, so the single
TODOinCargo.toml— swapping the pathdep for a version dep — is the only change needed to make both work.
Version resets to
0.0.1and the manifest gains the metadata crates.iorequires:
description,repository,readme,keywords,categories,rust-version.authorsandlicensewere workspace-inherited and are nowexplicit.
Adapting to core's newer API
Core gained a breaking change since this driver last built against it:
default_get_infoandcommon_get_info_raware now generic over the backendSQL_CURSOR_COMMIT_BEHAVIORis derived fromBackend::cursor_commit_behaviorrather than hard-coded. This driver reports
SQL_TC_NONE, so core'sCursorBehavior::Preservedefault is correct and the stale part was thesnapshot expectation —
SQL_CB_PRESERVE(2), notSQL_CB_DELETE(0).Fixes found during the move
These were latent or newly broken by the shallower paths, not pre-existing bugs
carried over:
PROJECT_DIRovershoot —test/setup.sh,test/run-tests.shandtest/windows_test.pycomputed$SCRIPT_DIR/../.., correct attest/trino/but one level above the repo root at
test/. Would have built in the wrongdirectory and looked for the DLL outside the repository.
windows/WINDOWS.mdomittedAccessToken/TokenandQueryTimeout/LoginTimeout, all four of whichconnect_params.rshas always accepted.cargo-denyallowances — theringandOpenSSLlicenceallowances were carried over on the assumption
trino-rust-clientpullsring. It does not: rustls 0.23 resolves toaws-lc-rs, andringis absentfrom the tree entirely. Removed and re-verified against all three targets.
What remains genuinely trino-specific versus core's config is the
x86_64-pc-windows-gnutriple and theRUSTSEC-2024-0436ignore for theunmaintained transitive
paste.SQL_TC_NONEwith"Trino has no explicit transactions". Trino does support transactions over
the
X-Trino-Transaction-Idheaders; it is this driver that does notimplement them. Reported capability unchanged, reasoning corrected.
#[ignore]reasons, doc comments,
cargo -p/--workspaceinvocations), plus theSQLite half of
WINDOWS.md.Tooling
check-yaml,check-toml,check-merge-conflict,mixed-line-ending,cargo-sort), plusshellcheckre-added — this repo ships nine shell scripts where core had none.
build.yaml(unit tests + Windows cross-compile),pr_pre-commit,security_audit, andrelease.yamltriggering onv*. No Miri or fuzz —Miri cannot execute the OpenSSL and aws-lc-rs code
trino-rust-clientlinksin, and both fuzz targets belong to core.
cargo-release(release.toml+release/release.sh) cuts asigned
v<version>tag, which firesrelease.yamlto build both binaries andpublish the archives.
README.mdfor a standalone driver repo, a driver-focusedself-contained
AGENTS.md,CLAUDE.md, and a Keep a ChangelogCHANGELOG.md.The Trino integration suite is not run in CI — the Docker Trino + Postgres
stack exceeds a standard runner. The existing
TODO(@maltesander)is carriedinto
build.yaml, and the suite is documented as a pre-release local step.Verification
All run locally against this branch:
cargo testcargo clippy --all-targets -- -D warningscargo fmt --check,cargo sort,cargo deny checkcargo doc --no-deps,cargo bench --no-runpre-commit run --all-filesactionlintConfigDSNWexportedrelease.yaml's sanity check passescargo-releasedry runIntegration suites, both green:
Follow-ups
CI and
cargo publish.trino-rust-clienttransaction fixships.
TrinoBackend::end_tranlists every site that needs revisiting.