Stronger dialect support + 14 new readers by thomasp85 · Pull Request #569 · posit-dev/ggsql

thomasp85 · 2026-09-29T12:06:14Z

This PR provides a large swath of readers build on top of the existing ADBC/ODBC support we have. It refactors the dialects into a specific module with dialect detection, expands the dialect to accomodate new wrinkles discovered by the new readers, provides a multitiered selectioon mechanism where ADBC is preferred, but ODBC is tried if no suitable ADBC driver exist, and lastly adds an autowrapping of a cache reader when needed. The latter is supported through the dialect system as some dbs forcefully requires a cache to work. For the others we try a temp table creation and if it fails we use a cache.

The dialects added here are

Snowflake
Databricks
BigQuery
Redshift
PostgreSQL
MySQL/MariaDB
Datafusion
Clickhouse
MonetDB
Exasol
Trino
Drill
Druid
Oracle
Microsoft SQL Server

Fix #341, fix #509, closes #386, closes #458

Changes with runtime-semantics impact riding along in this PR

These commits change behavior rather than just how SQL text is built. They are
called out here so they can be reviewed individually; see also the CHANGELOG
entries.

a687c9c4 — Vega-Lite timestamp double-rescaling fix
(src/writer/vegalite/data.rs): unrelated correctness fix for microsecond
timestamps being rescaled twice.
Temporal-unit mismatch detection/conversion + new error path in scale
resolution (src/plot/scale/scale_type/mod.rs, temporal_unit_micros,
convert_range_to_transform_unit, check_temporal_domain): new error path,
surfaced by the broader multi-backend test matrix.
Extent merging/sorting (src/execute/schema.rs, merge_extent_rows):
portability fix for MySQL "can't reopen table" / ClickHouse alias reuse;
changes how min/max extents are derived.
Public API breaks: CacheBackend moved to test-support, CachingReader::new
is now #[cfg(test)], Spec::layer_sql/Spec::stat_sql removed. Recorded in
CHANGELOG under [Unreleased].

…dule

…, sqlite, redshift

thomasp85 · 2026-09-30T10:56:34Z

I've been hard at work setting up testing infrastructure for this so we can be somewhat sure that ggsql actually works with the stated backends. For non-cloud backends we have a live CI that runs a battery of queries against a db. For cloud based we have a nightly run backed by credentials in the repo secrets

The tests currently miss:

Drill (only commercial odbc driver + it requires a cache layer meaning that ggsql will not really talk to it)
Oracle (only commercial drivers available)
Datafusion (bug in the available ADBC driver - filed upstream)
Snowflake (awaiting an account from org to set up as nightly job)

All of this testing has payed off. So many portability issues have been uncovered and fixed at the detriment of the complexity of this PR. Some stat queries have been rewritten, the dialect system expanded, and quoting has become a part of the dialect

- Error instead of panicking when a scheme without registry ADBC info (monetdb, drill) reaches AdbcReader::from_connection_string, e.g. via the GGSQL_<SCHEME>_ADBC_DRIVER env override. - reader=native no longer falls through to ODBC when no native reader exists or is compiled in; it errors explicitly like reader=adbc. - odbc:// URIs are handed to the driver with ggsql-owned params stripped, so ?cache=off and friends no longer leak into the connection string.

CachingReader::materialize_table executes the body against the primary but quoted aliases with the cache dialect, sending double-quoted identifiers to backtick dialects like MySQL. Regression test included.

- case_greatest/case_least no longer index-panic on empty input. - Date/datetime/time literal defaults clamp out-of-range input instead of panicking (and no longer wrap negative time-of-day via as u32). - drop_stmt_handle skips its debug_assert while unwinding, avoiding an abort on a failed free during panic. - ODBC diagnostic buffer growth is clamped to SqlSmallInt::MAX so the length argument cannot wrap negative.

- Dialect module docs for redshift, exasol, monetdb and bigquery claimed spatial support that supports_spatial() (false by default) blocks; the docs now match the code. - Battery live_skip for the boxplot global-source case extended to mariadb, which shares MySQL's temp-table reopen limitation. - Rename leftovers (sql_percentile -> sql_quantile) and stale doc references cleaned up. - CHANGELOG entries for the API breaks and behavior changes riding along in this PR.

teunbrand

Calling it a day, I'll continue reviewing tomorrow

teunbrand · 2026-10-07T08:41:10Z

+    /// SQL type name for numeric columns (e.g., "DOUBLE PRECISION").
+    /// Derived from [`type_names`]; override that, not this.
+    ///
+    /// [`type_names`]: SqlDialect::type_names
    fn number_type_name(&self) -> Option<&str> {


Bit frivolous comment, but I get that these accessors were valuable before your PR, but can now mostly be replaced by self.type_names().{number, date, integer, ...} at call sites. Doesn't do any harm to keep them, but it'd clean up the SqlDialect trait a bit.

If we want to keep them, it might make sense to double-down on their accessor role by using a get_-prefix.

teunbrand · 2026-10-07T09:27:28Z

+    /// `CREATE TABLE AS`).
+    SelectInto,
+}
+


From comment it is not clear what role this function serves

Suggested change

/// Emulate `GENERATE_SERIES(0, n - 1)` for DBs that lack it

teunbrand · 2026-10-07T09:38:35Z

@@ -46,39 +46,55 @@ use crate::{naming, DataFrame, GgsqlError, Result};
 ///
 /// Default implementations produce portable ANSI SQL.
 pub trait SqlDialect {


For our own sense-making efforts, I'd recommend to group the methods into categories. Some of these are pure SQL translations, others are SQL pattern generators, some are self-knowledge (i.e. does this dialect support spatial stuff), there are some spatial specific patterns/translations. The methods are currently somewhat mixed and reordering them gives a semblance of structure.

teunbrand · 2026-10-07T11:35:03Z

-        select_parts.join(", ")
-    );
+    let sql = crate::sql::Select::new(dialect)
+        .select(select_parts.join(", "))


Suggested change

.select(select_parts.join(", "))

.select_items(select_parts)

teunbrand · 2026-10-07T11:36:22Z

-    );
+    let __ggsql_vertices__ = dialect.quote_ident("__ggsql_vertices__");
+    let sql = crate::sql::Select::new(dialect)
+        .select(select_parts.join(", "))


Suggested change

.select(select_parts.join(", "))

.select_items(select_parts)

teunbrand · 2026-10-07T14:53:45Z

+/// These are consumed during dispatch and must never reach a driver's own
+/// URI parsing or option map — drivers reject unknown keys.
+#[derive(Debug, Default, Clone, PartialEq, Eq)]
+pub struct GgsqlParams {


I think the name of this structure is too general, mostly because it implies this'd be a parameter structure we use throughout (like GgsqlError for example). I'd be happy with something like ConnectParams or Own(ed)Params or something.

teunbrand · 2026-10-07T14:56:03Z

+    /// `cache_ttl=<secs>`: raw value; parsed by
+    /// [`ConnUri::cache_config_override`].
+    pub cache_ttl: Option<String>,
+    /// `cache_max_bytes=<n|1MB|…>`: raw value.
+    pub cache_max_bytes: Option<String>,
+    /// `cache_disabled=1|true|yes`.
+    pub cache_disabled: Option<bool>,
+    /// `stmt.<key>=<value>` params (prefix stripped): driver *statement*
+    /// options, applied to every statement the reader creates.


Comments here don't immediately make sense to me, so maybe add 1 line of explanatory prose per field (e.g. 'Maximum bytes the cache is allowed to have', or whatever it actually means)

teunbrand · 2026-10-07T15:20:10Z

+                continue;
+            }
+            let (key, value) = match segment.split_once('=') {
+                Some((k, v)) => (k, Some(v)),


This is a nitpick with 0 priority.

We could do this

Suggested change

Some((k, v)) => (k, Some(v)),

Some((k, v)) => (k, Some(v.to_string())),

And then the match arms could simplify to just use value instead of value.map(|v| v.to_string()).

teunbrand · 2026-10-07T15:21:09Z

Does this mean that we can only use the datafusion cache when duckdb/sqlite are compiled in?

teunbrand · 2026-10-07T15:23:41Z

@@ -78,48 +191,37 @@ fn cache_uri(scheme: &str) -> Result<&'static str> {
    match scheme {
        "duckdb" => Ok("duckdb://memory"),
        "sqlite" => Ok("sqlite://memory"),


This differs from L549 (sqlite://:memory:). Should this be unified?

teunbrand

Mostly LGTM, few comments that deserve a glance but nothing that should stop a merge.

Caveat to my review is that I don't really know the DB connection stuff very well, so I'm mostly trusting you on this.

Few things that didn't map neatly onto changes:

Can you update any relevant CLAUDE.mds? I found the testing architecture with e.g. golden.rs living in the execute folder hard to follow, so having this written down somewhere would help me as well as future agents.
rewrite_namespaced_sql in src/parser/sql.rs should probably also use the dialect quoting instead of the naming module. It is now only called in DuckDB/SQLite so not immediately relevant.
Could have the assistant give another look at comments. I typically ask it to review the comments it made and pay attention to (1) generally terse style (2) delete commentary about decisions (3) delete commentary about history and (4) move implementation details to relevant lines of code. (2) and (3) are mostly because assistants are bad at following CLAUDE.md:121 (Comments describe the current state of the code) directive.

teunbrand · 2026-10-08T07:50:24Z

-                "AdbcReader::register: empty DataFrame not supported".into(),
-            ));
-        }
+        // Zero-row frames are fine: the CREATE below still runs, leaving an


I think the comments here can generally be improved. Many are a full paragraph, contain details that are already abstracted and I've not found them helpful in grokking what the code is doing. This specific one here seems to be misplaced, as it is about logic that is at L625: ~40 lines down the road.

teunbrand · 2026-10-08T07:51:53Z

-                    )));
+        self.registered_tables.note_registered(name);
+
+        if batch.num_rows() > 0 {


This is just a personal bias against indentation depth, but if you early exit here if you have less than 1 row, you save 1 level.

teunbrand · 2026-10-08T09:07:08Z

+            let grp_cols = group_by
+                .iter()
+                .map(|c| dialect.quote_ident(c))
+                .collect::<Vec<_>>()
+                .join(", ");


This (quoting vector followed by joining) is a recurring pattern that might be worth a little helper, maybe on the dialect itself?

There are also some variants of this that follow the same pattern (like quoted_groups in boxplot.rs).

teunbrand · 2026-10-08T09:22:46Z

+         {__bin_src__} AS (SELECT *, {bin_expr} AS {bin_key} FROM {__stat_src__}), \
+         {__binned__} AS (SELECT {binned} FROM {__bin_src__} GROUP BY {group}) \
+         SELECT *, {bin} + {width} AS {bin_end}, \


Oughtn't this be Select-constructed to deal with the wildcard?

teunbrand · 2026-10-08T09:45:51Z

+/// registered tables by name; anything unrecognized defaults to
+/// `Float64`, which is the right shape for the derived columns
+/// (bins, densities, quantiles) the pipeline produces.
+pub(crate) struct StubReader {


Easily will get confused with stubs in TABULATE (read: I'd like Ctrl/Cmd+F global search on 'stub' to only give tabulate stuff 😁 ), so I'd argue for some different name, e.g. TestReader, DummyReader or SterileReader or something you see fit.

teunbrand · 2026-10-08T10:10:48Z

+      matrix:
+        include:
+          - { backend: snowflake, driver: snowflake }
+          - { backend: bigquery, driver: bigquery }
+          - { backend: databricks, driver: databricks }
+
+    env:
+      GGSQL_TEST_URI_SNOWFLAKE: ${{ secrets.GGSQL_TEST_URI_SNOWFLAKE }}
+      GGSQL_TEST_URI_BIGQUERY: ${{ secrets.GGSQL_TEST_URI_BIGQUERY }}
+      GGSQL_TEST_URI_DATABRICKS: ${{ secrets.GGSQL_TEST_URI_DATABRICKS }}
+


Flagged by assistant: all 3 backends have access to all 3 secrets, whereas snowflake db doesn't need to know about bigquery secret. Probably not too severe, but worth mentioning.

teunbrand · 2026-10-08T10:12:25Z

@@ -0,0 +1,154 @@
+name: Nightly Cloud Dialect Tests


Flagged by assistant: workflow has default permissions, whereas it needs only read permissions. Probably not severe, but worth mentioning.

teunbrand · 2026-10-08T10:29:50Z

@@ -718,6 +758,32 @@ fn build_bin_condition(
    oob_squish: bool,
    is_first: bool,
    is_last: bool,
+    dialect: &dyn super::SqlDialect,
+) -> String {
+    build_bin_condition_expr(
+        &dialect.quote_ident(column_name),
+        lower_expr,
+        upper_expr,
+        closed_left,
+        oob_squish,
+        is_first,
+        is_last,
+        dialect,
+    )
+}


This wrapper seems frivolous (i.e. 20 lines of wrapper + comments to save 1 inline dialect.quote_ident(). Do we really need it?

thomasp85 added 28 commits September 29, 2026 11:55

Wire dialect through for quoting behavior

05c313b

Portability fix for histogram

a3b24ba

Fix bug causing double rescaling of timestamps

a687c9c

all the dialects plus moving of duckdb and sqlite into the dialect mo…

e336017

…dule

Automatic cache detection

eb54420

update to use native drivers

478235b

Add dialect testing scheme

88a2c6c

reverse fmt regressions

e678490

allow many arguments

22e1676

split dbc installs to multiple calls

32057ed

reuse from cache

1b33b67

include needed featuers

41f0239

Fix driver lookup for dbc installed drivers

6a9dcea

add credentials to clickhouse

a427074

pass credentials correctly

b5d7235

Fix density query to avoid IS NOT DISTINCT FROM outside JOIN

73ebd4e

add additional backends to the test suite

1dc631b

improve CI run time

7611e95

fix issues in new dbs

aebde8d

Fix portability issues

d2c47c5

Make quoting fully dynamic - set up cloud testing infra - test duckdb…

8e8a653

…, sqlite, redshift

setup bigquery

b1ba6a1

More portability fixes

5c2b903

Expand the test battery

97b436a

one more fix for the road

47e6799

add monetdb and druid

4eac043

Try to cut donw on CI work

c0e783b

Fix POWER casting to avoid rounding to zero

0ba8031

fixing CI setup

596da92

thomasp85 added 5 commits October 5, 2026 13:31

fix regressions in the CLI

dc3f362

registry centralisation

2f6a6b7

finalize review

374d35e

Clean up CI

5acafaa

clean up comments

4b872f5

thomasp85 mentioned this pull request Oct 6, 2026

Move driver-specific workarounds behind registry hooks #575

Open

fix failing ci

b3bcc10

thomasp85 mentioned this pull request Oct 6, 2026

Fix recorded deficiencies in the readers #576

Open

Pin MySQL deficiancy with test

9acfff4

thomasp85 mentioned this pull request Oct 6, 2026

Add ClickHouse and chDB readers #535

Open

thomasp85 added 8 commits October 6, 2026 13:10

moar fix

e78b7d1

even moar

73a920c

fix druid test

cddc2d5

try fixing it again

2416789

Quote materialized column aliases with the primary dialect

a71fb74

CachingReader::materialize_table executes the body against the primary but quoted aliases with the cache dialect, sending double-quoted identifiers to backtick dialects like MySQL. Regression test included.

This was referenced Oct 6, 2026

Review follow-up: dialect/SQL-builder consolidation #577

Merged

Review follow-up: dialect dedup, capabilities and test/CI infrastructure #578

Merged

thomasp85 added this pull request to stack #579 October 6, 2026 17:51

thomasp85 requested a review from teunbrand October 6, 2026 17:55

teunbrand reviewed Oct 7, 2026

View reviewed changes

teunbrand approved these changes Oct 8, 2026

View reviewed changes

thomasp85 added 2 commits October 8, 2026 14:31

Fix flagged bugs from review

2f5c6b0

improve docs and comments

ac44245

thomasp85 mentioned this pull request Oct 8, 2026

Review follow-up: trait cleanup, MSSQL 2022 floor, naming and test feedback #580

Merged

thomasp85 merged commit afe60e6 into main Oct 9, 2026
23 checks passed

This was referenced Oct 9, 2026

Registry driver hooks #584

Open

Query generation fixes #585

Open



	/// Emulate `GENERATE_SERIES(0, n - 1)` for DBs that lack it

	Some((k, v)) => (k, Some(v)),
	Some((k, v)) => (k, Some(v.to_string())),

Conversation

thomasp85 commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes with runtime-semantics impact riding along in this PR

Uh oh!

thomasp85 commented Sep 30, 2026

Uh oh!

teunbrand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teunbrand left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teunbrand Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thomasp85 commented Sep 29, 2026 •

edited

Loading

teunbrand left a comment •

edited

Loading

teunbrand Oct 8, 2026 •

edited

Loading