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

.github/workflows/dialect-live-cloud.yml

@@ -37,56 +37,13 @@ on:
   workflow_dispatch:
 
 jobs:
-  # Build the dialect_live test binary once, exactly as dialect-live.yml
-  # does, so the per-backend legs download it instead of each recompiling
-  # the same crate against the same feature set.
+  # Build the dialect_live test binary once (reusable workflow shared with
+  # dialect-live.yml), so the per-backend legs download it instead of each
+  # recompiling the same crate against the same feature set. save-cache
+  # stays false: a nightly run must not fight over the shared cache key
+  # owned by build.yaml/main.
   compile:
-    runs-on: ubuntu-latest
-    steps:
-      - name: Check out repository
-        uses: actions/checkout@v4
-
-      - name: Setup Node.js
-        uses: actions/setup-node@v4
-        with:
-          node-version: "22"
-
-      - name: Install tree-sitter-cli
-        # parser.c is gitignored and generated from grammar.js by the
-        # tree-sitter-ggsql build script on every fresh checkout.
-        run: npm install -g tree-sitter-cli
-
-      - name: Install Rust
-        uses: dtolnay/rust-toolchain@stable
-
-      - name: Caching
-        uses: Swatinem/rust-cache@v2
-        with:
-          # Same shared-key as build.yaml and dialect-live.yml: the prefix
-          # fallback restores their target/ and this job only rebuilds the
-          # feature deltas.
-          shared-key: ${{ runner.os }}-build
-          cache-on-failure: true
-
-      - name: Compile the dialect_live test binary
-        # Same feature set as dialect-live.yml so the restored crate
-        # fingerprints hit; see the long comment there. --no-run plus
-        # --message-format=json yields the binary path; the binary is
-        # self-contained (deps statically linked, ADBC drivers dlopened at
-        # runtime), so the legs can run it directly.
-        run: |
-          bin=$(cargo test -p ggsql --features "adbc sqlite" --test dialect_live \
-                --no-run --message-format=json-render-diagnostics \
-                | jq -rs '[.[] | select(.target.name == "dialect_live" and .executable != null) | .executable][-1]')
-          test -n "$bin" -a -f "$bin"
-          cp "$bin" dialect-live-bin
-
-      - name: Upload the test binary
-        uses: actions/upload-artifact@v4
-        with:
-          name: dialect-live-bin
-          path: dialect-live-bin
-          retention-days: 1
+    uses: ./.github/workflows/dialect-live-compile.yml
 
   live:
     needs: compile

.github/workflows/dialect-live-compile.yml

@@ -0,0 +1,70 @@
+name: dialect-live compile (reusable)
+
+# Shared compile job for the dialect-live workflows: builds the
+# dialect_live test binary once and hands it to every backend leg as an
+# artifact, so the legs compile nothing. Callers:
+# - dialect-live.yml (PRs + main): passes save-cache on main only.
+# - dialect-live-cloud.yml (nightly): never saves — a scheduled run must
+#   not fight over the shared cache key owned by build.yaml/main.
+
+on:
+  workflow_call:
+    inputs:
+      save-cache:
+        description: Whether to save the rust-cache back under the shared key.
+        type: boolean
+        default: false
+
+jobs:
+  compile:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Check out repository
+        uses: actions/checkout@v4
+
+      - name: Setup Node.js
+        uses: actions/setup-node@v4
+        with:
+          node-version: "22"
+
+      - name: Install tree-sitter-cli
+        # parser.c is gitignored and generated from grammar.js by the
+        # tree-sitter-ggsql build script on every fresh checkout.
+        run: npm install -g tree-sitter-cli
+
+      - name: Install Rust
+        uses: dtolnay/rust-toolchain@stable
+
+      - name: Caching
+        uses: Swatinem/rust-cache@v2
+        with:
+          # Deliberately the same shared-key as build.yaml: rust-cache's
+          # prefix fallback restores the build job's target/, so this job
+          # only rebuilds the feature deltas instead of every dependency,
+          # and on main it saves the warmed variant back for build.yaml
+          # and PR runs to reuse.
+          shared-key: ${{ runner.os }}-build
+          cache-on-failure: true
+          save-if: ${{ inputs.save-cache }}
+
+      - name: Compile the dialect_live test binary
+        # --no-run plus --message-format=json yields the binary path; the
+        # binary is self-contained (deps statically linked, ODBC/ADBC
+        # drivers dlopened at runtime), so the legs can run it directly.
+        # Feature set is load-bearing: `adbc` compiles in the ADBC reader
+        # path and `sqlite` provides the cache backend Druid wraps in. The
+        # datafusion leg uses the Foundry driver through the driver manager,
+        # so the DataFusion graph never enters the test build.
+        run: |
+          bin=$(cargo test -p ggsql --features "adbc sqlite" --test dialect_live \
+                --no-run --message-format=json-render-diagnostics \
+                | jq -rs '[.[] | select(.target.name == "dialect_live" and .executable != null) | .executable][-1]')
+          test -n "$bin" -a -f "$bin"
+          cp "$bin" dialect-live-bin
+
+      - name: Upload the test binary
+        uses: actions/upload-artifact@v4
+        with:
+          name: dialect-live-bin
+          path: dialect-live-bin
+          retention-days: 1

.github/workflows/dialect-live.yml

@@ -30,62 +30,15 @@ on:
   workflow_dispatch:
 
 jobs:
-  # Build the dialect_live test binary once. rust-cache (same shared-key as
-  # build.yaml) restores build.yaml's warmed target/, so this compiles only
-  # the workspace crates and the feature deltas; the binary is then handed
-  # to every backend leg as an artifact, so the legs compile nothing.
+  # Build the dialect_live test binary once (reusable workflow shared with
+  # dialect-live-cloud.yml). rust-cache restores build.yaml's warmed
+  # target/ via the same shared-key; on main the warmed variant is saved
+  # back for build.yaml and PR runs to reuse. The binary is handed to
+  # every backend leg as an artifact, so the legs compile nothing.
   compile:
-    runs-on: ubuntu-latest
-    steps:
-      - name: Check out repository
-        uses: actions/checkout@v4
-
-      - name: Setup Node.js
-        uses: actions/setup-node@v4
-        with:
-          node-version: "22"
-
-      - name: Install tree-sitter-cli
-        # parser.c is gitignored and generated from grammar.js by the
-        # tree-sitter-ggsql build script on every fresh checkout.
-        run: npm install -g tree-sitter-cli
-
-      - name: Install Rust
-        uses: dtolnay/rust-toolchain@stable
-
-      - name: Caching
-        uses: Swatinem/rust-cache@v2
-        with:
-          # Deliberately the same shared-key as build.yaml: rust-cache's
-          # prefix fallback restores the build job's target/, so this job
-          # only rebuilds the feature deltas instead of every dependency,
-          # and on main it saves the warmed variant back for build.yaml
-          # and PR runs to reuse.
-          shared-key: ${{ runner.os }}-build
-          cache-on-failure: true
-          save-if: ${{ github.ref == 'refs/heads/main' }}
-
-      - name: Compile the dialect_live test binary
-        # --no-run plus --message-format=json yields the binary path; the
-        # binary is self-contained (deps statically linked, ODBC/ADBC
-        # drivers dlopened at runtime), so the legs can run it directly.
-        # Feature set is load-bearing: `adbc` compiles in the ADBC reader
-        # path and `sqlite` provides the cache backend Druid wraps in. The
-        # datafusion leg uses the Foundry driver through the driver manager,
-        # so the DataFusion graph never enters the test build.
-        run: |
-          bin=$(cargo test -p ggsql --features "adbc sqlite" --test dialect_live \
-                --no-run --message-format=json-render-diagnostics \
-                | jq -rs '[.[] | select(.target.name == "dialect_live" and .executable != null) | .executable][-1]')
-          test -n "$bin" -a -f "$bin"
-          cp "$bin" dialect-live-bin
-
-      - name: Upload the test binary
-        uses: actions/upload-artifact@v4
-        with:
-          name: dialect-live-bin
-          path: dialect-live-bin
-          retention-days: 1
+    uses: ./.github/workflows/dialect-live-compile.yml
+    with:
+      save-cache: ${{ github.ref == 'refs/heads/main' }}
 
   live:
     needs: compile

src/execute/golden.rs

@@ -27,8 +27,7 @@ mod battery;
 
 use crate::reader::registry;
 use crate::reader::test_support::StubReader;
-use crate::reader::{AnsiDialect, Reader, SqlDialect};
-use crate::DataFrame;
+use crate::reader::{AnsiDialect, Reader};
 use std::path::PathBuf;
 use std::sync::{Arc, Mutex};
 
@@ -37,64 +36,15 @@ use std::sync::{Arc, Mutex};
 /// predates and outlives any single scheme); everything else resolves
 /// through the registry, so a dialect constructor can only live in one
 /// place. The `goldens_cover_registry` test keeps the file set in step.
-fn dialect_for(name: &str) -> Box<dyn SqlDialect + Send> {
+fn dialect_for(name: &str) -> crate::reader::DialectRef {
     if name == "ansi" {
-        return Box::new(AnsiDialect);
+        return &AnsiDialect;
     }
     registry::by_scheme(name)
         .unwrap_or_else(|| panic!("no registry entry for golden dialect '{name}'"))
         .dialect()
 }
 
-/// The shared fixture as a DataFrame, matching the live table's schema:
-/// id/val/grp/day(a real date)/"mixed Case". `TextDay` swaps the date for
-/// ISO text to force the text→temporal cast path.
-fn fixture_df(fixture: &battery::Fixture) -> DataFrame {
-    use arrow::array::{ArrayRef, Date32Array, Float64Array, Int32Array, StringArray};
-    let day: ArrayRef = match fixture {
-        battery::Fixture::Shared => Arc::new(Date32Array::from(
-            battery::ROWS
-                .iter()
-                .map(|r| r.day_epoch)
-                .collect::<Vec<_>>(),
-        )),
-        battery::Fixture::TextDay => Arc::new(StringArray::from(
-            battery::ROWS
-                .iter()
-                .map(|r| battery::day_iso(r.day_epoch))
-                .collect::<Vec<_>>(),
-        )),
-    };
-    DataFrame::new(vec![
-        (
-            "id",
-            Arc::new(Int32Array::from(
-                battery::ROWS.iter().map(|r| r.id).collect::<Vec<_>>(),
-            )) as ArrayRef,
-        ),
-        (
-            "val",
-            Arc::new(Float64Array::from(
-                battery::ROWS.iter().map(|r| r.val).collect::<Vec<_>>(),
-            )) as ArrayRef,
-        ),
-        (
-            "grp",
-            Arc::new(StringArray::from(
-                battery::ROWS.iter().map(|r| r.grp).collect::<Vec<_>>(),
-            )) as ArrayRef,
-        ),
-        ("day", day),
-        (
-            "mixed Case",
-            Arc::new(Float64Array::from(
-                battery::ROWS.iter().map(|r| r.mixed).collect::<Vec<_>>(),
-            )) as ArrayRef,
-        ),
-    ])
-    .unwrap()
-}
-
 /// Collapse all whitespace runs so goldens don't churn on formatting, and
 /// replace the random per-session hex in generated temp-table names
 /// (`__ggsql_global_<32 hex>__`) with a stable placeholder.
@@ -130,7 +80,7 @@ fn golden_dir() -> PathBuf {
 
 /// Render every battery case's SQL for one dialect. Re-registers the
 /// fixture only when its variant changes.
-fn render_dialect(dialect: Box<dyn SqlDialect + Send>) -> String {
+fn render_dialect(dialect: crate::reader::DialectRef) -> String {
     let (reader, log) = StubReader::new(dialect);
     let mut out = String::new();
     let mut registered: Option<u8> = None;
@@ -145,7 +95,7 @@ fn render_dialect(dialect: Box<dyn SqlDialect + Send>) -> String {
                 reader.unregister(battery::TABLE).unwrap();
             }
             reader
-                .register(battery::TABLE, fixture_df(&case.fixture), true)
+                .register(battery::TABLE, battery::fixture_df(&case.fixture), true)
                 .unwrap();
             registered = Some(variant);
         }

src/reader/adbc.rs

@@ -26,7 +26,7 @@ pub struct AdbcReader<D: Driver> {
     // new_statement / set_sql_query / execute all take &mut, but Reader::execute_sql
     // takes &self.
     connection: RefCell<<D::DatabaseType as Database>::ConnectionType>,
-    dialect: Box<dyn SqlDialect + Send>,
+    dialect: crate::reader::DialectRef,
     /// Registry-declared driver quirks (error-text conventions), when the
     /// reader was built for a registered backend. `None` for ad-hoc drivers.
     adbc_info: Option<crate::reader::registry::AdbcInfo>,
@@ -40,7 +40,7 @@ impl<D: Driver> AdbcReader<D> {
     /// Construct an `AdbcReader` with an explicit `SqlDialect`. Use this to
     /// plug in backend-specific dialects (e.g. a TrinoDialect, SnowflakeDialect)
     /// when the reader is pointed at that backend.
-    pub fn with_dialect(driver: D, dialect: Box<dyn SqlDialect + Send>) -> Result<Self> {
+    pub fn with_dialect(driver: D, dialect: crate::reader::DialectRef) -> Result<Self> {
         Self::new(driver, dialect)
     }
 
@@ -50,7 +50,7 @@ impl<D: Driver> AdbcReader<D> {
     /// options. For convenience, use `from_driver` for the common case
     /// with the ANSI dialect, or pass a custom `SqlDialect`
     /// (e.g. a Trino / Snowflake dialect) here directly.
-    pub fn new(mut driver: D, dialect: Box<dyn SqlDialect + Send>) -> Result<Self> {
+    pub fn new(mut driver: D, dialect: crate::reader::DialectRef) -> Result<Self> {
         let database = driver
             .new_database()
             .map_err(|e| GgsqlError::ReaderError(format!("ADBC new_database failed: {}", e)))?;
@@ -74,7 +74,7 @@ impl<D: Driver> AdbcReader<D> {
     /// or other auth-required backends).
     pub fn new_with_database_opts(
         mut driver: D,
-        dialect: Box<dyn SqlDialect + Send>,
+        dialect: crate::reader::DialectRef,
         opts: impl IntoIterator<
             Item = (
                 adbc_core::options::OptionDatabase,
@@ -156,7 +156,7 @@ impl<D: Driver> AdbcReader<D> {
     /// standards-compliant backends; use `new` directly to plug in a
     /// backend-specific dialect.
     pub fn from_driver(driver: D) -> Result<Self> {
-        Self::new(driver, Box::new(AnsiDialect))
+        Self::new(driver, &AnsiDialect)
     }
 }
 
@@ -493,6 +493,12 @@ fn bigquery_driver_uri(body: &str, query: &str) -> (String, String) {
 
 /// Probe whether an ADBC driver for `scheme` can be loaded, without opening
 /// a connection. Used by reader dispatch to decide between ADBC and ODBC.
+///
+/// Note this loads (dlopens) the driver library and drops it;
+/// `from_connection_string` loads it again when the probe succeeds. That
+/// double-load is accepted connect-time cost: threading the preloaded
+/// driver through dispatch would complicate the probe-then-build flow for
+/// no per-query benefit.
 pub fn adbc_driver_available(scheme: &str) -> bool {
     load_driver_for_scheme(scheme).is_ok()
 }
@@ -629,7 +635,7 @@ where
                 .map_err(|e| GgsqlError::ReaderError(format!("ADBC execute_update DROP: {}", e)))?;
         }
 
-        let create_sql = crate::reader::create_table_sql(name, &schema, &*self.dialect)?;
+        let create_sql = crate::reader::create_table_sql(name, &schema, self.dialect)?;
         self.new_query_statement(&mut conn, &create_sql)?
             .execute_update()
             .map_err(|e| GgsqlError::ReaderError(format!("ADBC execute_update CREATE: {}", e)))?;
@@ -740,7 +746,7 @@ where
     }
 
     fn dialect(&self) -> &dyn SqlDialect {
-        &*self.dialect
+        self.dialect
     }
 }
 
@@ -1018,10 +1024,9 @@ mod equivalence_tests {
             None,
         )
         .expect("`dbc install sqlite` first; see module docs");
-        let dialect: Box<dyn crate::reader::SqlDialect + Send> = Box::new(SqliteDialect);
         AdbcReader::new_with_database_opts(
             driver,
-            dialect,
+            &SqliteDialect,
             std::iter::once((
                 OptionDatabase::Uri,
                 OptionValue::String(format!("file:{}", db_path)),

src/reader/cache.rs

@@ -1424,8 +1424,8 @@ mod behavior_tests {
         use crate::reader::dialects::{DuckDbDialect, MySqlDialect};
         use crate::reader::test_support::StubReader;
 
-        let (primary, primary_sql) = StubReader::new(Box::new(MySqlDialect));
-        let (cache, _cache_sql) = StubReader::new(Box::new(DuckDbDialect));
+        let (primary, primary_sql) = StubReader::new(&MySqlDialect);
+        let (cache, _cache_sql) = StubReader::new(&DuckDbDialect);
         // Memoization disabled: the stub fabricates Float64 results that the
         // memo metadata path cannot store, which is beside the point here.
         let reader = CachingReader::with_config(