From 0e858e6d5e9b32edcd2076414d76dada7b84d5cd Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 6 Oct 2026 17:34:22 +0200 Subject: [PATCH 01/20] fix: lead each shared SQLGetTypeInfo DATA_TYPE with its plain type --- Cargo.lock | 2 +- Cargo.toml | 7 +- integration-tests/suites/test_sql_surface.py | 14 ++++ src/backend/info.rs | 88 +++++++++++--------- src/ffi_integration_tests.rs | 53 ++++++++++++ 5 files changed, 122 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f5475e5..9000c4a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1971,7 +1971,7 @@ checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" [[package]] name = "stackable-odbc-core" version = "0.1.0" -source = "git+https://github.com/stackabletech/stackable-odbc-core.git?tag=v0.1.0#23c924489e135d1d3da1d1664ae16bf8656d5aa3" +source = "git+https://github.com/stackabletech/stackable-odbc-core.git?rev=23f055e451cde6b842ad117f5c8a16957abe113f#23f055e451cde6b842ad117f5c8a16957abe113f" dependencies = [ "odbc-sys", "snafu", diff --git a/Cargo.toml b/Cargo.toml index 663ee78..7ec1fca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,7 +38,10 @@ snafu = "0.9" # reviewable edit to this line instead of whatever the branch happens to point # at. To build against a local checkout, use a `[patch]` in your own # `.cargo/config.toml` rather than editing this line; see CONTRIBUTING.md. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", tag = "v0.1.0" } +# TEMPORARY: pinned to the core commit adding `TypeInfoRow::with_preferred` +# until a core release containing it is tagged; switch both entries back to +# that tag before merging. +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "23f055e451cde6b842ad117f5c8a16957abe113f" } # `time` is needed directly by `query_all_rows_within`, which bounds the login # round trip with `tokio::time::timeout`. It resolves without being declared, # because reqwest enables it, but a direct use must not rely on another crate's @@ -60,7 +63,7 @@ serial_test = "4" # offline FFI tests need. It is default-off because it is test code, so it is # enabled here rather than on the [dependencies] entry above -- that keeps it # out of the shipped cdylib. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", tag = "v0.1.0", features = ["test-support"] } +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "23f055e451cde6b842ad117f5c8a16957abe113f", features = ["test-support"] } [lints.clippy] unwrap_in_result = "deny" diff --git a/integration-tests/suites/test_sql_surface.py b/integration-tests/suites/test_sql_surface.py index cde5b31..277037e 100755 --- a/integration-tests/suites/test_sql_surface.py +++ b/integration-tests/suites/test_sql_surface.py @@ -289,6 +289,20 @@ def prepared_handle_is_reusable(): cur.getTypeInfo().fetchall() or (_ for _ in ()).throw(AssertionError("no type info")))) + def type_info_leads_wvarchar_with_varchar(): + """Power Query takes the first SQLGetTypeInfo row for a DATA_TYPE as its + CAST target. For SQL_WVARCHAR that must be VARCHAR, not one of the + Trino types the driver also renders as text (INTERVAL DAY TO SECOND + sorted first before core ranked a preferred row). + """ + SQL_WVARCHAR = -9 # as in test_describe_param.py + first = cur.getTypeInfo(SQL_WVARCHAR).fetchone() + assert first is not None, "no SQL_WVARCHAR rows" + assert first[0] == "VARCHAR", f"first SQL_WVARCHAR row is {first[0]!r}" + + R.run("SQLGetTypeInfo leads SQL_WVARCHAR with VARCHAR", + type_info_leads_wvarchar_with_varchar) + def datetime_columns_report_the_verbose_type(): """SQLColumns and SQLGetTypeInfo must not disagree about a datetime. diff --git a/src/backend/info.rs b/src/backend/info.rs index d6e823c..430dc49 100644 --- a/src/backend/info.rs +++ b/src/backend/info.rs @@ -85,12 +85,12 @@ const TRINO_TZ_TIMESTAMP_SPACE_LEN: i32 = 1; /// COLUMN_SIZE must never be a literal copied from the per-column path (or /// vice versa). /// -/// Rows are sorted by DATA_TYPE ascending (as signed i16, so ODBC extension -/// types with negative codes sort first), then by TYPE_NAME ascending within -/// an equal DATA_TYPE, per the SQLGetTypeInfo spec's "ordered by DATA_TYPE and -/// then ... TYPE_NAME" requirement. This invariant is asserted directly by -/// `type_info_rows_sorted_by_data_type_then_type_name` below; keep new rows -/// in the correct sorted position rather than appending them. +/// Core orders the result set itself (DATA_TYPE, then the row marked +/// `with_preferred`, then TYPE_NAME; see +/// `stackable_odbc_core::ffi::info::sql_get_type_info`), so rows are grouped +/// here for reading, not for the spec. Every DATA_TYPE shared by several rows +/// marks its closest match, as `every_shared_data_type_has_one_preferred_row` +/// asserts. fn trino_type_info() -> &'static [TypeInfoRow] { static ROWS: OnceLock> = OnceLock::new(); ROWS.get_or_init(|| { @@ -148,6 +148,8 @@ fn trino_type_info() -> &'static [TypeInfoRow] { MaxScale(0), )) .with_literal_affixes(Some("'"), Some("'")), + // The closest match for SQL_WVARCHAR, so core orders it first + // among the types this driver renders as text. TypeInfoRow::new(TrinoTypeName::Varchar.name(), SqlDataType::EXT_W_VARCHAR) .with_column_size(catalog_column_size( SqlDataType::EXT_W_VARCHAR, @@ -156,7 +158,8 @@ fn trino_type_info() -> &'static [TypeInfoRow] { )) .with_literal_affixes(Some("'"), Some("'")) .with_create_params(Some("max length")) - .with_case_sensitive(true), + .with_case_sensitive(true) + .with_preferred(true), TypeInfoRow::new(TrinoTypeName::Char.name(), SqlDataType::EXT_W_CHAR) .with_column_size(catalog_column_size( SqlDataType::EXT_W_CHAR, @@ -289,7 +292,8 @@ fn trino_type_info() -> &'static [TypeInfoRow] { .with_literal_affixes(Some("TIME '"), Some("'")) .with_create_params(Some("precision")) .with_scale_range(Some(0), Some(MAX_FRACTIONAL_SECONDS_PRECISION)) - .with_verbose_type(SqlDataType::DATETIME.0, Some(SQL_CODE_TIME)), + .with_verbose_type(SqlDataType::DATETIME.0, Some(SQL_CODE_TIME)) + .with_preferred(true), // TIME WITH TIME ZONE shares plain TIME's DATA_TYPE (see // TrinoTypeName::sql_type) and still needs its own row: an // application looking SQLGetTypeInfo up by TYPE_NAME, to build a @@ -317,7 +321,8 @@ fn trino_type_info() -> &'static [TypeInfoRow] { .with_literal_affixes(Some("TIMESTAMP '"), Some("'")) .with_create_params(Some("precision")) .with_scale_range(Some(0), Some(MAX_FRACTIONAL_SECONDS_PRECISION)) - .with_verbose_type(SqlDataType::DATETIME.0, Some(SQL_CODE_TIMESTAMP)), + .with_verbose_type(SqlDataType::DATETIME.0, Some(SQL_CODE_TIMESTAMP)) + .with_preferred(true), // TIMESTAMP WITH TIME ZONE shares plain TIMESTAMP's DATA_TYPE, // for the reason TIME WITH TIME ZONE above gives. TypeInfoRow::new( @@ -1147,8 +1152,9 @@ pub(super) fn get_type_info() -> &'static [TypeInfoRow] { /// /// A failed parse falls back to a canonical name chosen here for `sql_type`, /// never to whichever [`trino_type_info`] row sorts first under that -/// `DATA_TYPE`. That row is `INTERVAL DAY TO SECOND`, an accident of the -/// table's required sort order, and it would misname every compound type: +/// `DATA_TYPE`. That is a positional pick, not a deliberate one (before core +/// ranked preferred rows it was `INTERVAL DAY TO SECOND`), and it would +/// misname every compound type: /// ARRAY, MAP, ROW, TUPLE, `ipaddress` and anything else with no dedicated /// `TrinoTypeName` variant. `trino_ty_to_sql_type` renders all of them as /// `EXT_W_VARCHAR` text, so `VARCHAR` is the honest name for that `DATA_TYPE`. @@ -1747,35 +1753,39 @@ mod tests { .unwrap_or_else(|| panic!("no SQLGetTypeInfo row for {type_name:?}")) } + /// Core orders the result set (DATA_TYPE, preferred row, TYPE_NAME), so the + /// declaration order here no longer matters. What does matter is that every + /// DATA_TYPE shared by several rows names its closest match: unmarked, the + /// alphabetically first row leads, and for SQL_WVARCHAR that was + /// `INTERVAL DAY TO SECOND`, which Power Query then used as the CAST + /// target for every varchar column. #[test] - fn type_info_rows_sorted_by_data_type_then_type_name() { - // Spec (SQLGetTypeInfo): "ordered by DATA_TYPE and then ... TYPE_NAME, - // both ascending." DATA_TYPE is a signed i16 (negative for ODBC - // extension types), so the comparison must not treat it as unsigned. - // This walks adjacent pairs rather than asserting a fixed sequence, - // so it keeps holding as rows are added or reordered. - for pair in trino_type_info().windows(2) { - let (prev, next) = (&pair[0], &pair[1]); - assert!( - prev.data_type().0 <= next.data_type().0, - "trino_type_info() not sorted by DATA_TYPE: {:?} (DATA_TYPE={}) \ - appears before {:?} (DATA_TYPE={})", - prev.type_name(), - prev.data_type().0, - next.type_name(), - next.data_type().0 - ); - if prev.data_type() == next.data_type() { - assert!( - prev.type_name() <= next.type_name(), - "rows sharing DATA_TYPE={} not sorted by TYPE_NAME: {:?} appears \ - before {:?}", - prev.data_type().0, - prev.type_name(), - next.type_name() - ); - } - } + fn every_shared_data_type_has_one_preferred_row() { + let issues = + stackable_odbc_core::conformance::type_info_preference_issues(trino_type_info()); + assert!( + issues.is_empty(), + "SQLGetTypeInfo preference markers: {issues:#?}" + ); + } + + /// The specific choices, not just their count: the plain type is the + /// closest match for each ODBC type it shares with Trino variants. + #[test] + fn preferred_rows_are_the_plain_types() { + let preferred: Vec<(i16, &str)> = trino_type_info() + .iter() + .filter(|r| r.preferred()) + .map(|r| (r.data_type().0, r.type_name())) + .collect(); + assert_eq!( + preferred, + vec![ + (SqlDataType::EXT_W_VARCHAR.0, TrinoTypeName::Varchar.name()), + (SqlDataType::TIME.0, TrinoTypeName::Time.name()), + (SqlDataType::TIMESTAMP.0, TrinoTypeName::Timestamp.name()), + ] + ); } /// `driver_version!()` must resolve `SQL_DRIVER_VER` from *this* crate's diff --git a/src/ffi_integration_tests.rs b/src/ffi_integration_tests.rs index 2b13484..3a65651 100644 --- a/src/ffi_integration_tests.rs +++ b/src/ffi_integration_tests.rs @@ -611,6 +611,59 @@ fn get_info_groups_that_constrain_each_other_agree() { } } +/// What Power Query sees: `SQLGetTypeInfo(DATA_TYPE)` through core's real +/// entry point, first row's TYPE_NAME. A shared DATA_TYPE must lead with the +/// plain type, never a variant such as `INTERVAL DAY TO SECOND` that merely +/// sorts first. Needs no server: `get_type_info` reads a static table. +#[test] +#[serial] +fn get_type_info_leads_each_shared_data_type_with_the_plain_type() { + unsafe { + let (env, conn) = alloc_conn_with_injected_trino_connection(); + for (data_type, expected) in [ + (SqlDataType::EXT_W_VARCHAR, "VARCHAR"), + (SqlDataType::TIME, "TIME"), + (SqlDataType::TIMESTAMP, "TIMESTAMP"), + ] { + let mut stmt: *mut c_void = std::ptr::null_mut(); + assert_eq!( + ffi::handle::sql_alloc_handle::( + HandleType::Stmt as i16, + conn, + &mut stmt + ), + SqlReturn::SUCCESS + ); + assert_eq!( + ffi::info::sql_get_type_info::(stmt, data_type.0), + SqlReturn::SUCCESS, + "SQLGetTypeInfo({data_type:?})" + ); + assert_eq!( + ffi::fetch::sql_fetch::(stmt), + SqlReturn::SUCCESS + ); + let mut buf = [0u16; 64]; + let mut ind: isize = 0; + assert_eq!( + ffi::fetch::sql_get_data::( + stmt, + 1, + CDataType::WChar as i16, + buf.as_mut_ptr().cast(), + std::mem::size_of_val(&buf) as isize, + &mut ind, + ), + SqlReturn::SUCCESS + ); + let name = String::from_utf16(&buf[..(ind as usize / 2)]).expect("UTF-16 TYPE_NAME"); + assert_eq!(name, expected, "first SQLGetTypeInfo row for {data_type:?}"); + let _ = ffi::handle::sql_free_handle::(HandleType::Stmt as i16, stmt); + } + cleanup_injected_conn(env, conn); + } +} + /// Property 2: no genuine `SQL_CONVERT_*` code ever returns 0 through /// `TrinoBackend`: per `AGENTS.md`, a `0` conversion bitmap is what makes /// the Windows Driver Manager block `SQLGetData` with `HYC00`. From c0f4def10be004146785d17901e9b1658ae1f71a Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 6 Oct 2026 17:44:46 +0200 Subject: [PATCH 02/20] chore: bump rustls to 0.23.45 for RUSTSEC-2026-0285 --- Cargo.lock | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9000c4a..6a0b10f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -83,9 +83,9 @@ checksum = "f2032f911046de80f0a198e0901378627c33f59ea0ac00e363d481118bd70a53" [[package]] name = "aws-lc-rs" -version = "1.17.3" +version = "1.18.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "00bdb5da18dac48ca2cc7cd4a98e533e8635a58e2361d13a1a4ee3888e0d72f1" +checksum = "b281d307588d634de920874890732659e2e7672f72b5e10e81badc1a8a83621e" dependencies = [ "aws-lc-sys", "zeroize", @@ -93,9 +93,9 @@ dependencies = [ [[package]] name = "aws-lc-sys" -version = "0.43.0" +version = "0.45.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "43103168cc76fe62678a375e722fc9cb3a0146159ac5828bc4f0dfd755c2224c" +checksum = "9bff6c3b54fad79a2e60b8102caf565819711497c1f5f092f49508e2f5c31b27" dependencies = [ "cc", "cmake", @@ -485,7 +485,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -1437,7 +1437,7 @@ dependencies = [ "once_cell", "socket2", "tracing", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -1659,14 +1659,14 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] name = "rustls" -version = "0.23.43" +version = "0.23.45" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0283386ce02abc0151e1761d08802dfe86c173b0b494af5cbc086574e453da06" +checksum = "0d41d731c7d2f962d1ccc364cec258de3c0e93b38c2fb3ba97ac74513048d634" dependencies = [ "aws-lc-rs", "once_cell", @@ -1716,7 +1716,7 @@ dependencies = [ "security-framework", "security-framework-sys", "webpki-root-certs", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -1727,9 +1727,9 @@ checksum = "f87165f0995f63a9fbeea62b64d10b4d9d8e78ec6d7d51fb2125fda7bb36788f" [[package]] name = "rustls-webpki" -version = "0.103.13" +version = "0.103.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "61c429a8649f110dddef65e2a5ad240f747e85f7758a6bccc7e5777bd33f756e" +checksum = "f3c3cf1d8b1e7d4927e2d154c3fcb02979afb9939629c62cd9048d4f07b60ac2" dependencies = [ "aws-lc-rs", "ring", @@ -2086,7 +2086,7 @@ dependencies = [ "getrandom 0.3.4", "once_cell", "rustix", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -2603,7 +2603,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] From dd31b43338fff33c0697557a30be1f88f18b1df4 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 7 Oct 2026 15:15:13 +0200 Subject: [PATCH 03/20] fix: quote and escape VARCHAR constants in the connector's Constant visitor Power Query hands text constants to the visitor unquoted (verified in Power BI Desktop), so `Cast(_, "VARCHAR")` folded a slicer into `CAST(hello world as VARCHAR)`. Quote the value and double single quotes. Adds a round-trip check to test_folding_contract.py and ordinary text rows (incl. O'Brien) to types_test. Together with the preferred VARCHAR type-info row this fixes DirectQuery slicers on text columns; the two changes must ship together. --- connector/StackableTrinoODBC.pq | 16 ++++-- integration-tests/stack/postgres/init.sql | 9 +++ .../suites/test_folding_contract.py | 56 +++++++++++++++++++ 3 files changed, 75 insertions(+), 6 deletions(-) diff --git a/connector/StackableTrinoODBC.pq b/connector/StackableTrinoODBC.pq index 191a999..9040f2c 100644 --- a/connector/StackableTrinoODBC.pq +++ b/connector/StackableTrinoODBC.pq @@ -394,12 +394,16 @@ StackableTrinoODBCImpl = ( // rendering it as Trino's `X'..'` literal // cannot be verified without Power BI Desktop. // + // Power Query hands a text-valued constant to the visitor + // bare, not quoted: confirmed in Power BI Desktop on + // 2026-10-07, where `Cast(_, "VARCHAR")` folded a slicer + // into `CAST(hello world as VARCHAR)`. A text entry therefore + // quotes the value itself and doubles any single quote in + // it, or `O'Brien` ends the literal early. + // // TODO: add UUID, JSON, TIME WITH TIME ZONE and TIMESTAMP - // WITH TIME ZONE. Each has a valid Trino cast, but the - // rendering turns on whether Power Query hands a - // text-valued constant to the visitor already quoted, which - // the VARCHAR entry below also rests on. Confirm that in - // Power BI Desktop, then add all four. + // WITH TIME ZONE, now that the quoting question above is + // settled. Each has a valid Trino cast. Visitor = [ DECIMAL = each Cast(_, "DECIMAL"), INTEGER = each Cast(_, "INTEGER"), @@ -410,7 +414,7 @@ StackableTrinoODBCImpl = ( DOUBLE = each Cast(_, "DOUBLE PRECISION"), BOOLEAN = each Cast(_, "BOOLEAN"), DATE = each Cast(Quote(Date.ToText(_, "yyyy-MM-dd")), "DATE"), - VARCHAR = each Cast(_, "VARCHAR"), + VARCHAR = each Cast(Quote(Text.Replace(_, "'", "''")), "VARCHAR"), // `fffffff`, not `sssssss`: in a custom format string // `s` is the second and `f` is the fractional second, // so the latter spelling renders the second eight times diff --git a/integration-tests/stack/postgres/init.sql b/integration-tests/stack/postgres/init.sql index 91cbb86..4dfc1d5 100644 --- a/integration-tests/stack/postgres/init.sql +++ b/integration-tests/stack/postgres/init.sql @@ -81,6 +81,15 @@ INSERT INTO types_test VALUES ( '{"unicode": "日本語"}' ); +-- Rows 5-8: ordinary text values, so a Power BI slicer on col_varchar offers +-- more than the edge cases above. `O'Brien` checks that the connector's +-- Constant visitor escapes a single quote in a folded filter literal. +INSERT INTO types_test (id, col_varchar, col_integer) VALUES + (5, 'apple', 10), + (6, 'banana', 20), + (7, 'cherry', 30), + (8, 'O''Brien', 40); + -- --------------------------------------------------------------------------- -- public schema: relational tables for PK/FK/index testing -- --------------------------------------------------------------------------- diff --git a/integration-tests/suites/test_folding_contract.py b/integration-tests/suites/test_folding_contract.py index 5fb8f5b..8fb101a 100644 --- a/integration-tests/suites/test_folding_contract.py +++ b/integration-tests/suites/test_folding_contract.py @@ -117,6 +117,39 @@ def parse_temporal_formats(source): ) +def parse_text_entries(source): + """Map each Constant-visitor key that casts to VARCHAR to its whole body. + + `VARCHAR = each Cast(Quote(Text.Replace(_, "'", "''")), "VARCHAR")` yields + `{"VARCHAR": 'Cast(Quote(Text.Replace(_, "\\'", "\\'\\'")), "VARCHAR")'}`. + """ + return dict( + re.findall(r'(\w+)\s*=\s*each\s+(Cast\(.*?"VARCHAR"\s*\))', source) + ) + + +def render_text_constant(body, value): + """The SQL a text visitor entry produces for `value`. + + Power Query hands the visitor the bare text, not a quoted literal: verified + in Power BI Desktop on 2026-10-07, where the entry `Cast(_, "VARCHAR")` + produced `CAST(hello world as VARCHAR)`. So the entry has to quote the value + itself, and double any single quote inside it, or a value such as `O'Brien` + ends the literal early. This mirrors exactly those two steps, read off the + entry's body, so the check below fails for an entry that skips either. + """ + quoted = "Quote(" in body + escaped = re.search(r"""Text\.Replace\(\s*_\s*,\s*"'"\s*,\s*"''"\s*\)""", body) + text = value.replace("'", "''") if escaped else value + return f"'{text}'" if quoted else text + + +# Slicer values a text constant must survive. The first three are what Power BI +# sent from `postgresql.public.types_test.col_varchar` on 2026-10-07 (the empty +# string included); the apostrophe is the one that needs escaping. +TEXT_SAMPLES = ["hello world", "日本語テスト 🎉🦀 café résumé", "", "O'Brien"] + + # The .NET custom date/time specifiers the connector is allowed to use, longest # first so `mm` is matched before `m` would be. Anything else is rejected rather # than guessed at: an unrecognised specifier is exactly the defect this looks @@ -296,6 +329,29 @@ def main(): f" {str(e)[:90]}", ) + # ------------------------------------------------------------------ + print("\n--- the text literals the Constant visitor renders round-trip ---") + # A slicer on a text column folds into `"col" = `, and the + # constant is whatever this entry renders. Unquoted, every value but NULL + # is a syntax error; quoted but unescaped, an apostrophe ends the literal. + # Round-tripped through Trino rather than pattern-matched, because what + # matters is the value Trino compares against. + text_entries = parse_text_entries(source) + check( + "the Constant visitor has a VARCHAR entry", + bool(text_entries), + "" if text_entries else " (none parsed)", + ) + for key in sorted(text_entries): + for value in TEXT_SAMPLES: + literal = render_text_constant(text_entries[key], value) + label = f"{key} renders {value!r} as {literal!r}, which CASTs back to it" + try: + got = cur.execute(f"SELECT CAST({literal} AS VARCHAR)").fetchone()[0] + check(label, got == value, "" if got == value else f" (got {got!r})") + except pyodbc.Error as e: + check(label, False, f" {str(e)[:90]}") + # ------------------------------------------------------------------ print("\n--- the row-limiting clause the AstVisitor builds parses ---") rendered = parse_limit_clause(source) From 494c1cd710a7bc9cbc984c04144f5d8354f7b0e9 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 7 Oct 2026 15:47:16 +0200 Subject: [PATCH 04/20] test: seed a hive view with INTERVAL and date/time columns for Power BI tests No table in the stack has an INTERVAL column: a Hive table cannot store the type and the postgresql catalog does not expose Postgres intervals. seed-hive.sh now also creates hive.tx.interval_test, a view over postgresql.public.types_test with an INTERVAL DAY TO SECOND and an INTERVAL YEAR TO MONTH column, alongside the table's text, integer, date, time(6), timestamp(6) and timestamp(6) with time zone columns, so Power BI slicers on each type can be tested from one view. --- integration-tests/scripts/seed-hive.sh | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/integration-tests/scripts/seed-hive.sh b/integration-tests/scripts/seed-hive.sh index 9473ede..7327b31 100755 --- a/integration-tests/scripts/seed-hive.sh +++ b/integration-tests/scripts/seed-hive.sh @@ -6,7 +6,7 @@ # suite connecting normally gets `Access Denied: Cannot create schema`. Once the # schema exists and admin owns it, everything else an ordinary connection needs # works without a role: creating tables, writing, reading and granting. So this -# seeds exactly the schema and stops. +# seeds the schema, plus one view no other catalog can provide (below). # # Idempotent, and run on every setup: the file metastore lives in the # coordinator's writable layer, so recreating the container starts it empty. @@ -49,4 +49,19 @@ trino_run() { } trino_run "CREATE SCHEMA IF NOT EXISTS hive.$HIVE_SCHEMA" admin -echo "Seeded hive.$HIVE_SCHEMA" + +# INTERVAL columns, which no table in the stack has: a Hive table cannot store +# the type, and the postgresql catalog does not expose Postgres intervals. A +# view computes them over postgresql.public.types_test, so a Power BI slicer on +# an interval column can be tested next to the text and date/time columns that +# table already has. +trino_run "CREATE OR REPLACE VIEW hive.$HIVE_SCHEMA.interval_test AS +SELECT id, col_varchar, col_integer, + col_date, col_time, col_timestamp, col_timestamptz, + CASE id WHEN 5 THEN INTERVAL '1' DAY + WHEN 6 THEN INTERVAL '2' HOUR + WHEN 7 THEN INTERVAL '1' DAY + INTERVAL '30' MINUTE END AS col_interval_ds, + CASE id WHEN 5 THEN INTERVAL '1' YEAR + WHEN 6 THEN INTERVAL '3' MONTH END AS col_interval_ym +FROM postgresql.public.types_test" admin +echo "Seeded hive.$HIVE_SCHEMA and hive.$HIVE_SCHEMA.interval_test" From c8a84a2723a4752bf64cf18a66af55171b007b6d Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 7 Oct 2026 17:03:46 +0200 Subject: [PATCH 05/20] test: add a Power BI slicer-semantics acceptance suite (currently failing) Power BI folds a slicer selection into a filter built from the value it showed. For interval, time and timestamp-with-time-zone columns that filter silently selects no rows. test_pbi_slicer_semantics.py replays the filters Power BI Desktop sent on 2026-10-07 through the driver and checks the row counts, in the default session time zone and in Europe/Berlin. It fails on purpose: it is the acceptance test for the fixes that follow (intervals, time, timestamptz). seed-hive.sh adds negative and fractional interval values to hive.tx.interval_test for it. --- integration-tests/scripts/seed-hive.sh | 9 +- integration-tests/suites/registry.py | 1 + .../suites/test_pbi_slicer_semantics.py | 112 ++++++++++++++++++ 3 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 integration-tests/suites/test_pbi_slicer_semantics.py diff --git a/integration-tests/scripts/seed-hive.sh b/integration-tests/scripts/seed-hive.sh index 7327b31..20cb19a 100755 --- a/integration-tests/scripts/seed-hive.sh +++ b/integration-tests/scripts/seed-hive.sh @@ -58,10 +58,13 @@ trino_run "CREATE SCHEMA IF NOT EXISTS hive.$HIVE_SCHEMA" admin trino_run "CREATE OR REPLACE VIEW hive.$HIVE_SCHEMA.interval_test AS SELECT id, col_varchar, col_integer, col_date, col_time, col_timestamp, col_timestamptz, - CASE id WHEN 5 THEN INTERVAL '1' DAY + CASE id WHEN 1 THEN INTERVAL '0.5' SECOND + WHEN 5 THEN INTERVAL '1' DAY WHEN 6 THEN INTERVAL '2' HOUR - WHEN 7 THEN INTERVAL '1' DAY + INTERVAL '30' MINUTE END AS col_interval_ds, + WHEN 7 THEN INTERVAL '1' DAY + INTERVAL '30' MINUTE + WHEN 8 THEN INTERVAL '-1' DAY END AS col_interval_ds, CASE id WHEN 5 THEN INTERVAL '1' YEAR - WHEN 6 THEN INTERVAL '3' MONTH END AS col_interval_ym + WHEN 6 THEN INTERVAL '3' MONTH + WHEN 8 THEN INTERVAL '-1' YEAR END AS col_interval_ym FROM postgresql.public.types_test" admin echo "Seeded hive.$HIVE_SCHEMA and hive.$HIVE_SCHEMA.interval_test" diff --git a/integration-tests/suites/registry.py b/integration-tests/suites/registry.py index 005664a..ce38129 100644 --- a/integration-tests/suites/registry.py +++ b/integration-tests/suites/registry.py @@ -109,6 +109,7 @@ def windows_skip_reason(self): # the connector travels with it. deploy=("connector/StackableTrinoODBC.pq",), ), + Suite("pbi slicer semantics", "test_pbi_slicer_semantics.py"), Suite( "tls", "test_tls.py", argv="none", # keycloak.crt is a leaf signed by the same CA, used as a trust anchor diff --git a/integration-tests/suites/test_pbi_slicer_semantics.py b/integration-tests/suites/test_pbi_slicer_semantics.py new file mode 100644 index 0000000..c8c0d4a --- /dev/null +++ b/integration-tests/suites/test_pbi_slicer_semantics.py @@ -0,0 +1,112 @@ +#!/usr/bin/env python3 +""" +Power BI slicer semantics: does the filter Power BI folds select the value it showed? + +A Power BI DirectQuery slicer shows each value the driver delivers, and when one is +picked it folds a filter built from that shown value. The filter only works if Trino, +evaluating it, arrives back at the same value. Whether it does depends on the +column's type, and for several types it silently does not: the filter runs, finds no +rows, and the report shows nothing, with no error anywhere. + +This reproduces Power BI's filters through the driver and checks the row count. The +templates below are copied verbatim from the SQL Power BI Desktop sent to Trino on +2026-10-07 (DirectQuery, the Stackable connector, `hive.tx.interval_test`); the +expected count is computed here from the fetched rows, so no comparison inside Trino +is trusted to define the answer. + +Everything runs twice, in the server's default session time zone and in +`Europe/Berlin`, because a timestamp-with-time-zone filter depends on it. + +Usage: + uv run --with pyodbc python3 integration-tests/suites/test_pbi_slicer_semantics.py "" + +Requires a running Trino (integration-tests/setup.sh), whose seed-hive.sh creates the +`hive.tx.interval_test` view this reads. Needs no compose profile. +""" + +import os +import sys + +import pyodbc + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +from harness import Results, Stack # noqa: E402 + +R = Results("pbi slicer semantics") + +VIEW = "hive.tx.interval_test" + + +def text(v): + return v.replace("'", "''") + + +def fraction7(v): + """Power BI renders seconds with seven fractional digits (100 ns ticks).""" + return f"{v.microsecond:06d}0" + + +# column -> the WHERE clause Power BI folded for a slicer value `v`, as captured. +TEMPLATES = { + "col_varchar": lambda v: f"\"col_varchar\" = CAST('{text(v)}' as VARCHAR)", + "col_date": lambda v: f"\"col_date\" = CAST('{v:%Y-%m-%d}' as DATE)", + "col_timestamp": lambda v: ( + f"\"col_timestamp\" = CAST('{v:%Y-%m-%d %H:%M:%S}.{fraction7(v)}' as TIMESTAMP)" + ), + "col_interval_ds": lambda v: ( + f"cast(\"col_interval_ds\" as VARCHAR) = CAST('{text(v)}' as VARCHAR)" + ), + "col_interval_ym": lambda v: ( + f"cast(\"col_interval_ym\" as VARCHAR) = CAST('{text(v)}' as VARCHAR)" + ), + # Power BI anchors a time of day on its base date, 1899-12-30. + "col_time": lambda v: ( + f"cast(\"col_time\" as TIMESTAMP) = " + f"CAST('1899-12-30 {v:%H:%M:%S}.{fraction7(v)}' as TIMESTAMP)" + ), + "col_timestamptz": lambda v: ( + f"\"col_timestamptz\" = CAST('{v:%Y-%m-%d %H:%M:%S}.{fraction7(v)}' as TIMESTAMP)" + ), +} + + +def check_zone(conn_str, zone_label): + print(f"\n--- session time zone: {zone_label} ---") + conn = pyodbc.connect(conn_str, autocommit=True) + cur = conn.cursor() + for col, template in TEMPLATES.items(): + pairs = cur.execute(f"SELECT id, {col} FROM {VIEW}").fetchall() + values = [] + for _, x in pairs: + if x is not None and x not in values: + values.append(x) + for v in values: + expected = sum(1 for _, x in pairs if x == v) + where = template(v) + label = f"[{zone_label}] {col} = {v!r} selects its rows" + try: + got = cur.execute(f"SELECT count(*) FROM {VIEW} WHERE {where}").fetchone()[0] + R.check(label, got == expected, "" if got == expected + else f"got {got}, expected {expected} (WHERE {where})") + except pyodbc.Error as e: + R.check(label, False, f"{str(e)[:120]} (WHERE {where})") + # A slicer's "(Blank)" folds to `is null`; that already works and must keep working. + nulls = sum(1 for _, x in pairs if x is None) + got = cur.execute(f"SELECT count(*) FROM {VIEW} WHERE \"{col}\" is null").fetchone()[0] + R.check(f"[{zone_label}] {col} (Blank) selects its rows", got == nulls, + "" if got == nulls else f"got {got}, expected {nulls}") + cur.close() + conn.close() + + +def main(): + conn_str = sys.argv[1] if len(sys.argv) > 1 else Stack.load().conn_str() + print(f"=== pbi slicer semantics ===\nview: {VIEW}") + check_zone(conn_str, "server default") + check_zone(conn_str + ";TimeZone=Europe/Berlin", "Europe/Berlin") + return R.summary() + + +if __name__ == "__main__": + sys.exit(main()) From 4bde7ee45d9360bb5f4f3bc63a2fd109cf086962 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 7 Oct 2026 17:42:46 +0200 Subject: [PATCH 06/20] fix: deliver interval columns as Trino's own text A Power BI DirectQuery slicer on an interval column folds to cast(col as VARCHAR) = '', so the value shown has to be exactly what Trino's CAST renders. The driver parsed interval text into fields and core re-rendered them, which gave -1-00 for Trino's -1-0 and 0 00:00:00.5 for 0 00:00:00.500: the filter ran, matched nothing, and the report showed no rows without an error. Interval columns are now delivered as the text Trino sent. Reading them as SQL_C_INTERVAL_* still works, because stackable-odbc-core now converts character data to the interval C types as the SQL to C: Character table requires; core is pinned to that commit (b37ca99) until a release is tagged. The two interval parsers, which nothing else used, are removed. Tests: unit tests expect the text verbatim; the FFI tests compare each slicer view value against Trino's own CAST AS VARCHAR and read both interval families back as SQL_INTERVAL_STRUCT; the type matrix checks the interval text. The slicer-semantics suite's interval checks pass; its time and timestamp-with-time-zone checks still fail, as intended until those fixes land. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 10 + Cargo.lock | 2 +- Cargo.toml | 9 +- fuzz/README.md | 4 +- fuzz/fuzz_targets/json_value.rs | 4 +- integration-tests/suites/test_type_matrix.py | 5 +- src/ffi_integration_tests.rs | 120 +++++++++- src/type_conversion.rs | 222 ++++--------------- 8 files changed, 174 insertions(+), 202 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f4aaf2..50e580a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `INTERVAL YEAR TO MONTH` and `INTERVAL DAY TO SECOND` columns read as text now + return Trino's own rendering, the same text `CAST(... AS VARCHAR)` produces + (`-1-0`, `0 00:00:00.500`). They were parsed into fields and re-rendered + (`-1-00`, `0 00:00:00.5`), so a Power BI DirectQuery slicer on an interval + column, which folds to `cast(col as VARCHAR) = ''`, silently + selected no rows. Reading these columns as `SQL_C_INTERVAL_*` still works: + stackable-odbc-core now converts interval text to those C types. + ## [0.1.2] — 2026-09-01 ### Changed diff --git a/Cargo.lock b/Cargo.lock index 6a0b10f..60edd05 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1971,7 +1971,7 @@ checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" [[package]] name = "stackable-odbc-core" version = "0.1.0" -source = "git+https://github.com/stackabletech/stackable-odbc-core.git?rev=23f055e451cde6b842ad117f5c8a16957abe113f#23f055e451cde6b842ad117f5c8a16957abe113f" +source = "git+https://github.com/stackabletech/stackable-odbc-core.git?rev=b37ca99433b15b2646e98594a6571719acc2a8fa#b37ca99433b15b2646e98594a6571719acc2a8fa" dependencies = [ "odbc-sys", "snafu", diff --git a/Cargo.toml b/Cargo.toml index 7ec1fca..8cee15a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,10 +38,11 @@ snafu = "0.9" # reviewable edit to this line instead of whatever the branch happens to point # at. To build against a local checkout, use a `[patch]` in your own # `.cargo/config.toml` rather than editing this line; see CONTRIBUTING.md. -# TEMPORARY: pinned to the core commit adding `TypeInfoRow::with_preferred` -# until a core release containing it is tagged; switch both entries back to +# TEMPORARY: pinned to the core commit converting character data to the +# SQL_C_INTERVAL_* C types (after `TypeInfoRow::with_preferred`) until a core +# release containing both is tagged; switch both entries back to # that tag before merging. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "23f055e451cde6b842ad117f5c8a16957abe113f" } +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "b37ca99433b15b2646e98594a6571719acc2a8fa" } # `time` is needed directly by `query_all_rows_within`, which bounds the login # round trip with `tokio::time::timeout`. It resolves without being declared, # because reqwest enables it, but a direct use must not rely on another crate's @@ -63,7 +64,7 @@ serial_test = "4" # offline FFI tests need. It is default-off because it is test code, so it is # enabled here rather than on the [dependencies] entry above -- that keeps it # out of the shipped cdylib. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "23f055e451cde6b842ad117f5c8a16957abe113f", features = ["test-support"] } +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "b37ca99433b15b2646e98594a6571719acc2a8fa", features = ["test-support"] } [lints.clippy] unwrap_in_result = "deny" diff --git a/fuzz/README.md b/fuzz/README.md index edca93e..4347f73 100644 --- a/fuzz/README.md +++ b/fuzz/README.md @@ -20,8 +20,8 @@ one: a value a coordinator legitimately sent must fail safe, and where a release build has no overflow checks the same defect returns a wrong answer instead of an error. -- `json_value` covers `json_to_column_value` and the dozen temporal, interval - and decimal scanners under it. This is the half of the read path core does +- `json_value` covers `json_to_column_value` and the temporal and decimal + scanners under it. This is the half of the read path core does not see: core fuzzes `write_column_value`, which turns the resulting `ColumnValue` into the caller's buffer, and nothing covered the step that produces it. diff --git a/fuzz/fuzz_targets/json_value.rs b/fuzz/fuzz_targets/json_value.rs index 00c126e..b5c45be 100644 --- a/fuzz/fuzz_targets/json_value.rs +++ b/fuzz/fuzz_targets/json_value.rs @@ -11,8 +11,8 @@ use trino_rust_client::{TrinoFloat, TrinoInt, TrinoTy}; // // stackable-odbc-core already fuzzes the second half (`write_column_value`, // ColumnValue -> the caller's buffer). Nothing covered the step before it, -// which is where this crate's temporal, interval and decimal parsers live: -// roughly a dozen hand-written scanners over text a Trino coordinator chose. +// which is where this crate's temporal and decimal parsers live: hand-written +// scanners over text a Trino coordinator chose. // Every one of them runs on the server's side of the trust boundary. // // The property is that no input panics. A panic here is caught at the FFI diff --git a/integration-tests/suites/test_type_matrix.py b/integration-tests/suites/test_type_matrix.py index e4c3883..c949860 100755 --- a/integration-tests/suites/test_type_matrix.py +++ b/integration-tests/suites/test_type_matrix.py @@ -134,8 +134,9 @@ ("timestamp tz", "CAST('2020-02-03 04:05:06 UTC' AS TIMESTAMP WITH TIME ZONE)", None), ("uuid", "CAST('12151fd2-7586-11e9-8f9e-2a86e4085a59' AS UUID)", None), ("json", "CAST('{\"a\":1}' AS JSON)", None), - ("interval day", "INTERVAL '2' DAY", None), - ("interval year", "INTERVAL '2' YEAR", None), + # Trino's own text, which is what Power BI compares a slicer value against. + ("interval day", "INTERVAL '2' DAY", "2 00:00:00.000"), + ("interval year", "INTERVAL '2' YEAR", "2-0"), ("array", "ARRAY[1,2,3]", None), ("row", "CAST(ROW(1,'a') AS ROW(x INTEGER, y VARCHAR))", None), ] diff --git a/src/ffi_integration_tests.rs b/src/ffi_integration_tests.rs index 3a65651..6c98c67 100644 --- a/src/ffi_integration_tests.rs +++ b/src/ffi_integration_tests.rs @@ -33,8 +33,8 @@ use stackable_odbc_core::odbc_sys; use stackable_odbc_core::test_support::{attach_connection, detach_connection}; use stackable_odbc_core::types::{ AttrOdbcVersion, CDataType, Desc, EnvironmentAttribute, HandleType, HeaderDiagnosticIdentifier, - InfoType, ParamType, SQL_AUTOCOMMIT_OFF, SQL_AUTOCOMMIT_ON, SQL_FETCH_BOOKMARK, SQL_IC_LOWER, - SQL_INDEX_UNIQUE, SQL_LOCK_NO_CHANGE, SQL_NTS, SQL_NULL_DATA, SQL_PARAM_ERROR, + InfoType, Interval, ParamType, SQL_AUTOCOMMIT_OFF, SQL_AUTOCOMMIT_ON, SQL_FETCH_BOOKMARK, + SQL_IC_LOWER, SQL_INDEX_UNIQUE, SQL_LOCK_NO_CHANGE, SQL_NTS, SQL_NULL_DATA, SQL_PARAM_ERROR, SQL_PARAM_SUCCESS, SQL_POSITION, SQL_QUICK, SqlDataType, SqlReturn, StatementAttribute, expected_kind, }; @@ -1658,9 +1658,10 @@ fn interval_year_month_returns_wchar() { ffi::fetch::sql_fetch::(stmt), SqlReturn::SUCCESS ); - let s = fetch_wchar(stmt); - assert!(s.contains('3'), "expected '3' in {s:?}"); - assert!(s.contains('7'), "expected '7' in {s:?}"); + // Trino's own rendering, unchanged: Power BI folds a slicer on this + // column to `cast(col as VARCHAR) = ''`, which only matches if + // the shown text is what that CAST produces. + assert_eq!(fetch_wchar(stmt), "3-7"); cleanup_stmt(stmt); } } @@ -1679,8 +1680,113 @@ fn interval_day_time_returns_wchar() { ffi::fetch::sql_fetch::(stmt), SqlReturn::SUCCESS ); - let s = fetch_wchar(stmt); - assert!(s.contains('2'), "expected '2' in {s:?}"); + assert_eq!(fetch_wchar(stmt), "2 03:04:05.678"); + cleanup_stmt(stmt); + } +} + +/// The values the Power BI slicer view (`seed-hive.sh`) holds, whose re-rendered +/// text (`-1-00`, `0 00:00:00.5`) differed from Trino's and so selected no rows. +#[test] +#[serial] +#[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] +fn intervals_are_delivered_as_trinos_cast_to_varchar_text() { + for expr in [ + "INTERVAL '-1' YEAR", + "INTERVAL '3' MONTH", + "INTERVAL '-1' DAY", + "INTERVAL '0.5' SECOND", + "INTERVAL '1' DAY + INTERVAL '30' MINUTE", + ] { + unsafe { + let (_env, _conn, stmt) = alloc_stmt(); + assert_eq!( + exec_direct(stmt, &format!("SELECT {expr}, CAST({expr} AS VARCHAR)")), + SqlReturn::SUCCESS + ); + assert_eq!( + ffi::fetch::sql_fetch::(stmt), + SqlReturn::SUCCESS + ); + let cast = get_wchar_col(stmt, 2); + assert_eq!(get_wchar_col(stmt, 1), cast, "{expr}"); + cleanup_stmt(stmt); + } + } +} + +/// Read column 1 into a `SQL_INTERVAL_STRUCT` as `target`. +unsafe fn fetch_interval(stmt: *mut c_void, target: CDataType) -> odbc_sys::IntervalStruct { + let mut out = odbc_sys::IntervalStruct { + interval_type: 0, + interval_sign: 0, + interval_value: odbc_sys::IntervalUnion { + day_second: odbc_sys::DaySecond::default(), + }, + }; + let mut ind: isize = 0; + let ret = unsafe { + ffi::fetch::sql_get_data::( + stmt, + 1, + target as i16, + (&raw mut out).cast(), + size_of::() as isize, + &mut ind, + ) + }; + assert_eq!(ret, SqlReturn::SUCCESS, "sql_get_data as {target:?} failed"); + out +} + +/// Delivering interval columns as text must not cost an application the +/// `SQL_C_INTERVAL_*` C types: core converts the text, per the spec's +/// "SQL to C: Character" table. +#[test] +#[serial] +#[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] +fn interval_year_month_reads_as_sql_c_interval_year_to_month() { + unsafe { + let (_env, _conn, stmt) = alloc_stmt(); + assert_eq!( + exec_direct(stmt, "SELECT INTERVAL '-3-7' YEAR TO MONTH"), + SqlReturn::SUCCESS + ); + assert_eq!( + ffi::fetch::sql_fetch::(stmt), + SqlReturn::SUCCESS + ); + let got = fetch_interval(stmt, CDataType::IntervalYearToMonth); + assert_eq!(got.interval_type, Interval::YearToMonth as i32); + assert_eq!(got.interval_sign, 1, "SQL_TRUE: negative"); + let ym = got.interval_value.year_month; + assert_eq!((ym.year, ym.month), (3, 7)); + cleanup_stmt(stmt); + } +} + +#[test] +#[serial] +#[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] +fn interval_day_time_reads_as_sql_c_interval_day_to_second() { + unsafe { + let (_env, _conn, stmt) = alloc_stmt(); + assert_eq!( + exec_direct(stmt, "SELECT INTERVAL '2 03:04:05' DAY TO SECOND"), + SqlReturn::SUCCESS + ); + assert_eq!( + ffi::fetch::sql_fetch::(stmt), + SqlReturn::SUCCESS + ); + let got = fetch_interval(stmt, CDataType::IntervalDayToSecond); + assert_eq!(got.interval_type, Interval::DayToSecond as i32); + assert_eq!(got.interval_sign, 0); + let ds = got.interval_value.day_second; + assert_eq!( + (ds.day, ds.hour, ds.minute, ds.second, ds.fraction), + (2, 3, 4, 5, 0) + ); cleanup_stmt(stmt); } } diff --git a/src/type_conversion.rs b/src/type_conversion.rs index 758491f..ec5b6e8 100644 --- a/src/type_conversion.rs +++ b/src/type_conversion.rs @@ -5,10 +5,7 @@ use chrono::Datelike as _; use chrono::Timelike as _; use serde_json::Value; -use stackable_odbc_core::types::{ - ColumnValue, Interval, NANOS_PER_DAY, NANOS_PER_HOUR, NANOS_PER_MINUTE, NANOS_PER_SECOND, - PRECISION_UNDETERMINABLE, SqlDataType, column_size, -}; +use stackable_odbc_core::types::{ColumnValue, PRECISION_UNDETERMINABLE, SqlDataType, column_size}; use trino_rust_client::{TrinoFloat, TrinoInt, TrinoTy}; /// This driver's declared maximum fractional-seconds precision for TIME, @@ -925,88 +922,6 @@ fn parse_trino_timestamp(s: &str) -> Option { }) } -/// Parse a Trino INTERVAL YEAR TO MONTH string "Y-M" into years and months. -/// -/// The sign prefixes the whole interval, not just the year component: Trino -/// serialises a negative interval as `"-Y-M"`. Parsing the leading `-` once -/// (rather than relying on `i32::parse` to see it on the first token) and -/// applying it to both fields keeps the two in agreement: a split -/// representation must not let one field be negative while the other is -/// positive. -fn parse_interval_year_month(s: &str) -> Option { - let t = s.trim(); - let (negative, body) = match t.strip_prefix('-') { - Some(rest) => (true, rest), - None => (false, t), - }; - - let mut parts = body.splitn(2, '-'); - let years: i32 = parts.next()?.trim().parse().ok()?; - let months: i32 = parts.next()?.trim().parse().ok()?; - - let (years, months) = if negative { - (years.checked_neg()?, months.checked_neg()?) - } else { - (years, months) - }; - - Some(ColumnValue::IntervalYearMonth { - years, - months, - // Trino has one year-month interval type and it carries both fields, so - // the precision is always the two-field form. The narrower - // `Interval::Year` and `Interval::Month` have no Trino column type to - // come from. - precision: Interval::YearToMonth, - }) -} - -/// Parse Trino's `INTERVAL DAY TO SECOND` text, e.g. `"-2 03:04:05.678"`. -/// -/// The sign prefixes the whole interval, not just the day component. -fn parse_interval_day_time(s: &str) -> Option { - let t = s.trim(); - let (negative, body) = match t.strip_prefix('-') { - Some(rest) => (true, rest), - None => (false, t), - }; - - let mut outer = body.splitn(2, ' '); - let days: i64 = outer.next()?.trim().parse().ok()?; - let time_part = outer.next()?.trim(); - - let mut tp = time_part.splitn(3, ':'); - let h: i64 = tp.next()?.parse().ok()?; - let m: i64 = tp.next()?.parse().ok()?; - let sec_part = tp.next()?; - let (sec_text, frac_text) = match sec_part.split_once('.') { - Some((sec, frac)) => (sec, frac), - None => (sec_part, ""), - }; - let sec: i64 = sec_text.parse().ok()?; - // `ColumnValue::IntervalDayTime` counts nanoseconds, so the fraction is kept - // whole. Trino renders this type with three fractional digits, its own - // storage being a millisecond count, but `parse_fraction_nanos` reads - // whatever arrives on the same "pad right, then take nine" rule the temporal - // parsers use, so a shorter fragment like "5" is read as 500ms rather than - // 5ns and a longer one does not have to be special-cased here. - let frac_nanos = i128::from(parse_fraction_nanos(frac_text)); - - let magnitude = i128::from(days) - .checked_mul(NANOS_PER_DAY)? - .checked_add(i128::from(h) * NANOS_PER_HOUR)? - .checked_add(i128::from(m) * NANOS_PER_MINUTE)? - .checked_add(i128::from(sec) * NANOS_PER_SECOND)? - .checked_add(frac_nanos)?; - - Some(ColumnValue::IntervalDayTime { - total_nanoseconds: if negative { -magnitude } else { magnitude }, - // Trino has one day-time interval type and it spans all four fields, so - // the precision is always the widest form. - precision: Interval::DayToSecond, - }) -} - /// Parse a Trino TIMESTAMP WITH TIME ZONE string and convert to UTC. /// /// Trino REST API format: `"YYYY-MM-DD HH:MM:SS.fraction TIMEZONE"` where @@ -1272,26 +1187,17 @@ pub fn json_to_column_value(val: Value, ty: &TrinoTy) -> ColumnValue { Value::String(s) => ColumnValue::Json(s), other => ColumnValue::Json(other.to_string()), }, - TrinoTy::IntervalYearToMonth => { - if let Value::String(ref s) = val { - parse_interval_year_month(s).unwrap_or_else(|| { - tracing::warn!(raw = s, "failed to parse Trino INTERVAL YEAR TO MONTH"); - ColumnValue::String(s.clone()) - }) - } else { - ColumnValue::String(val.to_string()) - } - } - TrinoTy::IntervalDayToSecond => { - if let Value::String(ref s) = val { - parse_interval_day_time(s).unwrap_or_else(|| { - tracing::warn!(raw = s, "failed to parse Trino INTERVAL DAY TO SECOND"); - ColumnValue::String(s.clone()) - }) - } else { - ColumnValue::String(val.to_string()) - } - } + // Intervals are delivered as Trino's own text, not parsed into fields. + // Power BI folds a slicer on an interval column to + // `cast(col as VARCHAR) = ''`, so the shown text must be + // exactly what Trino's CAST renders (`-1-0`, `0 00:00:00.500`); core's + // rendering of structured fields (`-1-00`, `0 00:00:00.5`) matched no + // row. Core still converts this text to the SQL_C_INTERVAL_* types, per + // the spec's "SQL to C: Character" table. + TrinoTy::IntervalYearToMonth | TrinoTy::IntervalDayToSecond => match val { + Value::String(s) => ColumnValue::String(s), + other => ColumnValue::String(other.to_string()), + }, // VARBINARY arrives as a base64-encoded string in the REST API payload. TrinoTy::VarBinary => { if let Value::String(ref s) = val { @@ -2494,89 +2400,37 @@ mod tests { assert_eq!(val, ColumnValue::Json(r#"{"a":1}"#.to_string())); } - #[test] - fn interval_year_month_parses_correctly() { + /// An interval column is delivered as Trino's own text, unchanged. + /// + /// Power BI folds a slicer on an interval column to + /// `cast(col as VARCHAR) = ''`, so the text an application is + /// shown must be exactly what Trino's `CAST(... AS VARCHAR)` produces, or + /// the filter silently selects nothing. Trino renders `INTERVAL '-1' YEAR` + /// as `-1-0`; a parse into structured fields re-rendered by core gave + /// `-1-00`. + #[test] + fn interval_year_month_is_delivered_as_trinos_text() { use serde_json::json; - let val = json_to_column_value(json!("3-7"), &TrinoTy::IntervalYearToMonth); - assert_eq!( - val, - ColumnValue::IntervalYearMonth { - years: 3, - months: 7, - precision: Interval::YearToMonth, - } - ); + for raw in ["3-7", "-1-0", "0-3"] { + let val = json_to_column_value(json!(raw), &TrinoTy::IntervalYearToMonth); + assert_eq!(val, ColumnValue::String(raw.to_string()), "{raw}"); + } } + /// The day-time counterpart: `INTERVAL '0.5' SECOND` is `0 00:00:00.500` + /// in Trino, which core's re-rendering turned into `0 00:00:00.5`. #[test] - fn interval_day_time_parses_correctly() { + fn interval_day_time_is_delivered_as_trinos_text() { use serde_json::json; - let val = json_to_column_value(json!("2 03:04:05.678"), &TrinoTy::IntervalDayToSecond); - assert_eq!( - val, - ColumnValue::IntervalDayTime { - total_nanoseconds: 2 * NANOS_PER_DAY - + 3 * NANOS_PER_HOUR - + 4 * NANOS_PER_MINUTE - + 5 * NANOS_PER_SECOND - + 678_000_000, - precision: Interval::DayToSecond, - } - ); - } - - #[test] - fn negative_interval_day_time_is_fully_negative() { - let val = parse_interval_day_time("-2 03:04:05.678").expect("parses"); - // -(2 days + 3h4m5.678s) = -183_845_678 ms in nanoseconds. - assert_eq!( - val, - ColumnValue::IntervalDayTime { - total_nanoseconds: -183_845_678_000_000, - precision: Interval::DayToSecond, - } - ); - } - - #[test] - fn negative_zero_day_interval_keeps_its_sign() { - // "-0 03:04:05" must keep its sign: parsing the sign only off `days` - // loses it entirely, because "-0".parse::() is 0. - let val = parse_interval_day_time("-0 03:04:05").expect("parses"); - assert_eq!( - val, - ColumnValue::IntervalDayTime { - total_nanoseconds: -11_045_000_000_000, - precision: Interval::DayToSecond, - } - ); - } - - #[test] - fn positive_interval_day_time_is_unchanged() { - let val = parse_interval_day_time("2 03:04:05.678").expect("parses"); - assert_eq!( - val, - ColumnValue::IntervalDayTime { - total_nanoseconds: 183_845_678_000_000, - precision: Interval::DayToSecond, - } - ); - } - - /// A fraction finer than Trino's own millisecond rendering survives now that - /// the variant counts nanoseconds: the parser no longer truncates at three - /// digits. - #[test] - fn interval_day_time_keeps_sub_millisecond_digits() { - let val = parse_interval_day_time("0 00:00:01.234567").expect("parses"); - assert_eq!( - val, - ColumnValue::IntervalDayTime { - total_nanoseconds: NANOS_PER_SECOND + 234_567_000, - precision: Interval::DayToSecond, - } - ); + for raw in [ + "2 03:04:05.678", + "-1 00:00:00.000", + "-0 03:04:05.000", + "0 00:00:00.500", + ] { + let val = json_to_column_value(json!(raw), &TrinoTy::IntervalDayToSecond); + assert_eq!(val, ColumnValue::String(raw.to_string()), "{raw}"); + } } /// Numeric offset +05:30: 10:30 local = 05:00 UTC (subtract 5h30m). From 099590e308bd61856d4dbe96bac5deb08dc5466e Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 7 Oct 2026 18:29:14 +0200 Subject: [PATCH 07/20] fix: stop Power BI folding time slicers that can never match A DirectQuery slicer on a time column folds to cast("col" as TIMESTAMP) = CAST('1899-12-30 hh:mm:ss' as TIMESTAMP), anchoring the time on Power BI's base date. Trino, like ODBC's own conversion tables (SQL to C: Time, footnote [c]), puts a cast time on the current date, so the filter ran and silently selected no rows. The connector now reports SQL_CONVERT_TIME as the driver's answer minus SQL_CVT_TIMESTAMP. Power BI then refuses the fold with "We couldn't fold the expression to the data source" instead of showing an empty report, which was measured in Power BI Desktop, as was "(Blank)" still folding. It is a deliberate misreport, and the connector comment says so. Two alternatives were tried in Power BI Desktop and rejected: presenting time columns as WVARCHAR through SQLColumns stopped the table folding at all, and Trino rejects time = varchar anyway; changing the column to Text in Power Query is refused in DirectQuery. test_folding_contract.py pins the override to the driver's answer minus that one bit and checks that Trino still anchors on the current date. The slicer suite no longer checks time values, which Power BI no longer sends. The README documents the limitation and two workarounds, a view casting the column to VARCHAR and Import mode, both checked in Power BI Desktop, along with Import mode's blank-equals-midnight behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 9 +++++ README.md | 13 +++++++ connector/StackableTrinoODBC.pq | 25 +++++++++--- .../suites/test_folding_contract.py | 39 +++++++++++++++++++ .../suites/test_pbi_slicer_semantics.py | 15 ++++--- 5 files changed, 91 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50e580a..1b7c37d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- The Power BI connector reports that Trino cannot convert `TIME` to + `TIMESTAMP` (`SQL_CONVERT_TIME` without `SQL_CVT_TIMESTAMP`). A DirectQuery + slicer on a time column folded to a comparison against Power BI's base date, + 30 December 1899, while Trino anchors a cast time on the current date, so the + report silently showed no rows. Power BI now refuses that fold with a visible + error instead. The README describes a view-based workaround. + ### Fixed - `INTERVAL YEAR TO MONTH` and `INTERVAL DAY TO SECOND` columns read as text now diff --git a/README.md b/README.md index a029391..e3cca4b 100644 --- a/README.md +++ b/README.md @@ -259,6 +259,19 @@ ignored, so the tool can react instead of trusting a wrong answer. somewhere you never asked for. Set `Catalog` when you connect. - **Row and field size limits are not faked.** Trino can only limit a result set through `LIMIT` in the SQL you wrote. +- **Power BI cannot filter on a time column in DirectQuery.** Picking a value + in a slicer on a `time` column fails with "We couldn't fold the expression to + the data source". Power BI would filter by casting the column to a timestamp + and comparing it with that time on 30 December 1899, while Trino, like ODBC + itself, puts a cast time on today's date, so the filter could never match. + The connector declares the cast unsupported so you see an error instead of an + empty report. To slice on a time of day, expose it as text in a Trino view, + for example `CAST(col_time AS VARCHAR) AS col_time_text`, and slice on that + column. Selecting "(Blank)" still works. Users who cannot create views can + switch the table to Import mode, where the slicer filters Power BI's own copy + of the data. That copy is only as fresh as its last refresh, and Power BI + treats a blank time as equal to midnight, so selecting 12:00:00 AM also shows + rows without a time. - **One isolation level.** Trino catalogs disagree about which levels they accept, so the driver offers the one they all support and refuses the rest up front, rather than letting a query fail later for a reason nobody can see. diff --git a/connector/StackableTrinoODBC.pq b/connector/StackableTrinoODBC.pq index 9040f2c..3940eb3 100644 --- a/connector/StackableTrinoODBC.pq +++ b/connector/StackableTrinoODBC.pq @@ -307,10 +307,10 @@ StackableTrinoODBCImpl = ( SupportsTop = false ], - // Nothing is overridden. An override here silently wins over - // SQLGetInfoW and cannot be corrected by fixing the driver, so the - // record is reserved for what the driver gets wrong, and this group it - // answers honestly: SQL_SQL92_PREDICATES, SQL_AGGREGATE_FUNCTIONS, + // One entry is overridden, SQL_CONVERT_TIME, below. An override here + // silently wins over SQLGetInfoW and cannot be corrected by fixing the + // driver, so the record is otherwise reserved for what the driver gets + // wrong, and this group it answers honestly: SQL_SQL92_PREDICATES, SQL_AGGREGATE_FUNCTIONS, // SQL_SQL92_RELATIONAL_JOIN_OPERATORS, SQL_SQL92_VALUE_EXPRESSIONS and // SQL_IDENTIFIER_QUOTE_CHAR. // @@ -327,7 +327,22 @@ StackableTrinoODBCImpl = ( // Nothing Power Query generates is lost: comparison, IN, // LIKE, BETWEEN, IS NULL, EXISTS and the four join types are all in // the driver's ungated set. - SQLGetInfo = defaultConfig[SQLGetInfo], + // + // SQL_CONVERT_TIME is the driver's answer (every SQL_CVT_* bit) minus + // SQL_CVT_TIMESTAMP, and that is a deliberate misreport: Trino can + // cast TIME to TIMESTAMP. It anchors the time on the current date, as + // ODBC's own conversion tables do ("SQL to C: Time", footnote [c]), + // while Power BI folds a time slicer to + // `cast(col as TIMESTAMP) = CAST('1899-12-30 hh:mm:ss' as TIMESTAMP)`, + // anchoring on its own base date. That filter ran and silently matched + // no row. Without the bit, Power BI refuses the fold and says so ("We + // couldn't fold the expression to the data source"), which is the + // honest outcome for a filter Trino cannot evaluate the way Power BI + // means it. test_folding_contract.py pins the value to the driver's + // answer and checks Trino's anchor is still the current date. + SQLGetInfo = defaultConfig[SQLGetInfo] & [ + SQL_CONVERT_TIME = 0x01FDFFFF + ], SQLColumns = (catalogName, schemaName, tableName, columnName, source) => source, diff --git a/integration-tests/suites/test_folding_contract.py b/integration-tests/suites/test_folding_contract.py index 8fb101a..aa83cd4 100644 --- a/integration-tests/suites/test_folding_contract.py +++ b/integration-tests/suites/test_folding_contract.py @@ -74,6 +74,10 @@ # as a folding gap misreports a gap that cannot exist. DM_COMPAT_ONLY = {"SQL_CHAR", "SQL_VARCHAR"} +# SQL_CVT_* bits (sqlext.h): every defined conversion target, and TIMESTAMP's. +SQL_CVT_ALL = 0x01FFFFFF +SQL_CVT_TIMESTAMP = 0x00020000 + def check(label, ok, detail=""): R.check(f"{label}{detail}", ok) @@ -405,6 +409,41 @@ def main(): "" if bindings_on else " (set false, which disables a function the driver declares)", ) + # ------------------------------------------------------------------ + print("\n--- TIME -> TIMESTAMP is withheld, so Power BI cannot fold it ---") + # A Power BI slicer on a time column folds to + # `cast("col" as TIMESTAMP) = CAST('1899-12-30 hh:mm:ss' as TIMESTAMP)`, + # anchoring the time on Power BI's base date. Trino's cast, like ODBC's own + # conversion tables ("SQL to C: Time" footnote [c]), uses the current date, + # so the filter silently matched nothing. Withholding SQL_CVT_TIMESTAMP + # from SQL_CONVERT_TIME makes Power BI refuse the fold with a visible error + # instead (measured in Power BI Desktop, 2026-10-07). The override is a + # deliberate misreport, so it must stay exactly "the driver's answer minus + # that one bit" and must be revisited if Trino's anchor ever changes. + override = re.search(r"SQL_CONVERT_TIME\s*=\s*(0x[0-9A-Fa-f]+|\d+)", source) + check( + "the connector overrides SQL_CONVERT_TIME", + override is not None, + "" if override else " (no SQL_CONVERT_TIME entry in the SQLGetInfo record)", + ) + if override: + declared = int(override.group(1), 0) + driver_answer = conn.getinfo(pyodbc.SQL_CONVERT_TIME) & SQL_CVT_ALL + expected = driver_answer & ~SQL_CVT_TIMESTAMP + check( + "SQL_CONVERT_TIME is the driver's answer without SQL_CVT_TIMESTAMP", + declared == expected, + f" (declared {declared:#010x}, expected {expected:#010x})", + ) + anchored_today = cur.execute( + "SELECT CAST(CAST(TIME '14:30:00' AS TIMESTAMP) AS DATE) = current_date" + ).fetchone()[0] + check( + "Trino still anchors TIME -> TIMESTAMP on the current date", + bool(anchored_today), + "" if anchored_today else " (it no longer does: revisit the SQL_CONVERT_TIME override)", + ) + # ------------------------------------------------------------------ print("\n--- driver types with no Constant visitor entry ---") # Not a failure: an absent key makes Power Query evaluate that constant diff --git a/integration-tests/suites/test_pbi_slicer_semantics.py b/integration-tests/suites/test_pbi_slicer_semantics.py index c8c0d4a..98d4d3e 100644 --- a/integration-tests/suites/test_pbi_slicer_semantics.py +++ b/integration-tests/suites/test_pbi_slicer_semantics.py @@ -60,11 +60,14 @@ def fraction7(v): "col_interval_ym": lambda v: ( f"cast(\"col_interval_ym\" as VARCHAR) = CAST('{text(v)}' as VARCHAR)" ), - # Power BI anchors a time of day on its base date, 1899-12-30. - "col_time": lambda v: ( - f"cast(\"col_time\" as TIMESTAMP) = " - f"CAST('1899-12-30 {v:%H:%M:%S}.{fraction7(v)}' as TIMESTAMP)" - ), + # Power BI anchors a time of day on its base date, 1899-12-30, and folds + # `cast("col_time" as TIMESTAMP) = CAST('1899-12-30 hh:mm:ss' as TIMESTAMP)`. + # Trino's cast uses the current date, as ODBC's conversion tables do, so + # that filter can never match. The connector therefore withholds + # SQL_CVT_TIMESTAMP from SQL_CONVERT_TIME and Power BI refuses the fold with + # a visible error; test_folding_contract.py checks that. Only "(Blank)" is + # checked here. + "col_time": None, "col_timestamptz": lambda v: ( f"\"col_timestamptz\" = CAST('{v:%Y-%m-%d %H:%M:%S}.{fraction7(v)}' as TIMESTAMP)" ), @@ -81,6 +84,8 @@ def check_zone(conn_str, zone_label): for _, x in pairs: if x is not None and x not in values: values.append(x) + if template is None: + values = [] for v in values: expected = sum(1 for _, x in pairs if x == v) where = template(v) From d7da7046632d23a5d1f0d5e3836a7ab7c2f4874c Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 7 Oct 2026 18:47:17 +0200 Subject: [PATCH 08/20] fix: deliver timestamp with time zone in the session time zone Power BI folds a slicer on a timestamp-with-time-zone column to "col" = CAST('' as TIMESTAMP), and Trino reads that plain TIMESTAMP literal in the session time zone. The driver delivered these values in UTC, so the filter only selected its rows in a UTC session; with TimeZone=Europe/Berlin it silently matched nothing. Values are now delivered as wall time in the session zone, the zone Trino itself uses for a plain TIMESTAMP. The zone is worked out per statement, in Trino's own order: a SET TIME ZONE in force (the time_zone_id session property the client records), else TimeZone=, else the coordinator's default, which the connect probe now reads with current_timezone() at no extra round trip. Trino can report a fixed offset such as +05:30, so SessionZone keeps both named zones and offsets. Every page of a result set uses the zone read at execution. An instant has one wall time, so the driver's direction is unambiguous; the two instants of an autumn overlap hour are both shown as the same time. Reading the folded literal back, Trino picks the later instant, so a slicer cannot select a value from the first repeated hour. The slicer suite pins that, and the README says so under Troubleshooting. Tests: unit tests for the conversion in named, fixed-offset and DST-overlap cases and for the zone precedence; FFI tests comparing delivered values with Trino's own at_timezone(x, current_timezone()) across TimeZone=, SET TIME ZONE and SET TIME ZONE LOCAL, both of which fail if the driver delivers UTC regardless; the slicer suite's Europe/Berlin checks now pass. The README's TimeZone row, which ended mid-sentence, is completed. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 7 + README.md | 8 +- .../suites/test_pbi_slicer_semantics.py | 28 ++ src/backend.rs | 113 +++++- src/backend/execute.rs | 19 +- src/ffi_integration_tests.rs | 156 +++++++- src/fuzz_api.rs | 9 +- src/type_conversion.rs | 337 +++++++++++++----- 8 files changed, 579 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b7c37d..5ea6865 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `timestamp with time zone` values are delivered as wall time in the session + time zone instead of in UTC: `TimeZone=` when set, otherwise the + coordinator's default, and whatever a later `SET TIME ZONE` chose. Power BI + folds the value it showed back as a plain `TIMESTAMP` literal, which Trino + reads in the session zone, so a DirectQuery slicer on such a column only + selected its rows in a UTC session. Applications reading these columns see + different values unless the session is UTC. - The Power BI connector reports that Trino cannot convert `TIME` to `TIMESTAMP` (`SQL_CONVERT_TIME` without `SQL_CVT_TIMESTAMP`). A DirectQuery slicer on a time column folded to a comparison against Power BI's base date, diff --git a/README.md b/README.md index e3cca4b..1e7aac3 100644 --- a/README.md +++ b/README.md @@ -148,7 +148,7 @@ The authoritative list is `src/backend/types/connect_params.rs`. | `Roles` | No | Authorisation role per catalog, `{catalog:role;catalog2:ALL}` | | `SessionUser` | No | User statements run as, while `User` still authenticates. JDBC's `sessionUser` | | `Path` | No | Default SQL path for resolving unqualified function names | -| `TimeZone` | No | IANA session time zone (`Europe/Berlin`). Unset leaves the coordinator's | +| `TimeZone` | No | IANA session time zone (`Europe/Berlin`). Unset leaves the coordinator's default. `timestamp with time zone` values are delivered as wall time in the session zone, which a later `SET TIME ZONE` changes | | `Locale` | No | Locale for locale-dependent formatting, sent as `X-Trino-Language` | | `ClientInfo` | No | Free-form client metadata Trino records against the query | | `TraceToken` | No | Correlation token Trino records against the query | @@ -320,6 +320,12 @@ the coordinator's chain is refused even when the machine trusts that chain. **Only the first session property applies.** Wrap the value in braces. See [Values that contain a semicolon](#values-that-contain-a-semicolon). +**A Power BI slicer misses a timestamp from the night the clocks go back.** +`timestamp with time zone` values are shown in the session time zone, and in +the hour that repeats, two instants share one wall time. Power BI filters on the +wall time it showed and Trino reads that as the later of the two instants, so a +value from the first of the repeated hours is not selected. + **The browser login never opens.** Some tools, `pyodbc` among them, tell the driver it may not display anything. The driver reports this rather than hanging. Use `AccessToken` with those tools, or connect through one that allows a prompt. diff --git a/integration-tests/suites/test_pbi_slicer_semantics.py b/integration-tests/suites/test_pbi_slicer_semantics.py index 98d4d3e..cdda35b 100644 --- a/integration-tests/suites/test_pbi_slicer_semantics.py +++ b/integration-tests/suites/test_pbi_slicer_semantics.py @@ -24,6 +24,7 @@ `hive.tx.interval_test` view this reads. Needs no compose profile. """ +import datetime import os import sys @@ -105,11 +106,38 @@ def check_zone(conn_str, zone_label): conn.close() +def check_dst_overlap(conn_str): + """Pin what a slicer does with a value in the autumn overlap hour. + + The driver delivers both instants of 2025-10-26 02:30 in Europe/Berlin as the + same wall time, because an instant has only one. Reading the folded literal + back, Trino has to pick one of the two instants and picks the later (CET) + one, so the earlier (CEST) row cannot be selected. That is Trino's reading of + an ambiguous local time, not the driver's, and this records it. + """ + print("\n--- DST overlap, Europe/Berlin ---") + conn = pyodbc.connect(conn_str + ";TimeZone=Europe/Berlin", autocommit=True) + cur = conn.cursor() + rows = ("(VALUES (1, from_iso8601_timestamp('2025-10-26T00:30:00Z')), " + "(2, from_iso8601_timestamp('2025-10-26T01:30:00Z'))) t(id, ts)") + shown = [v for _, v in cur.execute(f"SELECT id, ts FROM {rows} ORDER BY id").fetchall()] + want = datetime.datetime(2025, 10, 26, 2, 30) + R.check("both overlap instants are shown as 02:30", shown == [want, want], + "" if shown == [want, want] else f"shown {shown}") + where = "ts = CAST('2025-10-26 02:30:00.0000000' as TIMESTAMP)" + ids = [r[0] for r in cur.execute(f"SELECT id FROM {rows} WHERE {where} ORDER BY id")] + R.check("the folded 02:30 selects only the later (CET) instant", ids == [2], + "" if ids == [2] else f"selected ids {ids}") + cur.close() + conn.close() + + def main(): conn_str = sys.argv[1] if len(sys.argv) > 1 else Stack.load().conn_str() print(f"=== pbi slicer semantics ===\nview: {VIEW}") check_zone(conn_str, "server default") check_zone(conn_str + ";TimeZone=Europe/Berlin", "Europe/Berlin") + check_dst_overlap(conn_str) return R.summary() diff --git a/src/backend.rs b/src/backend.rs index db2c2b0..ed127e3 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -12,6 +12,8 @@ use std::time::Duration; use snafu::Snafu; use stackable_odbc_core::types::QueryTimeout; + +use crate::type_conversion::SessionZone; use stackable_odbc_core::{ backend::Backend, errors::OdbcError, @@ -127,6 +129,36 @@ fn session_user_name( .unwrap_or_default() .to_string() } + +/// Trino's session property for a `SET TIME ZONE`, which the coordinator +/// returns as `X-Trino-Set-Session` and clears again on `SET TIME ZONE LOCAL` +/// (`SetTimeZoneTask.java`, `SystemSessionProperties.TIME_ZONE_ID`). +const TIME_ZONE_ID_PROPERTY: &str = "time_zone_id"; + +/// The session time zone a statement's timestamp-with-time-zone values are +/// delivered in. +/// +/// In the order Trino itself applies them: a `SET TIME ZONE` in force +/// (`time_zone_id`), else `TimeZone=` (sent as `X-Trino-Time-Zone`), else the +/// coordinator's default, read at connect. A `time_zone_id` this driver cannot +/// read falls through rather than failing the fetch. +fn effective_session_zone( + time_zone_id: Option<&str>, + configured: Option, + server_default: SessionZone, +) -> SessionZone { + let set = time_zone_id.and_then(|id| { + let zone = SessionZone::parse(id); + if zone.is_none() { + tracing::warn!( + time_zone_id = id, + "unrecognised session time zone; ignoring it" + ); + } + zone + }); + set.or(configured).unwrap_or(server_default) +} // `pub(crate)` only under `cfg(test)`: the FFI integration tests // (`ffi_integration_tests.rs`, a sibling of this module under `lib.rs`) need to // reach the `TRINO_*` capability bitmap constants declared in `info`. Non-test @@ -608,7 +640,10 @@ fn connection_failed(e: TrinoError) -> TrinoError { /// two are read into the result independently rather than through a shared /// early return. fn probe_session(conn: &TrinoConnection) -> SessionProbe { - let rows = match query_all_rows(conn, "SELECT version(), current_user".to_string()) { + let rows = match query_all_rows( + conn, + "SELECT version(), current_user, current_timezone()".to_string(), + ) { Ok(rows) => rows, Err(e) => { tracing::warn!(error = %e, "could not read the Trino server version and session user"); @@ -621,6 +656,7 @@ fn probe_session(conn: &TrinoConnection) -> SessionProbe { let mut probe = SessionProbe { user: column(1).map(str::to_string), + time_zone: column(2).and_then(SessionZone::parse), ..SessionProbe::default() }; if probe.user.is_none() { @@ -666,6 +702,10 @@ struct SessionProbe { server_major: u32, /// Trino's `current_user`, for `SQL_USER_NAME`. user: Option, + /// Trino's `current_timezone()`: the session zone before any + /// `SET TIME ZONE`, which is `TimeZone=` when given and the coordinator's + /// default otherwise. + time_zone: Option, } /// The Trino [`stackable_odbc_core::backend::Backend`] implementation. @@ -711,6 +751,11 @@ pub struct TrinoConnection { /// off. Understating capability is the safe direction: a BI tool folds /// less than it could, rather than emitting SQL the server rejects. pub server_major: u32, + /// `TimeZone=` from the connection string, sent as `X-Trino-Time-Zone`. + pub configured_time_zone: Option, + /// The session zone `current_timezone()` reported at connect, or UTC when + /// the probe failed. Without `TimeZone=` this is the coordinator's default. + pub server_time_zone: SessionZone, /// The `DSN` the application connected with, for `SQL_DATA_SOURCE_NAME`. /// /// Empty when the connection string named no DSN, which is the one case the @@ -892,6 +937,23 @@ impl TrinoConnection { .is_active() } + /// The session time zone for a statement about to run. + /// + /// Read per statement, because `SET TIME ZONE` changes it after connect and + /// the client only records that as a session property. See + /// [`effective_session_zone`] for the order of the sources. + pub(crate) fn session_zone(&self) -> SessionZone { + let snapshot = self.runtime.block_on(self.client.session_snapshot()); + effective_session_zone( + snapshot + .properties + .get(TIME_ZONE_ID_PROPERTY) + .map(String::as_str), + self.configured_time_zone, + self.server_time_zone, + ) + } + /// Open a transaction if manual-commit mode wants one and none is open. /// /// Called from the statement paths rather than from @@ -1054,6 +1116,8 @@ pub(crate) fn disconnected_trino_conn_with_catalog(catalog: Option<&str>) -> Tri // leave behind if `fetch_server_version` could not reach a coordinator. dbms_version: String::new(), server_major: 0, + configured_time_zone: None, + server_time_zone: SessionZone::UTC, // The three identity strings, matching the client built above rather // than left empty: `TrinoBackend::connect` derives the first two // without reaching the coordinator, so a fabricated connection that @@ -1103,6 +1167,9 @@ pub struct TrinoStatement { pub(crate) columns: Vec, /// Column types from Trino (needed for converting subsequent pages). trino_types: Vec<(String, trino_rust_client::TrinoTy)>, + /// The session time zone at execution, which every page of this result + /// set is converted in, so one result set never mixes two zones. + session_zone: SessionZone, /// Trino's own column metadata for the result set, kept because a spooled /// segment is decoded against it and Trino sends it on one page only. /// @@ -1723,6 +1790,8 @@ impl Backend for TrinoBackend { client: Arc::new(client), dbms_version: String::new(), server_major: 0, + configured_time_zone: p.time_zone().map(SessionZone::Named), + server_time_zone: SessionZone::UTC, // Core supplies the DSN on both connection entry points, so this is // the whole of `SQL_DATA_SOURCE_NAME`: absent means the application // connected by driver rather than by DSN, which is the case the @@ -1742,6 +1811,9 @@ impl Backend for TrinoBackend { let probe = probe_session(&conn); conn.dbms_version = probe.dbms_version; conn.server_major = probe.server_major; + if let Some(zone) = probe.time_zone { + conn.server_time_zone = zone; + } conn.user_name = session_user_name(probe.user.as_deref(), p.session_user(), p.user()); Ok(conn) } @@ -3057,6 +3129,45 @@ mod tests { assert_eq!(session_user_name(None, None, None), ""); } + fn zone(name: &str) -> SessionZone { + SessionZone::parse(name).expect("a zone Trino reports") + } + + /// `SET TIME ZONE` changes the session after connect, and Trino reports it + /// as the `time_zone_id` session property, so it wins over both + /// connect-time sources. + #[test] + fn a_set_time_zone_wins_over_the_connection_string_and_the_server() { + assert_eq!( + effective_session_zone(Some("+05:30"), Some(zone("Europe/Berlin")), zone("UTC")), + zone("+05:30") + ); + } + + /// `TimeZone=` is sent as `X-Trino-Time-Zone`, so it is the session's zone + /// until a `SET TIME ZONE`; the server default applies only without it. + #[test] + fn the_connection_string_zone_wins_over_the_server_default() { + assert_eq!( + effective_session_zone(None, Some(zone("Europe/Berlin")), zone("UTC")), + zone("Europe/Berlin") + ); + assert_eq!( + effective_session_zone(None, None, zone("America/New_York")), + zone("America/New_York") + ); + } + + /// A `time_zone_id` this driver cannot read is not a reason to fail a + /// fetch: the next source is used, and the mismatch is logged. + #[test] + fn an_unreadable_time_zone_id_falls_back() { + assert_eq!( + effective_session_zone(Some("Mars/Olympus"), None, zone("UTC")), + zone("UTC") + ); + } + #[test] fn get_type_info_returns_all_trino_types() { let conn = disconnected_trino_conn(); diff --git a/src/backend/execute.rs b/src/backend/execute.rs index 70c6a8e..428155c 100644 --- a/src/backend/execute.rs +++ b/src/backend/execute.rs @@ -20,8 +20,8 @@ use super::{ map_trino_error_on, }; use crate::type_conversion::{ - TrinoTypeName, discards_fractional_seconds, json_to_column_value, trino_ty_precision, - trino_ty_scale, trino_ty_to_sql_type, type_name_precision, type_name_scale, + SessionZone, TrinoTypeName, discards_fractional_seconds, json_to_column_value, + trino_ty_precision, trino_ty_scale, trino_ty_to_sql_type, type_name_precision, type_name_scale, }; /// Convert decoded Trino rows into `Vec>`, alongside the cells @@ -37,6 +37,7 @@ use crate::type_conversion::{ fn convert_rows( rows: Vec, types: &[(String, TrinoTy)], + session_zone: SessionZone, ) -> (Vec>, HashSet<(usize, usize)>) { let mut truncated = HashSet::new(); let batch = rows @@ -51,7 +52,7 @@ fn convert_rows( if discards_fractional_seconds(&val, ty) { truncated.insert((row_idx, col_idx)); } - json_to_column_value(val, ty) + json_to_column_value(val, ty, session_zone) }) .collect() }) @@ -145,6 +146,10 @@ pub(super) fn exec_direct( // interpolating its parameters, so this is the only site that needs it. conn.ensure_transaction()?; + // Read before the submit: a `SET TIME ZONE` takes effect for the + // statements after it, not for itself. + let session_zone = conn.session_zone(); + let submit_start = Instant::now(); let mut page = { let _span = tracing::info_span!("trino.submit").entered(); @@ -272,7 +277,7 @@ pub(super) fn exec_direct( let _span = tracing::info_span!("trino.convert_batch", page = page_count).entered(); let rows = decode_page_rows(&conn.runtime, &conn.client, page.data, &kept_columns) .map_err(|e| conn.statement_error(e))?; - convert_rows(rows, &trino_types) + convert_rows(rows, &trino_types, session_zone) }; let total_convert_time = convert_start.elapsed(); let total_rows_fetched = batch.len() as u64; @@ -288,6 +293,7 @@ pub(super) fn exec_direct( pending_sql: None, columns, trino_types, + session_zone, raw_columns: kept_columns, batch, truncated_cells, @@ -325,6 +331,8 @@ pub(super) fn prepare( pending_sql: Some(sql.to_string()), columns: Vec::new(), trino_types: Vec::new(), + // Nothing is converted until `execute` replaces this statement. + session_zone: SessionZone::UTC, raw_columns: Vec::new(), batch: Vec::new(), truncated_cells: HashSet::new(), @@ -671,7 +679,7 @@ impl StatementBackend for TrinoStatement { decode_page_rows(runtime, client, page.data, &self.raw_columns) }; (self.batch, self.truncated_cells) = match decoded { - Ok(rows) => convert_rows(rows, &self.trino_types), + Ok(rows) => convert_rows(rows, &self.trino_types, self.session_zone), Err(e) => { let mapped = self.map_client_error(e); return Err(self.end_page_fetch(mapped)); @@ -859,6 +867,7 @@ mod tests { pending_sql: None, columns: Vec::new(), trino_types: Vec::new(), + session_zone: SessionZone::UTC, raw_columns: Vec::new(), batch: Vec::new(), truncated_cells: HashSet::new(), diff --git a/src/ffi_integration_tests.rs b/src/ffi_integration_tests.rs index 6c98c67..c22d93f 100644 --- a/src/ffi_integration_tests.rs +++ b/src/ffi_integration_tests.rs @@ -197,6 +197,12 @@ unsafe fn alloc_handles() -> (*mut c_void, *mut c_void, *mut c_void) { /// Helper: connect to Trino at localhost:8443. unsafe fn connect_trino(conn: *mut c_void) -> SqlReturn { + unsafe { connect_trino_with(conn, "") } +} + +/// [`connect_trino`] with `extra` appended to the connection string, e.g. +/// `";TimeZone=Europe/Berlin"`. +unsafe fn connect_trino_with(conn: *mut c_void, extra: &str) -> SqlReturn { // The terminator is part of the buffer, because the length argument below // is SQL_NTS: that tells the driver the string is null-terminated and to // find the end itself. Without it the driver reads past this Vec until it @@ -207,7 +213,10 @@ unsafe fn connect_trino(conn: *mut c_void) -> SqlReturn { // // Every other wide buffer in this file passes an explicit length instead, // and needs no terminator. - let wide: Vec = CONN_STR.encode_utf16().chain(std::iter::once(0)).collect(); + let wide: Vec = format!("{CONN_STR}{extra}") + .encode_utf16() + .chain(std::iter::once(0)) + .collect(); unsafe { ffi::connect::sql_driver_connect_w::( conn, @@ -1815,6 +1824,9 @@ fn timestamp_with_tz_returns_wchar() { #[serial] #[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] fn timestamp_with_named_tz_returns_utc_via_get_data() { + // UTC because the shared connection sets no `TimeZone=` and the test + // stack's coordinator defaults to UTC: values are delivered in the session + // zone (see `timestamptz_is_delivered_in_the_session_time_zone`). unsafe { let (_env, _conn, stmt) = alloc_stmt(); // America/New_York in March 2025 is EDT (UTC-4). @@ -1860,6 +1872,148 @@ fn timestamp_with_named_tz_returns_utc_via_get_data() { } } +/// Column `col` read as a `SQL_TIMESTAMP_STRUCT`. +unsafe fn get_timestamp_col(stmt: *mut c_void, col: u16) -> odbc_sys::Timestamp { + let mut ts = odbc_sys::Timestamp::default(); + let mut ind: isize = 0; + let ret = unsafe { + ffi::fetch::sql_get_data::( + stmt, + col, + CDataType::TypeTimestamp as i16, + (&raw mut ts).cast(), + std::mem::size_of::() as isize, + &mut ind, + ) + }; + assert_eq!( + ret, + SqlReturn::SUCCESS, + "sql_get_data(TypeTimestamp) on column {col}" + ); + ts +} + +/// Run `sql` to completion in its own statement on `conn`. +unsafe fn run_on(conn: *mut c_void, sql: &str) { + unsafe { + let mut stmt: *mut c_void = std::ptr::null_mut(); + assert_eq!( + ffi::handle::sql_alloc_handle::(HandleType::Stmt as i16, conn, &mut stmt), + SqlReturn::SUCCESS + ); + assert_eq!( + exec_direct(stmt, sql), + SqlReturn::SUCCESS, + "{sql}: {}", + diag_message(stmt) + ); + cleanup_stmt(stmt); + } +} + +/// Assert that a timestamp-with-time-zone value arrives as the wall time Trino +/// itself gives it in the session zone, and return that zone's name. +/// +/// The oracle is Trino's own `at_timezone(x, current_timezone())` cast to a +/// plain `TIMESTAMP`: the wall time a Power BI literal folded back from the +/// delivered value is compared against. +unsafe fn assert_delivered_in_session_zone(conn: *mut c_void, label: &str) -> String { + unsafe { + let mut stmt: *mut c_void = std::ptr::null_mut(); + assert_eq!( + ffi::handle::sql_alloc_handle::(HandleType::Stmt as i16, conn, &mut stmt), + SqlReturn::SUCCESS + ); + assert_eq!( + exec_direct( + stmt, + "SELECT x, CAST(at_timezone(x, current_timezone()) AS TIMESTAMP(3)), \ + current_timezone() \ + FROM (VALUES TIMESTAMP '2025-03-10 20:21:22.123 America/New_York') t(x)" + ), + SqlReturn::SUCCESS, + "{label}: {}", + diag_message(stmt) + ); + assert_eq!( + ffi::fetch::sql_fetch::(stmt), + SqlReturn::SUCCESS + ); + let delivered = get_timestamp_col(stmt, 1); + let trinos_wall_time = get_timestamp_col(stmt, 2); + let zone = get_wchar_col(stmt, 3); + assert_eq!(delivered, trinos_wall_time, "{label}: session zone {zone}"); + cleanup_stmt(stmt); + zone + } +} + +/// The session zone comes from `TimeZone=` when it is given, and from the +/// coordinator otherwise. +#[test] +#[serial] +#[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] +fn timestamptz_is_delivered_in_the_session_time_zone() { + for (extra, want) in [ + ("", None), + (";TimeZone=Europe/Berlin", Some("Europe/Berlin")), + (";TimeZone=America/New_York", Some("America/New_York")), + ] { + unsafe { + let (env, conn, stmt) = alloc_handles(); + assert_eq!( + connect_trino_with(conn, extra), + SqlReturn::SUCCESS, + "{extra}: {}", + conn_diag_message(conn) + ); + let zone = assert_delivered_in_session_zone(conn, extra); + if let Some(want) = want { + assert_eq!(zone, want, "{extra}"); + } + cleanup(env, conn, stmt); + } + } +} + +/// `SET TIME ZONE` changes the zone of the statements after it, and +/// `SET TIME ZONE LOCAL` changes it back, without reconnecting. +#[test] +#[serial] +#[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] +fn set_time_zone_moves_later_statements_into_the_new_zone() { + unsafe { + let (env, conn, stmt) = alloc_handles(); + assert_eq!( + connect_trino_with(conn, ";TimeZone=Europe/Berlin"), + SqlReturn::SUCCESS, + "{}", + conn_diag_message(conn) + ); + assert_eq!( + assert_delivered_in_session_zone(conn, "connected"), + "Europe/Berlin" + ); + + run_on(conn, "SET TIME ZONE '+05:30'"); + assert_eq!( + assert_delivered_in_session_zone(conn, "after +05:30"), + "+05:30" + ); + + run_on(conn, "SET TIME ZONE 'America/New_York'"); + assert_eq!( + assert_delivered_in_session_zone(conn, "after America/New_York"), + "America/New_York" + ); + + run_on(conn, "SET TIME ZONE LOCAL"); + assert_delivered_in_session_zone(conn, "after LOCAL"); + cleanup(env, conn, stmt); + } +} + #[test] #[serial] #[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] diff --git a/src/fuzz_api.rs b/src/fuzz_api.rs index 3b31357..ce142cc 100644 --- a/src/fuzz_api.rs +++ b/src/fuzz_api.rs @@ -33,15 +33,18 @@ use stackable_odbc_core::types::ColumnValue; use trino_rust_client::TrinoTy; pub use crate::type_conversion::{ - json_to_column_value, trino_type_name_to_sql_type, type_name_precision, type_name_scale, + SessionZone, json_to_column_value, trino_type_name_to_sql_type, type_name_precision, + type_name_scale, }; /// Convert one coordinator JSON value under its declared Trino type. /// /// A thin alias for [`json_to_column_value`], kept so a target can name the -/// whole read-path conversion without importing the type-name helpers. +/// whole read-path conversion without importing the type-name helpers. The +/// session zone is one with daylight saving time, so fuzzed timestamps also +/// reach the overlap and gap hours. pub fn json_value(val: Value, ty: &TrinoTy) -> ColumnValue { - json_to_column_value(val, ty) + json_to_column_value(val, ty, SessionZone::Named(chrono_tz::Tz::Europe__Berlin)) } /// Run core's ODBC escape translator with this driver's dialect. diff --git a/src/type_conversion.rs b/src/type_conversion.rs index ec5b6e8..f2c9b15 100644 --- a/src/type_conversion.rs +++ b/src/type_conversion.rs @@ -922,16 +922,68 @@ fn parse_trino_timestamp(s: &str) -> Option { }) } -/// Parse a Trino TIMESTAMP WITH TIME ZONE string and convert to UTC. +/// The session time zone, in which timestamp-with-time-zone values are +/// delivered. +/// +/// Trino reports it from `current_timezone()` either as an IANA name (`UTC`, +/// `Europe/Berlin`) or, after `SET TIME ZONE '+05:30'`, as a fixed offset, so +/// both forms are kept. +#[derive(Debug, Clone, Copy, PartialEq)] +pub enum SessionZone { + Named(chrono_tz::Tz), + Fixed(chrono::FixedOffset), +} + +impl SessionZone { + pub const UTC: SessionZone = SessionZone::Named(chrono_tz::Tz::UTC); + + /// Read a zone the way Trino names one: an IANA name, or `±hh:mm`. + pub fn parse(s: &str) -> Option { + let s = s.trim(); + let sign = match s.as_bytes().first()? { + b'+' => 1, + b'-' => -1, + _ => return s.parse().ok().map(SessionZone::Named), + }; + let (h, m) = s[1..].split_once(':')?; + if h.len() != 2 || m.len() != 2 { + return None; + } + let (h, m): (i32, i32) = (h.parse().ok()?, m.parse().ok()?); + if h > 18 || m > 59 { + return None; + } + chrono::FixedOffset::east_opt(sign * (h * 3600 + m * 60)).map(SessionZone::Fixed) + } + + /// The wall time in this zone at the instant `utc`. + /// + /// Unambiguous: every instant has exactly one wall time, including the two + /// instants of an autumn overlap hour, which share one. + fn wall_time(self, utc: chrono::NaiveDateTime) -> chrono::NaiveDateTime { + use chrono::TimeZone as _; + match self { + SessionZone::Named(tz) => tz.from_utc_datetime(&utc).naive_local(), + SessionZone::Fixed(offset) => offset.from_utc_datetime(&utc).naive_local(), + } + } +} + +/// Parse a Trino TIMESTAMP WITH TIME ZONE string and deliver it as wall time +/// in the session time zone. /// /// Trino REST API format: `"YYYY-MM-DD HH:MM:SS.fraction TIMEZONE"` where /// TIMEZONE is either a numeric offset (`+05:30`, `-08:00`) or a named IANA -/// zone (`UTC`, `America/New_York`, `Europe/Berlin`). +/// zone (`UTC`, `America/New_York`, `Europe/Berlin`). That zone fixes the +/// instant; `session` decides how it is shown. /// -/// Returns `ColumnValue::Timestamp` with UTC-converted fields, matching the -/// official Trino ODBC driver's behaviour. The timezone information is consumed -/// during conversion: `SQL_TIMESTAMP_STRUCT` has no timezone field. -fn parse_trino_timestamp_tz(s: &str) -> Option { +/// `SQL_TIMESTAMP_STRUCT` has no time zone field, so one zone has to be chosen, +/// and the session's is the one Trino itself uses for a plain `TIMESTAMP`. A +/// Power BI slicer folds the value it showed back as exactly such a literal +/// (`"col" = CAST('2025-06-15 14:30:00' as TIMESTAMP)`), and Trino compares it +/// with the column after reading it in the session zone. Delivering UTC made +/// that filter match only in a UTC session. +fn parse_trino_timestamp_tz(s: &str, session: SessionZone) -> Option { let last_space = s.rfind(' ')?; let datetime_part = &s[..last_space]; let tz_part = s[last_space + 1..].trim(); @@ -990,15 +1042,17 @@ fn parse_trino_timestamp_tz(s: &str) -> Option { .naive_utc() }; - // Fraction (sub-second nanoseconds) is preserved as-is: UTC conversion - // only shifts hours/minutes/seconds, never sub-second precision. + let local = session.wall_time(utc_ndt); + + // Fraction (sub-second nanoseconds) is preserved as-is: a zone change only + // shifts hours/minutes/seconds, never sub-second precision. Some(ColumnValue::Timestamp { - year: i16::try_from(utc_ndt.date().year()).ok()?, - month: utc_ndt.date().month() as u16, - day: utc_ndt.date().day() as u16, - hour: utc_ndt.time().hour() as u16, - minute: utc_ndt.time().minute() as u16, - second: utc_ndt.time().second() as u16, + year: i16::try_from(local.date().year()).ok()?, + month: local.date().month() as u16, + day: local.date().day() as u16, + hour: local.time().hour() as u16, + minute: local.time().minute() as u16, + second: local.time().second() as u16, fraction, }) } @@ -1047,7 +1101,10 @@ fn json_as_text(val: &Value) -> String { } /// Convert a JSON value from Trino to an ODBC ColumnValue, guided by the column type. -pub fn json_to_column_value(val: Value, ty: &TrinoTy) -> ColumnValue { +/// +/// `session` is the session time zone, in which a timestamp-with-time-zone value +/// is delivered (see [`parse_trino_timestamp_tz`]). +pub fn json_to_column_value(val: Value, ty: &TrinoTy, session: SessionZone) -> ColumnValue { if val.is_null() { return ColumnValue::Null; } @@ -1162,13 +1219,14 @@ pub fn json_to_column_value(val: Value, ty: &TrinoTy) -> ColumnValue { ColumnValue::String(val.to_string()) } } - // TIMESTAMP WITH TIME ZONE: parse and convert to UTC via chrono-tz. - // Trino sends named zones (UTC, America/New_York, CET) or numeric - // offsets (+05:30, -08:00); the column type in the REST API metadata - // determines which parser is called, not the string content. + // TIMESTAMP WITH TIME ZONE: parse, then deliver in the session time + // zone via chrono-tz. Trino sends named zones (UTC, America/New_York, + // CET) or numeric offsets (+05:30, -08:00); the column type in the REST + // API metadata determines which parser is called, not the string + // content. TrinoTy::TimestampWithTimeZone => { if let Value::String(ref s) = val { - parse_trino_timestamp_tz(s).unwrap_or_else(|| { + parse_trino_timestamp_tz(s, session).unwrap_or_else(|| { tracing::warn!( raw = s, "failed to parse Trino TIMESTAMP WITH TIME ZONE string" @@ -1213,7 +1271,7 @@ pub fn json_to_column_value(val: Value, ty: &TrinoTy) -> ColumnValue { if let Value::Array(items) = val { let vals = items .into_iter() - .map(|v| json_to_column_value(v, inner_ty)) + .map(|v| json_to_column_value(v, inner_ty, session)) .collect(); ColumnValue::Array(vals) } else { @@ -1225,8 +1283,8 @@ pub fn json_to_column_value(val: Value, ty: &TrinoTy) -> ColumnValue { let pairs = map .into_iter() .map(|(k, v)| { - let key_col = json_to_column_value(Value::String(k), key_ty); - let val_col = json_to_column_value(v, val_ty); + let key_col = json_to_column_value(Value::String(k), key_ty, session); + let val_col = json_to_column_value(v, val_ty, session); (key_col, val_col) }) .collect(); @@ -1240,7 +1298,7 @@ pub fn json_to_column_value(val: Value, ty: &TrinoTy) -> ColumnValue { let vals = items .into_iter() .zip(fields.iter()) - .map(|(v, (_name, ty))| json_to_column_value(v, ty)) + .map(|(v, (_name, ty))| json_to_column_value(v, ty, session)) .collect(); ColumnValue::Row(vals) } else { @@ -1252,7 +1310,7 @@ pub fn json_to_column_value(val: Value, ty: &TrinoTy) -> ColumnValue { let vals = items .into_iter() .zip(fields.iter()) - .map(|(v, ty)| json_to_column_value(v, ty)) + .map(|(v, ty)| json_to_column_value(v, ty, session)) .collect(); ColumnValue::Row(vals) } else { @@ -1260,7 +1318,7 @@ pub fn json_to_column_value(val: Value, ty: &TrinoTy) -> ColumnValue { } } // Nullable wrapper: delegate to the inner type (null already handled above) - TrinoTy::Option(inner) => json_to_column_value(val, inner), + TrinoTy::Option(inner) => json_to_column_value(val, inner, session), _ => match val { Value::String(s) => ColumnValue::String(s), other => ColumnValue::String(other.to_string()), @@ -1272,6 +1330,120 @@ pub fn json_to_column_value(val: Value, ty: &TrinoTy) -> ColumnValue { mod tests { use super::*; + /// Convert in a UTC session, which is what every test below that does not + /// name a session zone is about. + fn convert(val: Value, ty: &TrinoTy) -> ColumnValue { + json_to_column_value(val, ty, SessionZone::UTC) + } + + fn timestamp( + (year, month, day): (i16, u16, u16), + (hour, minute, second): (u16, u16, u16), + fraction: u32, + ) -> ColumnValue { + ColumnValue::Timestamp { + year, + month, + day, + hour, + minute, + second, + fraction, + } + } + + fn tz_in(raw: &str, zone: &str) -> ColumnValue { + json_to_column_value( + Value::String(raw.into()), + &TrinoTy::TimestampWithTimeZone, + SessionZone::parse(zone).expect("a zone Trino reports"), + ) + } + + /// A timestamp-with-time-zone value is delivered as wall time in the + /// session time zone. Power BI folds the value it was shown back as a + /// plain `TIMESTAMP` literal, which Trino reads in the session zone; the + /// two only agree if the shown value is already in that zone. + #[test] + fn timestamp_tz_is_delivered_in_the_session_zone() { + // Summer: CEST, UTC+2. + assert_eq!( + tz_in("2025-06-15 12:30:00.000000 UTC", "Europe/Berlin"), + timestamp((2025, 6, 15), (14, 30, 0), 0) + ); + // Winter: CET, UTC+1. + assert_eq!( + tz_in("2025-01-15 12:30:00.000000 UTC", "Europe/Berlin"), + timestamp((2025, 1, 15), (13, 30, 0), 0) + ); + // A UTC session keeps UTC. + assert_eq!( + tz_in("2025-06-15 12:30:00.000000 UTC", "UTC"), + timestamp((2025, 6, 15), (12, 30, 0), 0) + ); + } + + /// The value's own zone and the session's are independent: the instant is + /// fixed by the first and rendered in the second, across a date boundary. + #[test] + fn timestamp_tz_moves_from_its_own_zone_to_the_session_zone() { + // 20:21:22.123 EDT (UTC-4) = 00:21:22.123 UTC = 01:21:22.123 CET. + assert_eq!( + tz_in("2025-03-10 20:21:22.123 America/New_York", "Europe/Berlin"), + timestamp((2025, 3, 11), (1, 21, 22), 123_000_000) + ); + } + + /// Trino reports a fixed-offset session zone as `+05:30` from + /// `current_timezone()` after `SET TIME ZONE '+05:30'`. + #[test] + fn timestamp_tz_is_delivered_in_a_fixed_offset_session_zone() { + assert_eq!( + tz_in("2025-06-15 12:30:00.000000 UTC", "+05:30"), + timestamp((2025, 6, 15), (18, 0, 0), 0) + ); + assert_eq!( + tz_in("2025-06-15 12:30:00.000000 UTC", "-08:00"), + timestamp((2025, 6, 15), (4, 30, 0), 0) + ); + } + + /// Both instants of the autumn overlap hour render as the same wall time: + /// an instant has exactly one wall time, so this direction is unambiguous. + /// The ambiguity is Trino's, reading the folded literal back, and it picks + /// the later instant (measured on Trino 483); a value in the first 02:30 + /// hour therefore cannot be selected by a Power BI slicer. + #[test] + fn both_instants_of_the_dst_overlap_render_as_the_same_wall_time() { + for utc in [ + "2025-10-26 00:30:00.000000 UTC", + "2025-10-26 01:30:00.000000 UTC", + ] { + assert_eq!( + tz_in(utc, "Europe/Berlin"), + timestamp((2025, 10, 26), (2, 30, 0), 0), + "{utc}" + ); + } + } + + #[test] + fn session_zone_parses_what_current_timezone_reports() { + for name in [ + "UTC", + "Europe/Berlin", + "America/New_York", + "+05:30", + "-08:00", + "+00:00", + ] { + assert!(SessionZone::parse(name).is_some(), "{name}"); + } + for bad in ["", "Europe/Berlim", "+5", "+25:00", "05:30"] { + assert_eq!(SessionZone::parse(bad), None, "{bad:?}"); + } + } + #[test] fn bigint_maps_to_ext_big_int() { assert_eq!( @@ -1309,7 +1481,7 @@ mod tests { // base64("\xDE\xAD\xBE\xEF") == "3q2+7w==" let val = Value::String("3q2+7w==".to_string()); assert_eq!( - json_to_column_value(val, &TrinoTy::VarBinary), + convert(val, &TrinoTy::VarBinary), ColumnValue::Bytes(vec![0xDE, 0xAD, 0xBE, 0xEF]) ); } @@ -1318,7 +1490,7 @@ mod tests { fn varbinary_empty_decodes_to_empty_bytes() { let val = Value::String(String::new()); assert_eq!( - json_to_column_value(val, &TrinoTy::VarBinary), + convert(val, &TrinoTy::VarBinary), ColumnValue::Bytes(Vec::new()) ); } @@ -1327,17 +1499,14 @@ mod tests { fn varbinary_invalid_base64_falls_back_to_string() { let val = Value::String("not!valid!base64".to_string()); assert_eq!( - json_to_column_value(val, &TrinoTy::VarBinary), + convert(val, &TrinoTy::VarBinary), ColumnValue::String("not!valid!base64".to_string()) ); } #[test] fn varbinary_null_maps_to_null() { - assert_eq!( - json_to_column_value(Value::Null, &TrinoTy::VarBinary), - ColumnValue::Null - ); + assert_eq!(convert(Value::Null, &TrinoTy::VarBinary), ColumnValue::Null); } #[test] @@ -1383,16 +1552,13 @@ mod tests { #[test] fn json_null_returns_column_null() { - assert_eq!( - json_to_column_value(Value::Null, &TrinoTy::Varchar), - ColumnValue::Null - ); + assert_eq!(convert(Value::Null, &TrinoTy::Varchar), ColumnValue::Null); } #[test] fn json_string_returns_column_string() { assert_eq!( - json_to_column_value(Value::String("hello".into()), &TrinoTy::Varchar), + convert(Value::String("hello".into()), &TrinoTy::Varchar), ColumnValue::String("hello".into()) ); } @@ -1400,7 +1566,7 @@ mod tests { #[test] fn json_number_bigint_returns_i64() { assert_eq!( - json_to_column_value(serde_json::json!(42), &TrinoTy::TrinoInt(TrinoInt::I64)), + convert(serde_json::json!(42), &TrinoTy::TrinoInt(TrinoInt::I64)), ColumnValue::I64(42) ); } @@ -1410,7 +1576,7 @@ mod tests { #[test] fn out_of_range_integer_for_declared_type_is_an_error_not_a_wrap() { // Server declared INTEGER but sent a value that does not fit i32. - let val = json_to_column_value( + let val = convert( serde_json::json!(4_294_967_296i64), &TrinoTy::TrinoInt(TrinoInt::I32), ); @@ -1421,13 +1587,13 @@ mod tests { #[test] fn in_range_integer_still_converts() { - let val = json_to_column_value(serde_json::json!(42i64), &TrinoTy::TrinoInt(TrinoInt::I32)); + let val = convert(serde_json::json!(42i64), &TrinoTy::TrinoInt(TrinoInt::I32)); assert_eq!(val, ColumnValue::I32(42)); } #[test] fn out_of_range_i16_falls_back_to_text() { - let val = json_to_column_value( + let val = convert( serde_json::json!(70_000i64), &TrinoTy::TrinoInt(TrinoInt::I16), ); @@ -1436,13 +1602,13 @@ mod tests { #[test] fn out_of_range_i8_falls_back_to_text() { - let val = json_to_column_value(serde_json::json!(200i64), &TrinoTy::TrinoInt(TrinoInt::I8)); + let val = convert(serde_json::json!(200i64), &TrinoTy::TrinoInt(TrinoInt::I8)); assert_eq!(val, ColumnValue::String("200".to_string())); } #[test] fn negative_out_of_range_integer_falls_back_to_text() { - let val = json_to_column_value( + let val = convert( serde_json::json!(-2_147_483_649i64), &TrinoTy::TrinoInt(TrinoInt::I32), ); @@ -1451,7 +1617,7 @@ mod tests { #[test] fn out_of_range_float_for_real_is_not_infinity() { - let val = json_to_column_value( + let val = convert( serde_json::json!(1e300f64), &TrinoTy::TrinoFloat(TrinoFloat::F32), ); @@ -1473,7 +1639,7 @@ mod tests { ("Infinity", f64::INFINITY), ("-Infinity", f64::NEG_INFINITY), ] { - let val = json_to_column_value( + let val = convert( serde_json::json!(raw), &TrinoTy::TrinoFloat(TrinoFloat::F64), ); @@ -1497,7 +1663,7 @@ mod tests { ("Infinity", f32::INFINITY), ("-Infinity", f32::NEG_INFINITY), ] { - let val = json_to_column_value( + let val = convert( serde_json::json!(raw), &TrinoTy::TrinoFloat(TrinoFloat::F32), ); @@ -1521,7 +1687,7 @@ mod tests { /// text two literal quote marks it never sent. #[test] fn unparseable_float_string_falls_back_without_json_quotes() { - let val = json_to_column_value( + let val = convert( serde_json::json!("abc"), &TrinoTy::TrinoFloat(TrinoFloat::F64), ); @@ -1530,7 +1696,7 @@ mod tests { #[test] fn in_range_float_for_real_still_converts() { - let val = json_to_column_value( + let val = convert( serde_json::json!(3.5f64), &TrinoTy::TrinoFloat(TrinoFloat::F32), ); @@ -1551,7 +1717,7 @@ mod tests { #[test] fn json_bool_returns_column_bool() { assert_eq!( - json_to_column_value(Value::Bool(true), &TrinoTy::Boolean), + convert(Value::Bool(true), &TrinoTy::Boolean), ColumnValue::Bool(true) ); } @@ -2031,7 +2197,7 @@ mod tests { #[test] fn option_i64_non_null_converts_as_i64() { assert_eq!( - json_to_column_value( + convert( serde_json::json!(99), &TrinoTy::Option(Box::new(TrinoTy::TrinoInt(TrinoInt::I64))) ), @@ -2042,7 +2208,7 @@ mod tests { #[test] fn option_i64_null_returns_null() { assert_eq!( - json_to_column_value( + convert( Value::Null, &TrinoTy::Option(Box::new(TrinoTy::TrinoInt(TrinoInt::I64))) ), @@ -2055,7 +2221,7 @@ mod tests { #[test] fn date_string_parses_to_column_date() { assert_eq!( - json_to_column_value(Value::String("1998-01-14".into()), &TrinoTy::Date), + convert(Value::String("1998-01-14".into()), &TrinoTy::Date), ColumnValue::Date { year: 1998, month: 1, @@ -2075,7 +2241,7 @@ mod tests { #[test] fn a_date_before_1_ce_parses_to_column_date() { assert_eq!( - json_to_column_value(Value::String("-0001-01-01".into()), &TrinoTy::Date), + convert(Value::String("-0001-01-01".into()), &TrinoTy::Date), ColumnValue::Date { year: -1, month: 1, @@ -2091,7 +2257,7 @@ mod tests { #[test] fn a_timestamp_before_1_ce_parses_to_column_timestamp() { assert_eq!( - json_to_column_value( + convert( Value::String("-0001-01-01 12:34:56.789".into()), &TrinoTy::Timestamp ), @@ -2113,8 +2279,8 @@ mod tests { #[test] fn dates_and_timestamps_read_the_same_years() { for year in ["-4713", "-0001", "0000", "0001", "1970", "9999"] { - let date = json_to_column_value(Value::String(format!("{year}-06-15")), &TrinoTy::Date); - let timestamp = json_to_column_value( + let date = convert(Value::String(format!("{year}-06-15")), &TrinoTy::Date); + let timestamp = convert( Value::String(format!("{year}-06-15 00:00:00")), &TrinoTy::Timestamp, ); @@ -2131,7 +2297,7 @@ mod tests { #[test] fn year_zero_parses_to_column_date() { assert_eq!( - json_to_column_value(Value::String("0000-01-01".into()), &TrinoTy::Date), + convert(Value::String("0000-01-01".into()), &TrinoTy::Date), ColumnValue::Date { year: 0, month: 1, @@ -2146,7 +2312,7 @@ mod tests { #[test] fn a_year_beyond_the_date_struct_falls_back_to_text() { assert!(matches!( - json_to_column_value(Value::String("+99999-01-01".into()), &TrinoTy::Date), + convert(Value::String("+99999-01-01".into()), &TrinoTy::Date), ColumnValue::String(_) )); } @@ -2154,7 +2320,7 @@ mod tests { #[test] fn time_string_parses_to_column_time() { assert_eq!( - json_to_column_value(Value::String("13:14:15".into()), &TrinoTy::Time), + convert(Value::String("13:14:15".into()), &TrinoTy::Time), ColumnValue::Time { hour: 13, minute: 14, @@ -2169,7 +2335,7 @@ mod tests { // The fraction is kept, not discarded: SQL_TIME_STRUCT cannot carry // it, but the SQL_C_CHAR/SQL_C_WCHAR string rendering can. assert_eq!( - json_to_column_value(Value::String("09:05:03.336".into()), &TrinoTy::Time), + convert(Value::String("09:05:03.336".into()), &TrinoTy::Time), ColumnValue::Time { hour: 9, minute: 5, @@ -2182,7 +2348,7 @@ mod tests { #[test] fn time_with_timezone_parses_correctly() { assert_eq!( - json_to_column_value( + convert( Value::String("13:14:15.000 UTC".into()), &TrinoTy::TimeWithTimeZone ), @@ -2279,7 +2445,7 @@ mod tests { // *different* time instead of declining the value. assert_eq!(parse_trino_time_with_tz("00:00:00+949378864"), None); assert_eq!( - json_to_column_value( + convert( Value::String("+999\0\0\0\0+00949378864".into()), &TrinoTy::Option(Box::new(TrinoTy::TimeWithTimeZone)), ), @@ -2322,7 +2488,7 @@ mod tests { #[test] fn timestamp_string_parses_to_column_timestamp() { assert_eq!( - json_to_column_value( + convert( Value::String("1998-01-14 13:14:15".into()), &TrinoTy::Timestamp ), @@ -2341,7 +2507,7 @@ mod tests { #[test] fn timestamp_with_millis_converts_fraction_to_nanoseconds() { assert_eq!( - json_to_column_value( + convert( Value::String("1998-01-14 13:14:15.123".into()), &TrinoTy::Timestamp ), @@ -2360,7 +2526,7 @@ mod tests { /// UTC is a no-op conversion: fields should pass through unchanged. #[test] fn timestamp_with_named_timezone_converts_to_utc() { - let val = json_to_column_value( + let val = convert( Value::String("1998-01-14 13:14:15.000 UTC".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2380,23 +2546,20 @@ mod tests { #[test] fn date_null_returns_null() { - assert_eq!( - json_to_column_value(Value::Null, &TrinoTy::Date), - ColumnValue::Null - ); + assert_eq!(convert(Value::Null, &TrinoTy::Date), ColumnValue::Null); } #[test] fn decimal_maps_to_decimal_variant() { use serde_json::json; - let val = json_to_column_value(json!("123.456"), &TrinoTy::Decimal(6, 3)); + let val = convert(json!("123.456"), &TrinoTy::Decimal(6, 3)); assert_eq!(val, ColumnValue::Decimal("123.456".to_string())); } #[test] fn json_ty_maps_to_json_variant() { use serde_json::json; - let val = json_to_column_value(json!(r#"{"a":1}"#), &TrinoTy::Json); + let val = convert(json!(r#"{"a":1}"#), &TrinoTy::Json); assert_eq!(val, ColumnValue::Json(r#"{"a":1}"#.to_string())); } @@ -2412,7 +2575,7 @@ mod tests { fn interval_year_month_is_delivered_as_trinos_text() { use serde_json::json; for raw in ["3-7", "-1-0", "0-3"] { - let val = json_to_column_value(json!(raw), &TrinoTy::IntervalYearToMonth); + let val = convert(json!(raw), &TrinoTy::IntervalYearToMonth); assert_eq!(val, ColumnValue::String(raw.to_string()), "{raw}"); } } @@ -2428,7 +2591,7 @@ mod tests { "-0 03:04:05.000", "0 00:00:00.500", ] { - let val = json_to_column_value(json!(raw), &TrinoTy::IntervalDayToSecond); + let val = convert(json!(raw), &TrinoTy::IntervalDayToSecond); assert_eq!(val, ColumnValue::String(raw.to_string()), "{raw}"); } } @@ -2436,7 +2599,7 @@ mod tests { /// Numeric offset +05:30: 10:30 local = 05:00 UTC (subtract 5h30m). #[test] fn timestamp_with_tz_numeric_offset_converts_to_utc() { - let val = json_to_column_value( + let val = convert( Value::String("2024-03-15 10:30:00.000 +05:30".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2456,7 +2619,7 @@ mod tests { #[test] fn timestamp_tz_named_utc_converts_to_utc_timestamp() { - let val = json_to_column_value( + let val = convert( Value::String("2020-05-05 22:00:00.000 UTC".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2478,7 +2641,7 @@ mod tests { fn timestamp_tz_named_zone_converts_to_utc() { // America/New_York in March 2025 is EDT (UTC-4). // 20:21:22 EDT = 2025-03-11 00:21:22 UTC (date rolls forward). - let val = json_to_column_value( + let val = convert( Value::String("2025-03-10 20:21:22.123 America/New_York".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2500,7 +2663,7 @@ mod tests { fn timestamp_tz_numeric_offset_converts_to_utc() { // +05:30 means wall clock is 5h30m ahead of UTC. // 10:30:00 +05:30 = 05:00:00 UTC (same day). - let val = json_to_column_value( + let val = convert( Value::String("2024-03-15 10:30:00.000 +05:30".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2521,7 +2684,7 @@ mod tests { #[test] fn timestamp_tz_negative_offset_converts_to_utc() { // -08:00: 16:00:00 PST = 2024-12-16 00:00:00 UTC (date rolls forward). - let val = json_to_column_value( + let val = convert( Value::String("2024-12-15 16:00:00.000 -08:00".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2543,7 +2706,7 @@ mod tests { fn timestamp_tz_dst_winter_converts_correctly() { // America/New_York in December is EST (UTC-5). // 23:00:00 EST = 2025-01-02 04:00:00 UTC. - let val = json_to_column_value( + let val = convert( Value::String("2025-01-01 23:00:00.000 America/New_York".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2567,7 +2730,7 @@ mod tests { fn timestamp_tz_posix_abbreviation_cet_converts_to_utc() { // CET (Central European Time) = UTC+1. // 15:00:00 CET = 14:00:00 UTC. - let val = json_to_column_value( + let val = convert( Value::String("2025-01-15 15:00:00.000 CET".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2590,7 +2753,7 @@ mod tests { /// a winter date so the expected result matches CET. #[test] fn timestamp_tz_europe_berlin_winter_converts_to_utc() { - let val = json_to_column_value( + let val = convert( Value::String("2025-01-15 15:00:00.000 Europe/Berlin".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2613,7 +2776,7 @@ mod tests { fn timestamp_tz_europe_berlin_summer_converts_to_utc() { // 2025-07-15 is in CEST (UTC+2). // 15:00:00 CEST = 13:00:00 UTC. - let val = json_to_column_value( + let val = convert( Value::String("2025-07-15 15:00:00.000 Europe/Berlin".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2636,7 +2799,7 @@ mod tests { #[test] fn timestamp_tz_hour_only_numeric_offset() { // +05 = +05:00. 10:00:00 +05:00 = 05:00:00 UTC. - let val = json_to_column_value( + let val = convert( Value::String("2024-06-01 10:00:00.000 +05".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2657,7 +2820,7 @@ mod tests { /// Zero offset (+00:00) is equivalent to UTC. #[test] fn timestamp_tz_zero_numeric_offset() { - let val = json_to_column_value( + let val = convert( Value::String("2024-06-01 10:00:00.000 +00:00".into()), &TrinoTy::TimestampWithTimeZone, ); @@ -2679,7 +2842,7 @@ mod tests { /// `TrinoTy::Timestamp`, and no UTC conversion applies to it. #[test] fn timestamp_no_tz_is_unaffected_by_tz_changes() { - let val = json_to_column_value( + let val = convert( Value::String("2025-06-15 09:30:45.678".into()), &TrinoTy::Timestamp, ); @@ -2701,7 +2864,7 @@ mod tests { #[test] fn timestamp_tz_preserves_fraction_through_conversion() { // 10:30:00.999 +05:30 = 05:00:00.999 UTC; fraction stays 999ms. - let val = json_to_column_value( + let val = convert( Value::String("2024-03-15 10:30:00.999 +05:30".into()), &TrinoTy::TimestampWithTimeZone, ); From 34fc42413da33471af8353a0b27e9cb15c82a6a5 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 7 Oct 2026 18:49:49 +0200 Subject: [PATCH 09/20] docs: add changelog entries for the type-info and connector quoting fixes 0e858e6 (the preferred SQLGetTypeInfo row) and dd31b43 (quoting text constants in the connector's Constant visitor) are both observable by applications but landed without changelog entries. Both are needed for DirectQuery slicers on text columns, so the entry for the connector says that the driver and the connector have to be upgraded together. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ea6865..d8216de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `SQLGetTypeInfo` lists the plain type first among the rows that share a + `DATA_TYPE`: `VARCHAR` for `SQL_WVARCHAR`, `TIME` for `SQL_TYPE_TIME` and + `TIMESTAMP` for `SQL_TYPE_TIMESTAMP`, as the spec's "how closely the data type + maps" ordering requires. The rows were sorted by name, so `SQL_WVARCHAR` led + with `INTERVAL DAY TO SECOND`, and Power Query, which takes the first row as + its `CAST` target, folded a DirectQuery slicer on a text column into + `CAST(... AS INTERVAL DAY TO SECOND)`, which Trino rejected. +- The Power BI connector quotes text constants and doubles any `'` in them. + Power Query hands the constant over unquoted, so a slicer value folded into + `CAST(hello world as VARCHAR)` and failed. Together with the `SQLGetTypeInfo` + fix above this makes DirectQuery slicers on text columns work, so the driver + and the connector have to be upgraded together. - `INTERVAL YEAR TO MONTH` and `INTERVAL DAY TO SECOND` columns read as text now return Trino's own rendering, the same text `CAST(... AS VARCHAR)` produces (`-1-0`, `0 00:00:00.500`). They were parsed into fields and re-rendered From a9f7617b309a3c2f461a64ea9d81d9430f5588b8 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 8 Oct 2026 16:27:40 +0200 Subject: [PATCH 10/20] test: run Windows suites with UTF-8 output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Over WinRM a suite's stdout is not a console, so Python on the VM encoded it as cp1252. The folding contract and Power BI slicer suites print non-Latin test values (日本語, emoji), so both died with UnicodeEncodeError part-way through and were reported as failed. The runner already decodes the output as UTF-8, so run_remote now sets PYTHONIOENCODING=utf-8 to match. Co-Authored-By: Claude Opus 5.5 (1M context) --- integration-tests/windows/windows_test.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/integration-tests/windows/windows_test.py b/integration-tests/windows/windows_test.py index b2ef9a6..854de19 100644 --- a/integration-tests/windows/windows_test.py +++ b/integration-tests/windows/windows_test.py @@ -563,9 +563,15 @@ def run_remote(session, script: str, argv) -> int: """Run one suite on the VM and return its exit code.""" args = " ".join(ps_quote(a) for a in argv) # Enable driver-side debug logging and DM tracing. + # + # PYTHONIOENCODING: over WinRM the suite's stdout is not a console, so + # Python would encode it as cp1252, which cannot represent the non-Latin + # test values some suites print (a UnicodeEncodeError ends the suite). The + # output is decoded as UTF-8 below, so it is encoded as UTF-8 here. r = session.run_ps( f'$env:ODBC_LOG_LEVEL = "debug"; ' f'$env:ODBC_LOG_FILE = "{REMOTE_LOG}"; ' + f'$env:PYTHONIOENCODING = "utf-8"; ' f'& {REMOTE_PYTHON} "{REMOTE_SUITES}\\{script}" {args}' ) stdout = r.std_out.decode("utf-8", errors="replace") From 44c1da7919fe5da96c1736712f452e6ab35721d1 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 8 Oct 2026 16:28:50 +0200 Subject: [PATCH 11/20] fix: deliver time with time zone in the session zone too Follow-ups from the whole-branch review of fix/review-feedback. TIME WITH TIME ZONE was still normalised to UTC while TIMESTAMP WITH TIME ZONE now follows the session zone, so with TimeZone=Europe/Berlin the two columns of one row disagreed, and the comment justifying UTC ("matches what TIMESTAMP WITH TIME ZONE already does") had become false. A time has no date, so a named session zone is resolved at its current offset, which is what Trino does when it casts TIME to TIME WITH TIME ZONE (+02:00 in Europe/Berlin on 2026-10-07, measured). An FFI test checks the delivered time against Trino's own cast in a UTC session, with TimeZone=Europe/Berlin and after SET TIME ZONE; it fails if the time is delivered in UTC. The connect probe now warns when current_timezone() returns a zone the driver cannot read, instead of silently falling back to UTC, as the SET TIME ZONE path already did. timestamp_with_named_tz_returns_utc_via_get_data is renamed timestamp_with_named_tz_returns_utc_in_a_utc_session, which is what it now tests. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 4 +- README.md | 2 +- src/backend.rs | 9 ++ src/ffi_integration_tests.rs | 84 +++++++++++++++++- src/type_conversion.rs | 159 ++++++++++++++++++++++++++++------- 5 files changed, 226 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8216de..a77435a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 folds the value it showed back as a plain `TIMESTAMP` literal, which Trino reads in the session zone, so a DirectQuery slicer on such a column only selected its rows in a UTC session. Applications reading these columns see - different values unless the session is UTC. + different values unless the session is UTC. `time with time zone` values + follow the same zone, at its current offset because a time has no date, which + is how Trino itself casts `TIME` to `TIME WITH TIME ZONE`. - The Power BI connector reports that Trino cannot convert `TIME` to `TIMESTAMP` (`SQL_CONVERT_TIME` without `SQL_CVT_TIMESTAMP`). A DirectQuery slicer on a time column folded to a comparison against Power BI's base date, diff --git a/README.md b/README.md index 1e7aac3..f76023e 100644 --- a/README.md +++ b/README.md @@ -148,7 +148,7 @@ The authoritative list is `src/backend/types/connect_params.rs`. | `Roles` | No | Authorisation role per catalog, `{catalog:role;catalog2:ALL}` | | `SessionUser` | No | User statements run as, while `User` still authenticates. JDBC's `sessionUser` | | `Path` | No | Default SQL path for resolving unqualified function names | -| `TimeZone` | No | IANA session time zone (`Europe/Berlin`). Unset leaves the coordinator's default. `timestamp with time zone` values are delivered as wall time in the session zone, which a later `SET TIME ZONE` changes | +| `TimeZone` | No | IANA session time zone (`Europe/Berlin`). Unset leaves the coordinator's default. `timestamp with time zone` and `time with time zone` values are delivered as wall time in the session zone, which a later `SET TIME ZONE` changes | | `Locale` | No | Locale for locale-dependent formatting, sent as `X-Trino-Language` | | `ClientInfo` | No | Free-form client metadata Trino records against the query | | `TraceToken` | No | Correlation token Trino records against the query | diff --git a/src/backend.rs b/src/backend.rs index ed127e3..0702fa0 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -662,6 +662,15 @@ fn probe_session(conn: &TrinoConnection) -> SessionProbe { if probe.user.is_none() { tracing::warn!("SELECT current_user returned no usable value"); } + if probe.time_zone.is_none() { + // Falls back to UTC, which shifts every timestamp-with-time-zone value + // by the server's offset, so it must not pass silently. + tracing::warn!( + current_timezone = column(2), + "SELECT current_timezone() returned no zone this driver can read; \ + delivering time-zone values in UTC unless TimeZone= is set" + ); + } let Some(raw) = column(0) else { tracing::warn!("SELECT version() returned no usable value"); diff --git a/src/ffi_integration_tests.rs b/src/ffi_integration_tests.rs index c22d93f..e170ea7 100644 --- a/src/ffi_integration_tests.rs +++ b/src/ffi_integration_tests.rs @@ -1823,7 +1823,7 @@ fn timestamp_with_tz_returns_wchar() { #[test] #[serial] #[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] -fn timestamp_with_named_tz_returns_utc_via_get_data() { +fn timestamp_with_named_tz_returns_utc_in_a_utc_session() { // UTC because the shared connection sets no `TimeZone=` and the test // stack's coordinator defaults to UTC: values are delivered in the session // zone (see `timestamptz_is_delivered_in_the_session_time_zone`). @@ -2014,6 +2014,88 @@ fn set_time_zone_moves_later_statements_into_the_new_zone() { } } +/// Assert that a `TIME WITH TIME ZONE` arrives as the time of day Trino itself +/// gives it in the session: cast back to `TIME WITH TIME ZONE` in that +/// session, the delivered time is the same instant. +unsafe fn assert_time_tz_delivered_in_session(conn: *mut c_void, label: &str) { + let value = "TIME '13:14:15+05:00'"; + unsafe { + let mut stmt: *mut c_void = std::ptr::null_mut(); + assert_eq!( + ffi::handle::sql_alloc_handle::(HandleType::Stmt as i16, conn, &mut stmt), + SqlReturn::SUCCESS + ); + assert_eq!( + exec_direct(stmt, &format!("SELECT {value}")), + SqlReturn::SUCCESS + ); + assert_eq!( + ffi::fetch::sql_fetch::(stmt), + SqlReturn::SUCCESS + ); + let mut t = odbc_sys::Time::default(); + let mut ind: isize = 0; + assert_eq!( + ffi::fetch::sql_get_data::( + stmt, + 1, + CDataType::TypeTime as i16, + (&raw mut t).cast(), + std::mem::size_of::() as isize, + &mut ind, + ), + SqlReturn::SUCCESS + ); + cleanup_stmt(stmt); + + let shown = format!("{:02}:{:02}:{:02}", t.hour, t.minute, t.second); + let mut stmt: *mut c_void = std::ptr::null_mut(); + assert_eq!( + ffi::handle::sql_alloc_handle::(HandleType::Stmt as i16, conn, &mut stmt), + SqlReturn::SUCCESS + ); + let sql = format!( + "SELECT CAST({value} = CAST(TIME '{shown}' AS TIME WITH TIME ZONE) AS VARCHAR), \ + current_timezone()" + ); + assert_eq!(exec_direct(stmt, &sql), SqlReturn::SUCCESS, "{sql}"); + assert_eq!( + ffi::fetch::sql_fetch::(stmt), + SqlReturn::SUCCESS + ); + let same = get_wchar_col(stmt, 1); + let zone = get_wchar_col(stmt, 2); + assert_eq!( + same, "true", + "{label}: delivered {shown} in session zone {zone}" + ); + cleanup_stmt(stmt); + } +} + +/// `TIME WITH TIME ZONE` follows the session zone as `TIMESTAMP WITH TIME +/// ZONE` does, including after `SET TIME ZONE`. +#[test] +#[serial] +#[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] +fn time_with_tz_is_delivered_in_the_session_time_zone() { + for extra in ["", ";TimeZone=Europe/Berlin"] { + unsafe { + let (env, conn, stmt) = alloc_handles(); + assert_eq!( + connect_trino_with(conn, extra), + SqlReturn::SUCCESS, + "{extra}: {}", + conn_diag_message(conn) + ); + assert_time_tz_delivered_in_session(conn, extra); + run_on(conn, "SET TIME ZONE '+05:30'"); + assert_time_tz_delivered_in_session(conn, "after +05:30"); + cleanup(env, conn, stmt); + } + } +} + #[test] #[serial] #[ignore = "requires Trino at localhost:8443; run ./integration-tests/setup.sh first"] diff --git a/src/type_conversion.rs b/src/type_conversion.rs index f2c9b15..74f1775 100644 --- a/src/type_conversion.rs +++ b/src/type_conversion.rs @@ -183,8 +183,8 @@ impl TrinoTypeName { // DEFAULT_TEMPORAL_SCALE_WITHOUT_TYPE_NAME, Trino's actual // default declared precision). TimeWithTimeZone is also 12, not // HH:MM:SS.mmm+HH:MM (18): `parse_trino_time_with_tz` applies the - // offset and normalises to UTC (matching TIMESTAMP WITH TIME - // ZONE), so the offset never survives into SQL_TIME_STRUCT; + // offset and converts to the session time zone (matching TIMESTAMP + // WITH TIME ZONE), so the offset never survives into SQL_TIME_STRUCT; // only the fractional seconds do (preserved via `ColumnValue:: // Time`'s `fraction` field, delivered through SQL_C_CHAR/WCHAR // text conversions). Keep the two as distinct arms, matching @@ -204,8 +204,8 @@ impl TrinoTypeName { // YYYY-MM-DD HH:MM:SS.mmm = 23 chars (20 + scale 3, see // DEFAULT_TEMPORAL_SCALE_WITHOUT_TYPE_NAME). TimestampWithTimeZone // is also 23, not YYYY-MM-DD HH:MM:SS.mmm+HH:MM (29): - // `parse_trino_timestamp_tz` applies the offset and normalises to - // UTC, so it doesn't survive into SQL_TIMESTAMP_STRUCT (which has + // `parse_trino_timestamp_tz` applies the offset and converts to + // the session time zone, so it doesn't survive into SQL_TIMESTAMP_STRUCT (which has // no zone field either); only YYYY-MM-DD HH:MM:SS.mmm is // delivered. Kept as a distinct match arm (not merged with // `Timestamp`) to match `trino_ty_precision`, for the same reason @@ -428,7 +428,8 @@ pub fn trino_ty_precision(ty: &TrinoTy) -> u32 { DEFAULT_TEMPORAL_SCALE_WITHOUT_TYPE_NAME, )), // Also HH:MM:SS.mmm, not HH:MM:SS.mmm+HH:MM: the offset is applied - // and the value normalised to UTC (see `parse_trino_time_with_tz`), + // and the value converted to the session zone (see + // `parse_trino_time_with_tz`), // so only the fractional seconds reach the application, through the // text conversions. SQL_TIME_STRUCT has no fraction field of its own. TrinoTy::TimeWithTimeZone => precision_as_u32(column_size( @@ -444,7 +445,8 @@ pub fn trino_ty_precision(ty: &TrinoTy) -> u32 { DEFAULT_TEMPORAL_SCALE_WITHOUT_TYPE_NAME, )), // Also YYYY-MM-DD HH:MM:SS.mmm, not ...+HH:MM: the offset is applied - // and the value normalised to UTC (see `parse_trino_timestamp_tz`), + // and the value converted to the session zone (see + // `parse_trino_timestamp_tz`), // so only YYYY-MM-DD HH:MM:SS.mmm reaches SQL_TIMESTAMP_STRUCT. TrinoTy::TimestampWithTimeZone => precision_as_u32(column_size( SqlDataType::TIMESTAMP, @@ -781,12 +783,14 @@ fn parse_trino_time(s: &str) -> Option { }) } -/// Parse `TIME WITH TIME ZONE` and normalise to UTC. +/// Parse `TIME WITH TIME ZONE` and deliver it as the time of day at +/// `session_offset_minutes`, the session zone's offset (see +/// [`SessionZone::offset_minutes_at`]). /// /// `SQL_TIME_STRUCT` has no timezone field, so the offset cannot be carried. -/// Applying it and returning UTC matches what `TIMESTAMP WITH TIME ZONE` -/// already does; do not discard the offset silently, or the two "with time -/// zone" types behave inconsistently. +/// The value's own offset fixes the instant and the session's decides how it +/// is shown, as for `TIMESTAMP WITH TIME ZONE`; do not discard the offset +/// silently, or the two "with time zone" types behave inconsistently. /// /// Trino renders the zone two ways: a space-separated name (`"13:14:15.000 /// UTC"`) or a glued numeric offset (`"13:14:15+02:00"`, `"13:14:15-05:30"`). @@ -796,7 +800,7 @@ fn parse_trino_time(s: &str) -> Option { /// only affects the rare case of a non-UTC named zone, since Trino's own /// numeric-offset rendering is the common form. /// Returns `None` if the string is malformed. -fn parse_trino_time_with_tz(s: &str) -> Option { +fn parse_trino_time_with_tz(s: &str, session_offset_minutes: i32) -> Option { let t = s.trim(); // A space-separated named zone, e.g. "13:14:15.000 UTC". @@ -813,7 +817,7 @@ fn parse_trino_time_with_tz(s: &str) -> Option { ); 0 }; - return shift_time(time_part, offset_minutes); + return shift_time(time_part, offset_minutes - session_offset_minutes); } // A glued numeric offset, e.g. "13:14:15+02:00" or "13:14:15-05:30". @@ -832,10 +836,13 @@ fn parse_trino_time_with_tz(s: &str) -> Option { // `None` keeps the value as text, which is what every other unparseable // temporal string here already does. let offset_minutes = oh.checked_mul(60)?.checked_add(om)?.checked_mul(sign)?; - shift_time(time_part, offset_minutes) + shift_time( + time_part, + offset_minutes.checked_sub(session_offset_minutes)?, + ) } -/// Shift `HH:MM:SS[.f]` by `offset_minutes`, wrapping within the day. +/// Shift `HH:MM:SS[.f]` back by `offset_minutes`, wrapping within the day. /// /// A `TIME` has no date to carry an overflow into, so the result wraps /// within `[0, 24h)`. `rem_euclid` (not `%`) is used because a negative @@ -956,6 +963,21 @@ impl SessionZone { chrono::FixedOffset::east_opt(sign * (h * 3600 + m * 60)).map(SessionZone::Fixed) } + /// This zone's offset from UTC, in minutes, at the instant `utc`. + /// + /// A `TIME WITH TIME ZONE` has no date, so a named zone is resolved at the + /// current instant, as Trino resolves one when it casts `TIME` to + /// `TIME WITH TIME ZONE` in the session zone. + pub fn offset_minutes_at(self, utc: chrono::NaiveDateTime) -> i32 { + use chrono::Offset as _; + use chrono::TimeZone as _; + let seconds = match self { + SessionZone::Named(tz) => tz.offset_from_utc_datetime(&utc).fix().local_minus_utc(), + SessionZone::Fixed(offset) => offset.local_minus_utc(), + }; + seconds / 60 + } + /// The wall time in this zone at the instant `utc`. /// /// Unambiguous: every instant has exactly one wall time, including the two @@ -969,6 +991,23 @@ impl SessionZone { } } +/// The current instant, in UTC. +/// +/// From `SystemTime` because this crate builds chrono without its `clock` +/// feature. A clock before 1970 reads as the epoch, which only affects which +/// side of a DST change a named zone's offset is taken from. +fn utc_now() -> chrono::NaiveDateTime { + let since_epoch = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default(); + chrono::DateTime::from_timestamp( + i64::try_from(since_epoch.as_secs()).unwrap_or(0), + since_epoch.subsec_nanos(), + ) + .unwrap_or_default() + .naive_utc() +} + /// Parse a Trino TIMESTAMP WITH TIME ZONE string and deliver it as wall time /// in the session time zone. /// @@ -1194,16 +1233,18 @@ pub fn json_to_column_value(val: Value, ty: &TrinoTy, session: SessionZone) -> C ColumnValue::String(val.to_string()) } } - // TIME WITH TIME ZONE: parse and normalise to UTC, mirroring - // TIMESTAMP WITH TIME ZONE. See `parse_trino_time_with_tz` for why + // TIME WITH TIME ZONE: parse and convert to the session zone's + // current offset, mirroring TIMESTAMP WITH TIME ZONE. See `parse_trino_time_with_tz` for why // this must not share `parse_trino_time`, which silently discards // the offset instead of applying it. TrinoTy::TimeWithTimeZone => { if let Value::String(ref s) = val { - parse_trino_time_with_tz(s).unwrap_or_else(|| { - tracing::warn!(raw = s, "failed to parse Trino TIME WITH TIME ZONE string"); - ColumnValue::String(s.clone()) - }) + parse_trino_time_with_tz(s, session.offset_minutes_at(utc_now())).unwrap_or_else( + || { + tracing::warn!(raw = s, "failed to parse Trino TIME WITH TIME ZONE string"); + ColumnValue::String(s.clone()) + }, + ) } else { ColumnValue::String(val.to_string()) } @@ -1352,6 +1393,14 @@ mod tests { } } + fn convert_in(val: Value, ty: &TrinoTy, zone: &str) -> ColumnValue { + json_to_column_value( + val, + ty, + SessionZone::parse(zone).expect("a zone Trino reports"), + ) + } + fn tz_in(raw: &str, zone: &str) -> ColumnValue { json_to_column_value( Value::String(raw.into()), @@ -1427,6 +1476,58 @@ mod tests { } } + /// A `TIME WITH TIME ZONE` is delivered as the time of day in the + /// session's offset, as a `TIMESTAMP WITH TIME ZONE` is; the value's own + /// offset only fixes the instant. + #[test] + fn time_with_tz_is_delivered_in_the_session_offset() { + let time = |hour, minute, second| ColumnValue::Time { + hour, + minute, + second, + fraction: 0, + }; + assert_eq!( + parse_trino_time_with_tz("13:14:15+02:00", 120), + Some(time(13, 14, 15)) + ); + assert_eq!( + parse_trino_time_with_tz("13:14:15+00:00", -300), + Some(time(8, 14, 15)) + ); + // Wraps within the day, as the UTC normalisation already did. + assert_eq!( + parse_trino_time_with_tz("23:30:00+00:00", 120), + Some(time(1, 30, 0)) + ); + assert_eq!( + convert_in( + Value::String("13:14:15+00:00".into()), + &TrinoTy::TimeWithTimeZone, + "+05:30" + ), + time(18, 44, 15) + ); + } + + /// A time has no date, so a named session zone is resolved at the current + /// instant, which is what Trino does when it casts `TIME` to + /// `TIME WITH TIME ZONE` (`+02:00` in Europe/Berlin on 2026-10-07). + #[test] + fn a_session_zones_offset_depends_on_the_day_it_is_read() { + let at = |y, m, d| { + chrono::NaiveDate::from_ymd_opt(y, m, d) + .and_then(|d| d.and_hms_opt(12, 0, 0)) + .expect("a valid date") + }; + let berlin = SessionZone::parse("Europe/Berlin").expect("a zone"); + assert_eq!(berlin.offset_minutes_at(at(2026, 1, 15)), 60); + assert_eq!(berlin.offset_minutes_at(at(2026, 7, 15)), 120); + let fixed = SessionZone::parse("+05:30").expect("a zone"); + assert_eq!(fixed.offset_minutes_at(at(2026, 1, 15)), 330); + assert_eq!(SessionZone::UTC.offset_minutes_at(at(2026, 7, 15)), 0); + } + #[test] fn session_zone_parses_what_current_timezone_reports() { for name in [ @@ -2366,7 +2467,7 @@ mod tests { // The two "with time zone" types must agree: TIMESTAMP WITH TIME // ZONE converts to UTC, so discarding the offset here rather than // applying it would make TIME WITH TIME ZONE contradict it. - let val = parse_trino_time_with_tz("13:14:15+02:00").expect("parses"); + let val = parse_trino_time_with_tz("13:14:15+02:00", 0).expect("parses"); assert_eq!( val, ColumnValue::Time { @@ -2380,7 +2481,7 @@ mod tests { #[test] fn time_with_negative_offset_normalises_to_utc() { - let val = parse_trino_time_with_tz("13:14:15-05:30").expect("parses"); + let val = parse_trino_time_with_tz("13:14:15-05:30", 0).expect("parses"); assert_eq!( val, ColumnValue::Time { @@ -2394,7 +2495,7 @@ mod tests { #[test] fn time_with_offset_wraps_across_midnight() { - let val = parse_trino_time_with_tz("01:00:00+02:00").expect("parses"); + let val = parse_trino_time_with_tz("01:00:00+02:00", 0).expect("parses"); assert_eq!( val, ColumnValue::Time { @@ -2408,7 +2509,7 @@ mod tests { #[test] fn time_with_utc_offset_is_unchanged() { - let val = parse_trino_time_with_tz("13:14:15.000 UTC").expect("parses"); + let val = parse_trino_time_with_tz("13:14:15.000 UTC", 0).expect("parses"); assert_eq!( val, ColumnValue::Time { @@ -2424,7 +2525,7 @@ mod tests { fn time_with_timezone_keeps_fraction_through_offset_shift() { // The offset shift only touches whole minutes, so a fractional-seconds // part must survive `shift_time` unchanged. - let val = parse_trino_time_with_tz("13:14:15.123456+02:00").expect("parses"); + let val = parse_trino_time_with_tz("13:14:15.123456+02:00", 0).expect("parses"); assert_eq!( val, ColumnValue::Time { @@ -2443,7 +2544,7 @@ mod tests { // `i32` and only overflows when converted to minutes. Release builds // carry no overflow checks, so an unchecked multiply here reports a // *different* time instead of declining the value. - assert_eq!(parse_trino_time_with_tz("00:00:00+949378864"), None); + assert_eq!(parse_trino_time_with_tz("00:00:00+949378864", 0), None); assert_eq!( convert( Value::String("+999\0\0\0\0+00949378864".into()), @@ -2458,7 +2559,7 @@ mod tests { // The same class one frame down, in `shift_time`: the time-of-day hour // is text too, so `hour * 60` overflows before the offset is ever // applied. - assert_eq!(parse_trino_time_with_tz("2147483647:00:00+01:00"), None); + assert_eq!(parse_trino_time_with_tz("2147483647:00:00+01:00", 0), None); } #[test] @@ -2466,7 +2567,7 @@ mod tests { // The checked arithmetic must not narrow what a valid offset can be: // the widest zones in use are +14:00 and -12:00. assert_eq!( - parse_trino_time_with_tz("13:14:15+14:00"), + parse_trino_time_with_tz("13:14:15+14:00", 0), Some(ColumnValue::Time { hour: 23, minute: 14, @@ -2475,7 +2576,7 @@ mod tests { }) ); assert_eq!( - parse_trino_time_with_tz("13:14:15-12:00"), + parse_trino_time_with_tz("13:14:15-12:00", 0), Some(ColumnValue::Time { hour: 1, minute: 14, From fe0e43f29513040d790463803802caea3b546b8b Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 8 Oct 2026 16:29:02 +0200 Subject: [PATCH 12/20] chore: pin stackable-odbc-core to 2f9f725 Picks up the core fix that reports interval text with an oversized leading field as 22015 instead of wrapping it into a small value reported as success, and bounds the month field to 0-11. Still a temporary commit pin until a core release containing the branch's changes is tagged. Co-Authored-By: Claude Opus 5.5 (1M context) --- Cargo.lock | 2 +- Cargo.toml | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 60edd05..3c55b0a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1971,7 +1971,7 @@ checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" [[package]] name = "stackable-odbc-core" version = "0.1.0" -source = "git+https://github.com/stackabletech/stackable-odbc-core.git?rev=b37ca99433b15b2646e98594a6571719acc2a8fa#b37ca99433b15b2646e98594a6571719acc2a8fa" +source = "git+https://github.com/stackabletech/stackable-odbc-core.git?rev=2f9f7252e82de439d003b743a09001527f250b75#2f9f7252e82de439d003b743a09001527f250b75" dependencies = [ "odbc-sys", "snafu", diff --git a/Cargo.toml b/Cargo.toml index 8cee15a..8fa7eb9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,11 +38,11 @@ snafu = "0.9" # reviewable edit to this line instead of whatever the branch happens to point # at. To build against a local checkout, use a `[patch]` in your own # `.cargo/config.toml` rather than editing this line; see CONTRIBUTING.md. -# TEMPORARY: pinned to the core commit converting character data to the -# SQL_C_INTERVAL_* C types (after `TypeInfoRow::with_preferred`) until a core -# release containing both is tagged; switch both entries back to -# that tag before merging. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "b37ca99433b15b2646e98594a6571719acc2a8fa" } +# TEMPORARY: pinned to the core commit reporting an oversized interval text +# field as 22015 (after the character-to-SQL_C_INTERVAL_* conversion and +# `TypeInfoRow::with_preferred`) until a core release containing them is tagged; +# switch both entries back to that tag before merging. +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "2f9f7252e82de439d003b743a09001527f250b75" } # `time` is needed directly by `query_all_rows_within`, which bounds the login # round trip with `tokio::time::timeout`. It resolves without being declared, # because reqwest enables it, but a direct use must not rely on another crate's @@ -64,7 +64,7 @@ serial_test = "4" # offline FFI tests need. It is default-off because it is test code, so it is # enabled here rather than on the [dependencies] entry above -- that keeps it # out of the shipped cdylib. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "b37ca99433b15b2646e98594a6571719acc2a8fa", features = ["test-support"] } +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "2f9f7252e82de439d003b743a09001527f250b75", features = ["test-support"] } [lints.clippy] unwrap_in_result = "deny" From 6e8377194002d52d4858d643d608d2cbd95e599c Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 9 Oct 2026 10:14:20 +0200 Subject: [PATCH 13/20] test: pull MinIO from pgsty instead of quay.io The spooling CI leg failed before any test ran: pulls of quay.io/minio/minio and quay.io/minio/mc were refused with "unauthorized: access to the requested resource is not authorized". Switch both to pgsty's builds, as stackabletech/demos already does (stacks/airflow/minio.yaml): docker.io/pgsty/minio:RELEASE.2026-08-04T00-00-00Z and docker.io/pgsty/mc:RELEASE.2026-09-16T00-00-00Z. Both carry /bin/sh, which minio-init's retry loop needs. Verified locally with setup.sh --profile spooling: the bucket is created and the spooling suite passes (10 passed, 1 skipped by design). The workflow comment that named quay.io as the source of dropped connections now describes the spooling leg without it. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/build.yaml | 4 ++-- integration-tests/stack/compose.yaml | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index eb46908..7fdde3e 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -310,8 +310,8 @@ jobs: # per layer: once a single layer exhausts it, the pull fails and takes # every other image with it. Layers that did arrive stay in the local # content store, so a second attempt refetches only what is missing and - # costs seconds. The spooling leg is the one that reaches quay.io for - # MinIO, and that is where the dropped connections have been seen. + # costs seconds. The spooling leg pulls the most, MinIO on top of the + # core images, so it is the one most exposed to a dropped connection. - name: Pull the stack images # From the stack directory, as scripts/lib.sh's `compose` wrapper does. # Compose derives the project name from the compose file's directory, diff --git a/integration-tests/stack/compose.yaml b/integration-tests/stack/compose.yaml index d7a1c2a..c57ff09 100644 --- a/integration-tests/stack/compose.yaml +++ b/integration-tests/stack/compose.yaml @@ -83,7 +83,10 @@ services: minio: profiles: [spooling] - image: quay.io/minio/minio:RELEASE.2025-04-22T22-12-26Z + # pgsty's build of MinIO, as stackabletech/demos uses: pulls of + # quay.io/minio were refused ("unauthorized") in CI on 2026-10-09. pgsty also + # publishes `mc` (below), and both images carry /bin/sh. + image: docker.io/pgsty/minio:RELEASE.2026-08-04T00-00-00Z command: ["server", "/data"] environment: # Interpolated from scripts/lib.sh, which is the single source; the @@ -98,7 +101,7 @@ services: minio-init: profiles: [spooling] - image: quay.io/minio/mc:RELEASE.2025-04-16T18-13-26Z + image: docker.io/pgsty/mc:RELEASE.2026-09-16T00-00-00Z depends_on: minio: condition: service_started From 7c2722da157fda772d41ba6228eb918cd9961040 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 9 Oct 2026 10:42:30 +0200 Subject: [PATCH 14/20] test: listen on a fixed port in the HYT00 timeout test a_timeout_names_the_cause_beneath_reqwest bound 127.0.0.1:0 and then connected to whatever port the OS chose. A sandbox that allows loopback ports one by one, as Landlock does, can list a fixed port but not a random one, so the test could not pass in such a sandbox even with binding allowed. It now listens on 127.0.0.1:29871: below Linux's ephemeral range, so an outgoing connection does not take it by chance, and bound by this helper only. A failed bind names the port and says it may be in use or not allowed by the sandbox. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/backend.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/backend.rs b/src/backend.rs index 0702fa0..dc154ed 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -2823,13 +2823,21 @@ mod tests { /// when the client's own timeout fires. A refused port would produce a /// connect error instead, and no coordinator is needed to make a request /// take longer than it is allowed to. + /// + /// The port is fixed rather than OS-assigned (`:0`): a sandbox that allows + /// loopback ports one by one, as Landlock does, can list a fixed port, but + /// not the random one this test would then connect to. It sits below + /// Linux's ephemeral range (32768 and up), so an outgoing connection does + /// not take it by chance. Only this helper binds it. fn timeout_failure() -> trino_rust_client::error::Error { - let listener = - std::net::TcpListener::bind("127.0.0.1:0").expect("the loopback interface is bindable"); - let port = listener - .local_addr() - .expect("a bound listener has an address") - .port(); + const PORT: u16 = 29871; + let listener = std::net::TcpListener::bind(("127.0.0.1", PORT)).unwrap_or_else(|e| { + panic!( + "this test listens on 127.0.0.1:{PORT}, a fixed port so a sandbox can allow it; \ + binding it failed (in use, or not allowed by the sandbox): {e}" + ) + }); + let port = PORT; // Holds the accepted connection open, which is what makes the request // time out rather than fail. Detached, and outlived by the test. std::thread::spawn(move || { From e7fab7754693ee5d6cb6670bfede63377dd968b2 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 9 Oct 2026 11:09:49 +0200 Subject: [PATCH 15/20] chore: pin stackable-odbc-core to 53e5a89 Picks up core's exact numeric -> SQL_C_INTERVAL_* conversion and the 01S07 for a non-zero interval-text digit past the ninth. No driver code changes. Still a temporary commit pin until a core release is tagged; the comment now names the branch head rather than one commit's subject, so it does not go stale with the next bump. Co-Authored-By: Claude Opus 5.5 (1M context) --- Cargo.lock | 2 +- Cargo.toml | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3c55b0a..3ddc804 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1971,7 +1971,7 @@ checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" [[package]] name = "stackable-odbc-core" version = "0.1.0" -source = "git+https://github.com/stackabletech/stackable-odbc-core.git?rev=2f9f7252e82de439d003b743a09001527f250b75#2f9f7252e82de439d003b743a09001527f250b75" +source = "git+https://github.com/stackabletech/stackable-odbc-core.git?rev=53e5a89851fcfeeae404b9ab0ca461f149a0ff2b#53e5a89851fcfeeae404b9ab0ca461f149a0ff2b" dependencies = [ "odbc-sys", "snafu", diff --git a/Cargo.toml b/Cargo.toml index 8fa7eb9..2152b75 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,11 +38,11 @@ snafu = "0.9" # reviewable edit to this line instead of whatever the branch happens to point # at. To build against a local checkout, use a `[patch]` in your own # `.cargo/config.toml` rather than editing this line; see CONTRIBUTING.md. -# TEMPORARY: pinned to the core commit reporting an oversized interval text -# field as 22015 (after the character-to-SQL_C_INTERVAL_* conversion and -# `TypeInfoRow::with_preferred`) until a core release containing them is tagged; -# switch both entries back to that tag before merging. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "2f9f7252e82de439d003b743a09001527f250b75" } +# TEMPORARY: pinned to the head of core's fix/review-feedback branch (the +# preferred SQLGetTypeInfo row and the interval conversions) until a core +# release containing it is tagged; switch both entries back to that tag before +# merging. +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "53e5a89851fcfeeae404b9ab0ca461f149a0ff2b" } # `time` is needed directly by `query_all_rows_within`, which bounds the login # round trip with `tokio::time::timeout`. It resolves without being declared, # because reqwest enables it, but a direct use must not rely on another crate's @@ -64,7 +64,7 @@ serial_test = "4" # offline FFI tests need. It is default-off because it is test code, so it is # enabled here rather than on the [dependencies] entry above -- that keeps it # out of the shipped cdylib. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "2f9f7252e82de439d003b743a09001527f250b75", features = ["test-support"] } +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "53e5a89851fcfeeae404b9ab0ca461f149a0ff2b", features = ["test-support"] } [lints.clippy] unwrap_in_result = "deny" From e1b3c9194135ad6d53d1f98948258e0a1be5e240 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 9 Oct 2026 12:31:25 +0200 Subject: [PATCH 16/20] chore: pin stackable-odbc-core to the v0.1.1 release Replaces the temporary commit pin with the released tag. v0.1.1 is the squash-merged core work this branch was tested against (53e5a89 plus the version bump and changelog), so the dependency's code is unchanged; the lockfile records core 0.1.1 at tag v0.1.1 (4f5e802). Co-Authored-By: Claude Opus 5.5 (1M context) --- Cargo.lock | 4 ++-- Cargo.toml | 8 ++------ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3ddc804..fbff71c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1970,8 +1970,8 @@ checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" [[package]] name = "stackable-odbc-core" -version = "0.1.0" -source = "git+https://github.com/stackabletech/stackable-odbc-core.git?rev=53e5a89851fcfeeae404b9ab0ca461f149a0ff2b#53e5a89851fcfeeae404b9ab0ca461f149a0ff2b" +version = "0.1.1" +source = "git+https://github.com/stackabletech/stackable-odbc-core.git?tag=v0.1.1#4f5e802cf0cded7c75dba8afa905864b9f355302" dependencies = [ "odbc-sys", "snafu", diff --git a/Cargo.toml b/Cargo.toml index 2152b75..d48901c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,11 +38,7 @@ snafu = "0.9" # reviewable edit to this line instead of whatever the branch happens to point # at. To build against a local checkout, use a `[patch]` in your own # `.cargo/config.toml` rather than editing this line; see CONTRIBUTING.md. -# TEMPORARY: pinned to the head of core's fix/review-feedback branch (the -# preferred SQLGetTypeInfo row and the interval conversions) until a core -# release containing it is tagged; switch both entries back to that tag before -# merging. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "53e5a89851fcfeeae404b9ab0ca461f149a0ff2b" } +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", tag = "v0.1.1" } # `time` is needed directly by `query_all_rows_within`, which bounds the login # round trip with `tokio::time::timeout`. It resolves without being declared, # because reqwest enables it, but a direct use must not rely on another crate's @@ -64,7 +60,7 @@ serial_test = "4" # offline FFI tests need. It is default-off because it is test code, so it is # enabled here rather than on the [dependencies] entry above -- that keeps it # out of the shipped cdylib. -stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", rev = "53e5a89851fcfeeae404b9ab0ca461f149a0ff2b", features = ["test-support"] } +stackable-odbc-core = { git = "https://github.com/stackabletech/stackable-odbc-core.git", tag = "v0.1.1", features = ["test-support"] } [lints.clippy] unwrap_in_result = "deny" From a466d0b33cdac03d71189ae7c5a6a49631e92471 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 9 Oct 2026 12:56:06 +0200 Subject: [PATCH 17/20] test: refuse to run the Windows suites with ODBC tracing on ODBC Driver Manager tracing writes every call to a file. On the test VM 2,000 rows of tpcds.sf1.customer took 1.3-1.7 s with it off and 42-172 s with it on, slow enough for Trino to abandon a long read (ABANDONED_QUERY). Left on after a diagnosis, it failed the spooling suite and passed for a driver problem. windows_test.py now checks the three places Windows keeps the switch (current user, machine, 32-bit Driver Manager) right after connecting and stops when any is on, naming the key and its trace file. --allow-dm-trace continues with a warning, for runs where tracing is the point. WINDOWS.md documents both; the README's troubleshooting section explains ABANDONED_QUERY / "Query not found" on large reads and tracing as a measured cause. Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 10 +++++ integration-tests/windows/WINDOWS.md | 11 +++++ integration-tests/windows/windows_test.py | 54 +++++++++++++++++++++++ 3 files changed, 75 insertions(+) diff --git a/README.md b/README.md index f76023e..1e0294e 100644 --- a/README.md +++ b/README.md @@ -326,6 +326,16 @@ the hour that repeats, two instants share one wall time. Power BI filters on the wall time it showed and Trino reads that as the later of the two instants, so a value from the first of the repeated hours is not selected. +**Large reads fail with `ABANDONED_QUERY` or `Query not found`.** Trino +abandons a query whose results the client has not fetched within +`query.client.timeout` (5 minutes by default), and later forgets it altogether, +after which the next fetch returns `404 Not Found: Query not found`. A client +that reads slowly enough gets there on a large result. One cause we have +measured on Windows is ODBC tracing left switched on: it writes every call to a +file, and made a read 25 to 130 times slower. Turn it off in the ODBC Data Source +Administrator (Tracing tab, **Stop Tracing Now**), including on an on-premises +data gateway. + **The browser login never opens.** Some tools, `pyodbc` among them, tell the driver it may not display anything. The driver reports this rather than hanging. Use `AccessToken` with those tools, or connect through one that allows a prompt. diff --git a/integration-tests/windows/WINDOWS.md b/integration-tests/windows/WINDOWS.md index 6848f63..6ac23c4 100644 --- a/integration-tests/windows/WINDOWS.md +++ b/integration-tests/windows/WINDOWS.md @@ -119,6 +119,17 @@ which can predate the feature under test by days. | `--gateway ` | `$ODBC_TEST_HOST_GATEWAY`, else `192.168.197.1` | The host-only gateway the VM reaches the host on. `scripts/gen-certs.sh` reads the same environment variable, so the coordinator's certificate covers the address the VM connects to | | `--trino-host
` | the same as `--gateway` | Where the VM reaches Trino. The name `trino` is mapped to it in the VM's hosts file | | `--suite ` | unset | Run only the suites whose name contains the substring. `run-tests.sh --suite` forwards to this | +| `--allow-dm-trace` | off | Run even when ODBC Driver Manager tracing is on in the VM. Without it the run stops; see below | + +**Driver Manager tracing must be off.** The harness checks the three places +Windows keeps the switch (the current user's, the machine's and the 32-bit +Driver Manager's `ODBC.INI\ODBC` key) and stops if any has `Trace=1`. Tracing +writes every ODBC call to a file: on this VM, 2,000 rows took 1.3-1.7 s with it +off and 42-172 s with it on, slow enough for Trino to abandon a long read +(`ABANDONED_QUERY`). It is easy to leave on after a diagnosis, and the result +looks like a driver problem. Turn it off in the ODBC Data Source Administrator +(Tracing tab, **Stop Tracing Now**) or set `Trace` to `0` under the key the +error names. `--allow-dm-trace` is for a run where tracing is the point. The verified configurations connect to `trino` rather than to an address. TLS sends no SNI for an IP literal, and Jetty then serves Trino's internal diff --git a/integration-tests/windows/windows_test.py b/integration-tests/windows/windows_test.py index 854de19..94172d2 100644 --- a/integration-tests/windows/windows_test.py +++ b/integration-tests/windows/windows_test.py @@ -127,6 +127,8 @@ def main(): # Wait for VM setup to complete (Python, pyodbc installed). wait_for_setup(session) + check_dm_tracing(session, args.allow_dm_trace) + deploy(session, args.gateway, dll_path) # Before `register_driver`, because the uninstaller it runs deregisters the @@ -419,6 +421,12 @@ def parse_args(): default="", help="only run suites whose name contains this string", ) + p.add_argument( + "--allow-dm-trace", + action="store_true", + help="run even when ODBC Driver Manager tracing is on in the VM " + "(it slows every ODBC call; for deliberate diagnosis only)", + ) return p.parse_args() @@ -655,6 +663,52 @@ def _port_available(port: int) -> bool: return False +# Where the Driver Manager reads its tracing switch: the current user (the +# ODBC Data Source Administrator's Tracing tab), the machine, and the 32-bit +# Driver Manager's own copy. +DM_TRACE_KEYS = ( + r"HKCU:\Software\ODBC\ODBC.INI\ODBC", + r"HKLM:\SOFTWARE\ODBC\ODBC.INI\ODBC", + r"HKLM:\SOFTWARE\WOW6432Node\ODBC\ODBC.INI\ODBC", +) + + +def check_dm_tracing(session, allowed: bool): + """Refuse to run while ODBC Driver Manager tracing is on in the VM. + + Tracing writes every ODBC call to a file. Measured on this VM (2026-10-09): + 2,000 rows of tpcds.sf1.customer took 1.3-1.7 s with tracing off and + 42-172 s with it on. That is slow enough for Trino to abandon a long read + (ABANDONED_QUERY), which failed the spooling suite and was taken for a + driver problem for a day. Tracing is easy to leave on after a diagnosis, + and nothing else here would say so. + """ + keys = ", ".join(f"'{k}'" for k in DM_TRACE_KEYS) + r = session.run_ps( + f"foreach ($k in {keys}) {{ " + f"$p = Get-ItemProperty $k -ErrorAction SilentlyContinue; " + f"if ($p -and \"$($p.Trace)\" -eq '1') {{ \"$k -> $($p.TraceFile)\" }} }}" + ) + on = [line for line in r.std_out.decode("utf-8", errors="replace").splitlines() if line.strip()] + if not on: + return + message = "ODBC Driver Manager tracing is ON in the VM (Trace=1):\n" + "\n".join( + f" {line}" for line in on + ) + if allowed: + print(f"WARNING: {message}\n continuing because of --allow-dm-trace; " + "expect every suite to run far slower", file=sys.stderr) + return + print( + f"ERROR: {message}\n" + "Tracing slows every ODBC call enough for Trino to abandon long reads.\n" + "Turn it off (ODBC Data Source Administrator > Tracing > Stop Tracing Now, " + "or set Trace to 0 under the key above), or pass --allow-dm-trace.", + file=sys.stderr, + ) + sys.exit(1) + + def check_installers(session): """Run the shipped install.bat and uninstall.bat, and check what they left. From e26349ed9b17156383642832ecd85c97356d42e5 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 9 Oct 2026 12:56:23 +0200 Subject: [PATCH 18/20] test: keep Trino's config visible and current when setup.sh re-runs gen-trino-config.sh removed generated/trino and created it again. A running coordinator bind-mounts that directory, so after a re-run the container held the deleted one: /etc/trino was empty, Trino kept the settings it had read at its last start, and a restart would have found no config at all. setup.sh only recreated Trino when the profiles changed, so a changed fragment did not reach a running stack either. The generator now empties the directory in place, and setup.sh exports a hash of the assembled config that compose.yaml sets as a label on the trino service: a changed config is a changed service definition, which compose recreates. An unchanged re-run leaves Trino running. Checked against the live stack: after a re-run /etc/trino keeps its 6 entries (was empty); a fragment change recreates Trino (was not); an unchanged re-run keeps the container; switching to the spooling profile and the full integration run still pass. Co-Authored-By: Claude Opus 5.5 (1M context) --- integration-tests/scripts/gen-trino-config.sh | 7 ++++++- integration-tests/scripts/setup.sh | 6 ++++++ integration-tests/stack/compose.yaml | 4 ++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/integration-tests/scripts/gen-trino-config.sh b/integration-tests/scripts/gen-trino-config.sh index 9681822..2a6428e 100755 --- a/integration-tests/scripts/gen-trino-config.sh +++ b/integration-tests/scripts/gen-trino-config.sh @@ -19,7 +19,12 @@ set -euo pipefail source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/lib.sh" OUT="$GENERATED/trino" -rm -rf "$OUT" +# Emptied in place, never removed: a running coordinator bind-mounts this +# directory, and replacing it would leave the container holding the deleted +# one, empty, so a restart would find no config at all. setup.sh recreates +# Trino when the assembled content changes (TRINO_CONFIG_HASH). +mkdir -p "$OUT" +find "$OUT" -mindepth 1 -delete mkdir -p "$OUT/catalog" cp -r "$STACK_DIR/trino/base/." "$OUT/" diff --git a/integration-tests/scripts/setup.sh b/integration-tests/scripts/setup.sh index 595b515..7e20ca7 100755 --- a/integration-tests/scripts/setup.sh +++ b/integration-tests/scripts/setup.sh @@ -55,6 +55,12 @@ echo "=== Assembling the Keycloak realm ===" echo "=== Assembling Trino config ===" "$SCRIPT_DIR/gen-trino-config.sh" +# The assembled config's content, as a label on the trino service +# (compose.yaml): compose recreates a service whose definition changed, so a +# changed fragment reaches a running coordinator without a profile change. +TRINO_CONFIG_HASH="$(cd "$GENERATED/trino" && find . -type f -print0 | sort -z \ + | xargs -0 sha256sum | sha256sum | cut -c1-16)" +export TRINO_CONFIG_HASH # After every generator, before anything is mounted. make_mounts_readable diff --git a/integration-tests/stack/compose.yaml b/integration-tests/stack/compose.yaml index c57ff09..11dc1de 100644 --- a/integration-tests/stack/compose.yaml +++ b/integration-tests/stack/compose.yaml @@ -17,6 +17,10 @@ services: # Pinned, never `latest`. The floor is 466: the spooling protocol the # spooling profile needs did not exist before it. image: trinodb/trino:483 + labels: + # Set by scripts/setup.sh from the assembled config. A changed value is a + # changed service definition, so compose recreates the coordinator. + stackable.odbc.trino-config-hash: "${TRINO_CONFIG_HASH:-unset}" ports: - "8443:8443" volumes: From c7c0b813c36579e867831dd3a6a9386f31aee23d Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 9 Oct 2026 13:11:13 +0200 Subject: [PATCH 19/20] docs: describe the tracing slowdown without test-machine numbers WINDOWS.md, the README's troubleshooting entry and check_dm_tracing's doc comment quoted timings from one test VM, which say nothing about anyone else's setup. All three now describe the effect qualitatively: tracing writes every ODBC call to a file and slows reads down enough for Trino to abandon a long one. Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 6 +++--- integration-tests/windows/WINDOWS.md | 12 ++++++------ integration-tests/windows/windows_test.py | 10 ++++------ 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 1e0294e..de1e593 100644 --- a/README.md +++ b/README.md @@ -330,9 +330,9 @@ value from the first of the repeated hours is not selected. abandons a query whose results the client has not fetched within `query.client.timeout` (5 minutes by default), and later forgets it altogether, after which the next fetch returns `404 Not Found: Query not found`. A client -that reads slowly enough gets there on a large result. One cause we have -measured on Windows is ODBC tracing left switched on: it writes every call to a -file, and made a read 25 to 130 times slower. Turn it off in the ODBC Data Source +that reads slowly enough gets there on a large result. One cause on +Windows is ODBC tracing left switched on: it writes every call to a file and +slows reads down considerably. Turn it off in the ODBC Data Source Administrator (Tracing tab, **Stop Tracing Now**), including on an on-premises data gateway. diff --git a/integration-tests/windows/WINDOWS.md b/integration-tests/windows/WINDOWS.md index 6ac23c4..bb140e1 100644 --- a/integration-tests/windows/WINDOWS.md +++ b/integration-tests/windows/WINDOWS.md @@ -124,12 +124,12 @@ which can predate the feature under test by days. **Driver Manager tracing must be off.** The harness checks the three places Windows keeps the switch (the current user's, the machine's and the 32-bit Driver Manager's `ODBC.INI\ODBC` key) and stops if any has `Trace=1`. Tracing -writes every ODBC call to a file: on this VM, 2,000 rows took 1.3-1.7 s with it -off and 42-172 s with it on, slow enough for Trino to abandon a long read -(`ABANDONED_QUERY`). It is easy to leave on after a diagnosis, and the result -looks like a driver problem. Turn it off in the ODBC Data Source Administrator -(Tracing tab, **Stop Tracing Now**) or set `Trace` to `0` under the key the -error names. `--allow-dm-trace` is for a run where tracing is the point. +writes every ODBC call to a file, which slows reads down so much that Trino can +abandon a long one (`ABANDONED_QUERY`). It is easy to leave on after a +diagnosis, and the result looks like a driver problem. Turn it off in the ODBC +Data Source Administrator (Tracing tab, **Stop Tracing Now**) or set `Trace` to +`0` under the key the error names. `--allow-dm-trace` is for a run where +tracing is the point. The verified configurations connect to `trino` rather than to an address. TLS sends no SNI for an IP literal, and Jetty then serves Trino's internal diff --git a/integration-tests/windows/windows_test.py b/integration-tests/windows/windows_test.py index 94172d2..bcd2585 100644 --- a/integration-tests/windows/windows_test.py +++ b/integration-tests/windows/windows_test.py @@ -676,12 +676,10 @@ def _port_available(port: int) -> bool: def check_dm_tracing(session, allowed: bool): """Refuse to run while ODBC Driver Manager tracing is on in the VM. - Tracing writes every ODBC call to a file. Measured on this VM (2026-10-09): - 2,000 rows of tpcds.sf1.customer took 1.3-1.7 s with tracing off and - 42-172 s with it on. That is slow enough for Trino to abandon a long read - (ABANDONED_QUERY), which failed the spooling suite and was taken for a - driver problem for a day. Tracing is easy to leave on after a diagnosis, - and nothing else here would say so. + Tracing writes every ODBC call to a file, which slows reads down so much + that Trino can abandon a long one (ABANDONED_QUERY): timing-sensitive + suites then fail in a way that looks like a driver problem. Tracing is + easy to leave on after a diagnosis, and nothing else here would say so. """ keys = ", ".join(f"'{k}'" for k in DM_TRACE_KEYS) r = session.run_ps( From 197cf6650615cd51f0aaaf439e334a4075d64a4e Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 9 Oct 2026 15:05:32 +0200 Subject: [PATCH 20/20] docs: note that incremental-refresh bounds follow the session time zone For a timestamp with time zone column, Power BI sends RangeStart and RangeEnd as plain TIMESTAMP literals, which Trino interprets in the session time zone. Partition boundaries therefore move if TimeZone changes on a dataset that already has partitions. Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/README.md b/README.md index de1e593..fe6a3ce 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,11 @@ proper entry in the **Get Data** dialog instead of the generic ODBC one. 2. In **File > Options > Security**, allow any extension to load. 3. Restart Power BI Desktop. **Stackable Trino** now appears under **Get Data**. +For incremental refresh on a `timestamp with time zone` column, Trino compares +the `RangeStart` and `RangeEnd` bounds in the session time zone (`TimeZone`), +so partition boundaries follow that zone. Changing `TimeZone` on a dataset that +already has partitions moves the boundaries. + ### Your first query Assuming a Trino instance is reachable on the given host and port: