Support multiple pins and pin+data frame tables via a shared DuckDB executor

[cpsievert]

Closes #312

Stacked on #310.

Summary

Replaces the registration-time rejection of a second pin with real support, following the design in the issue: PinSource and DataFrameSource gain an internal register_into(conn) hook — pins re-run their file-based materialization (native read for parquet/CSV/JSON, polars staging for arrow in Python, pin_read() fallback) against the shared connection; data frames register as before. build_query_executor() routes pin+pin and pin+data-frame groups to a shared DuckDBExecutor, and check_source_compatibility() allows the DuckDB family to mix (mixed data-frame backends still rejected, now checked against all existing DataFrameSources). Pins keep eager private materialization at construction (fail-fast errors unchanged) and own their private connection; the shared connection is executor-owned, locked down once after all tables materialize, and closed by cleanup_executor(). A grouped pin therefore materializes twice (once private, once into the executor from the local pin cache) — chosen over lazy construction to preserve construction-time errors. In R, engine = "sqlite" pins can't join a DuckDB executor, so multi-table groups containing them are rejected with a targeted message. Python rejection tests and CHANGELOG entry replaced; R rejection tests replaced and NEWS bullet updated.

Verification

board <- pins::board_temp(); pins::pin_write(board, mtcars, "mtcars"); pins::pin_write(board, iris, "iris")
qc <- QueryChat$new(board, "mtcars"); qc$add_table(board, "iris")  # previously rejected

Python: QueryChat(board, "pin_a").add_table(board, "pin_b") likewise works. New tests cover cross-pin joins, pin+data-frame mixing, per-table schema/validation, sqlite-pin rejection (R), and cleanup ownership. Full pytest suite (901 passed) and affected testthat files (384 passed) pass; ruff and air are clean.

[Copilot]

🔵 Needs a closer look

Fix staging-name collisions and unregister failed staging registrations before fallback in _pin_source.py.

Review details

Files not reviewed (2)

• pkg-r/man/DataFrameSource.Rd: Generated file
• pkg-r/man/PinSource.Rd: Generated file

Suppressed comments (1)

pkg-py/src/querychat/_pin_source.py:244

• vname is deterministic from target, so staging relations can collide with real tables from another pin (for example, a pin named __pin_staging_pin_a registered before an arrow/RDS pin named pin_a). In addition, if the first CREATE TABLE fails, its staging registration is not unregistered before this fallback reuses the same name, so the fallback can fail with an "already exists" error and leave the executor partially registered. Use a unique per-call staging name and unregister it in a finally block before retrying from the private copy.

            vname = f"__pin_staging_{target}"
            conn.register(vname, self.get_data().to_native())
            conn.execute(
                f"CREATE TABLE {quote_identifier(target)} AS "
                f"SELECT * FROM {quote_identifier(vname)}"

• Files reviewed: 12/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved issues remain around pin-version snapshots and broad exception fallbacks during materialization.

Review details

Files not reviewed (2)

• pkg-r/man/DataFrameSource.Rd: Generated file
• pkg-r/man/PinSource.Rd: Generated file

Suppressed comments (5)

pkg-py/src/querychat/_pin_source.py:73

• This new module-level helper uses a leading underscore even though _pin_source.py is already a private module. Per the package's Python naming convention, use a regular module-level name (and update both call sites) rather than adding another _-prefixed symbol.

def _stage_frame_as_table(
    conn: duckdb.DuckDBPyConnection, frame: Any, table_name: str
) -> None:
    """Materialize a DataFrame as a real table via a unique staging view."""

pkg-py/src/querychat/_pin_source.py:244

• Catching every exception here treats unrelated failures—such as a corrupt pin file, a DuckDB conversion error, or a connection failure—as a pruned snapshot and silently serves the stale private copy. Narrow this fallback to the pins error that indicates the snapshotted version is unavailable (or re-raise after verifying that condition), so real materialization errors still fail the shared executor.

            # The snapshotted pin version may no longer exist on the board
            # (e.g. a non-versioned board rewritten after construction); fall
            # back to this source's own copy so the shared table matches the
            # private connection.
            _stage_frame_as_table(conn, self.get_data().to_native(), target)

pkg-r/R/PinSource.R:110

• This only snapshots pin_meta$local$version, but PinSource also supports remote boards such as board_connect() (as documented above). For those boards the resolved version may live in remote metadata; when version is NULL, this leaves private$.version as NULL, so register_into() can materialize the board's newer latest version while the private connection still contains the version read at construction. Capture the board-resolved version for both local and remote boards so the shared executor cannot diverge from the source snapshot.

      private$.version <- private$.pin_meta$local$version %||% version

pkg-r/R/PinSource.R:110

• The resolved version is snapshotted here, but the eager non-file path still reads with the original version argument instead of this snapshot. If a default/latest pin is updated between pin_meta() and pin_read(), the private source and the shared executor can contain different versions, defeating the stated snapshot guarantee; use the stored version for the initial materialization too.

      # Snapshot the resolved version so register_into() reads the same pin
      # content even if the pin is updated after construction.
      private$.version <- private$.pin_meta$local$version %||% version

pkg-r/R/PinSource.R:198

• This handler catches every error and then registers the private copy, so corrupt files, DuckDB conversion failures, or connection errors can be misclassified as a pruned version and silently return stale data. Restrict the fallback to the pins error that means the snapshotted version is unavailable, and propagate other materialization failures.

        error = function(e) {
          duckdb::duckdb_register(
            con,
            table_name,
            self$get_data(),

• Files reviewed: 12/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Two moderate review findings remain unresolved.

Review details

Files not reviewed (2)

• pkg-r/man/DataFrameSource.Rd: Generated file
• pkg-r/man/PinSource.Rd: Generated file

Suppressed comments (2)

pkg-py/src/querychat/_query_executor.py:95

• This shared path registers a pin under the registry key name, but PinSource sanitizes non-SQL-safe names into its private table_name. A single-pin QueryChat(board, "sales-2026") still uses DataSourceExecutor and delegates to that private connection, so a query against the registry name fails even though the multi-pin path (and the new non-safe-name test) works. Please make the registry and source use one canonical name, or route the single-pin case through the same alias-aware executor.

            for name, source in sources.items():
                source.register_into(self._conn, name)

pkg-r/R/QueryExecutor.R:262

• Allowing this combination can make the actual executor and the prompt disagree: a DataFrameSource(engine = "sqlite") registered first with a DuckDB pin is routed to DuckDBExecutor, but QueryChatSystemPrompt still derives db_type and is_duck_db from that first source. The model is therefore told to write SQLite SQL and misses DuckDB-specific guidance even though DuckDB executes the query. Either reject SQLite frame members here or derive prompt metadata from the shared executor.

  if (
    is_duckdb_family_source(new_source) && is_duckdb_family_source(first_source)
  ) {

• Files reviewed: 12/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

No unresolved review issues were identified.

Review details

Files not reviewed (2)

• pkg-r/man/DataFrameSource.Rd: Generated file
• pkg-r/man/PinSource.Rd: Generated file

• Files reviewed: 13/15 changed files
• Comments generated: 0 new
• Review effort level: Lite