From e9edc94d0c81cfd5cef840da6243951bfecd0a8c Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 12:14:04 -0700 Subject: [PATCH 01/21] docs: spec for result column conformance & sanitization redesign Design for fixing the 11 confirmed findings from the adversarial GLM-5.2 review (adjudicated by GPT-5.5 xhigh) of range 204380d..c5814eb. Locked decisions: - Scope C: full PostgreSQL result-column conformance - Compat C2: GUC pgsqlite.legacy_result_columns off-switch (default conformant) - Approach 1: schema-canonical + lazy AST for aliases (zero-alloc fast path preserved) - Type T3: argument-preserving OID + datetime conversion side effect Addresses F1-F10 + F-new. --- ...-07-02-result-column-conformance-design.md | 217 ++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-02-result-column-conformance-design.md diff --git a/docs/superpowers/specs/2026-07-02-result-column-conformance-design.md b/docs/superpowers/specs/2026-07-02-result-column-conformance-design.md new file mode 100644 index 0000000..697954a --- /dev/null +++ b/docs/superpowers/specs/2026-07-02-result-column-conformance-design.md @@ -0,0 +1,217 @@ +# Result Column Conformance & Sanitization Redesign + +**Status:** Design (pending implementation plan) +**Date:** 2026-07-02 +**Scope:** Full PostgreSQL result-column-name & type conformance for the SELECT paths, fixing the issues found in the adversarial GLM-5.2 review (and the GPT-5.5 adjudication) of range `204380d..c5814eb`. + +## Background + +Commit `3b9c49f` ("strip function parentheses from result column names") added a `sanitize_column_name()` that truncates every result-column name at the first `(`, applied at ~10 sites in `src/session/db_handler.rs`. The feature's design doc argued this was safe because "real table columns never contain `(`." That premise is false: PostgreSQL quoted identifiers (e.g. `"price(usd)"`) may contain `(`, and SQLite returns them verbatim. The blind truncation introduced a cluster of regressions, and the associated bare-name type-inference block introduced another. + +An adversarial review (two GLM-5.2 reviewers, adjudicated by GPT-5.5 xhigh) confirmed 10 findings with 0 false positives, plus 1 additional issue GPT-5.5 found. This spec fixes all of them. + +## Goals + +1. **Full PostgreSQL conformance** for result column names and types on all SELECT paths (simple, extended/portal, read-only). +2. **Fix every confirmed finding** (F1–F10 + F-new). +3. **Backward-compat escape hatch** via a session GUC, defaulting to conformant behavior. +4. **No regression to the zero-alloc fast path** for trivial `SELECT *` / plain-column queries. + +## Non-goals + +- Lowercasing column *data* or identifiers in DDL (already handled at `CREATE TABLE` time via `__pgsqlite_schema`). +- A general "execute arbitrary aggregate against catalog" engine (deferred; see F-new scope). +- Changing non-SELECT result metadata (DML returning columns is out of scope). + +## Decisions (locked during brainstorming) + +| Decision | Choice | Meaning | +|---|---|---| +| Scope | **C — Full conformance** | Global lowercase-folding of unquoted result identifiers; `?column?` for unnamed expressions; quoted-case preservation; function names bare + lowercased. | +| Backward compat | **C2 — Cutover with GUC off-switch** | Default conformant; `pgsqlite.legacy_result_columns = on` opts back to legacy casing. The paren-corruption, empty-name, and datetime fixes are **not** behind the GUC. | +| Architecture | **Approach 1 — Schema-canonical + lazy AST for aliases** | Real columns & wildcards use schema-stored effective names (no parse). Function-call names use the raw SQLite name's shape. Only queries containing `AS` parse the projection (cached). | +| Type inference | **T3 — Argument-preserving + datetime side effect** | `min/max/sum` over a datetime inner column → datetime OID **and** `datetime_flag` so the existing converter formats microseconds. | + +## Architecture + +### The central resolver + +A new `ProjectionResolver` (in `src/query/projection_resolver.rs`) is the single place that turns SQLite's `column_name(i)` into PostgreSQL-correct metadata. Every SELECT path calls it instead of `sanitize_column_name` + ad-hoc type inference. + +``` +ProjectionResolver::resolve(raw_name, position, ctx) -> ColumnMeta +ColumnMeta { wire_name: String, type_oid: i32, datetime_flag: bool } +``` + +**Context (`ctx`) per query:** +1. `raw_name = stmt.column_name(i)` — SQLite's string (shape only: has `(`? empty?). +2. `schema_types: HashMap` — already built from `__pgsqlite_schema`. Holds the **effective PG name** (correct case) for real columns. Truth for real columns & wildcards. +3. Parsed projection alias view (only when the query contains `AS`) → per-position `(alias, is_quoted, source_expr)`. +4. `translation_metadata` hints (already produced by the translator) — carry `source_column` + `expression_type` for function calls. +5. Session GUC `pgsqlite.legacy_result_columns` (off = conform). + +### Resolution precedence (first match wins) + +1. **Schema match** on the *raw* name → real column. `wire_name` = schema-stored effective name (correct case), `type_oid` = schema type. *(F1)* +2. **Function-call shape** (raw name contains `(`): `wire_name` = bare lowercased function name; `type_oid` from the function→type table (argument-preserving for `min/max/sum/first/last` by resolving the inner column via schema or `translation_metadata`); if the resolved type is datetime, set `datetime_flag`. *(F2, F5)* +3. **Alias** (query has `AS`, position maps to an alias item): `wire_name` = quoted ? `alias` : `alias.to_lowercase()`; `type_oid` = source-expr type via schema or generic. *(F6)* +4. **Unnamed non-function expression**: `wire_name` = `?column?` (subsequent duplicates: `?column?2`, `?column?3`, …); `type_oid` from `translation_metadata` hint or TEXT. *(F4)* +5. **Fallback**: raw name (lowercased if GUC off) + TEXT. + +**GUC effect:** when `pgsqlite.legacy_result_columns = on`, steps 3 & 5 skip lowercasing. Steps 1, 2, 4 stay conformant regardless. + +### Data flow + +``` +client SQL + → process_query (translator) → translation_metadata (already produced) + → SQLite prepare + query + → for each column i: + raw_name = stmt.column_name(i) + ctx = { schema_types, translation_metadata, alias_view(query) [if AS], legacy_guc } + ColumnMeta = resolver.resolve(raw_name, i, ctx) + → DbResponse.columns = ColumnMeta.wire_name + → field_descriptions[i].type_oid = ColumnMeta.type_oid + → if ColumnMeta.datetime_flag: datetime_columns.insert(wire_name, pg_type) // feeds existing converter +``` + +### Path convergence (fixes F3) + +Three SELECT entry points today produce result metadata independently: +- `DbHandler` (multiple sites in `db_handler.rs`) — sanitized. +- `ReadOnlyDbHandler` (`read_only_handler.rs`) — unsanitized. +- Extended/portal (`extended.rs`) — its own AST walk. + +All three call one helper: `resolve_columns(stmt, query, schema_types, hints, session) -> Vec`. The extended path's existing AST walk becomes the *source* of the alias view rather than a parallel implementation. Output is identical across pooling on/off. + +### Alias view caching (the only parse) + +`alias_view(query)` parses the projection with `sqlparser` only when the query contains `AS`. The result is cached on `PreparedStatement` (in `src/session/state.rs`) as a new optional field: + +```rust +pub struct PreparedStatement { + // ... existing fields ... + pub projection_metadata: Option>, +} +``` + +`AliasItem { alias: String, is_quoted: bool, source_expr: Expr }` — `is_quoted` derived from `Ident::quote_style.is_some()`. + +Prepared-statement re-execution hits the cache; one-off simple queries without `AS` never parse, preserving the zero-alloc fast path. + +### `fn_shape` correctness (the F2 root) + +The current `direct_pattern` regex runs against the *sanitized* name (broken — parens stripped). The resolver's `fn_shape` runs against the **raw** SQLite name (`max(created_at)`), so the inner column is extractable and its schema type resolvable. This is the mechanical fix that makes T3 work. + +### Type table (T3) + +| Function | Generic OID | Argument-preserving? | +|---|---|---| +| `count` | int8 | no | +| `sum` | numeric | yes (int→int4/8, numeric, float→float8) | +| `avg` | numeric | no | +| `min` / `max` | (arg type) | **yes — datetime→datetime** | +| `json_extract`, `json_agg`, `row_to_json`, … | text | no | +| `array_length`, `array_upper`, … | int4 | no | +| `now` / `current_timestamp` | timestamptz | no | + +For `min/max/sum` over a datetime inner column, `datetime_flag = true` → the existing converter formats the microseconds. + +### Removals + +- `src/query/column_sanitizer.rs` (`sanitize_column_name`) — deleted; subsumed by the resolver. +- The bare-name OID block in `src/types/schema_type_mapper.rs:344+` — deleted; the resolver's function→type table replaces it. The existing query-regex path below it (`:379`) remains as a fallback for aliased aggregates resolved from the query text. + +## Catalog fixes (independent of the resolver) + +Localized to `src/catalog/query_interceptor.rs` and `src/catalog/pg_roles.rs`. + +### F7 — `break` on Wildcard drops trailing items +In `extract_selected_columns` (`query_interceptor.rs:1756-1761`) and `PgRolesHandler::get_selected_columns` (`pg_roles.rs:77-83`, `102-106`), the `Wildcard`/`QualifiedWildcard` arm does `cols.extend(...); break;`. +**Fix:** remove `break`; continue the loop. For `QualifiedWildcard`, extend with all columns of the *named* relation only. No dedupe — PostgreSQL emits duplicates for `SELECT *, x`. + +### F-new — catalog aggregate projections dropped +`extract_projection_source_column` (`query_interceptor.rs:1767`) handles `Identifier`/`CompoundIdentifier`/`Cast`/`Nested` but returns `None` for `Expr::Function`. So `SELECT count(*) FROM pg_catalog.pg_namespace` → `UnnamedExpr(Function)` → zero selected columns → empty result. +**Fix (minimal scope, locked):** add an `Expr::Function` arm resolving to the bare function name; when no catalog column matches, the handler detects a `count`-shaped aggregate on a catalog table and computes the row count of its static dataset (e.g. `pg_namespace` → 2). The broader "execute any aggregate verbatim against SQLite" path is deferred as future work. + +### F6 — alias case not folded +`query_interceptor.rs:1749-1754` and `pg_roles.rs:95-100` push `alias.value.clone()` verbatim. +**Fix:** push `alias.value.to_lowercase()` for **unquoted** aliases, preserve for quoted. In sqlparser 0.57 the `alias` in `ExprWithAlias` is an `Ident` carrying `quote_style: Option` (the codebase already reads `alias.value`); quoted iff `alias.quote_style.is_some()`. Under `pgsqlite.legacy_result_columns = on`, skip lowercasing (same GUC as the hot path). + +### F10 — `catalog_alias_test.rs` coverage gaps +Add tests for: uppercase aliases (F6), mid-list `SELECT *, oid` (F7), `SELECT count(*) FROM pg_catalog.pg_namespace` (F-new), `CAST`/`Nested` projections, `CompoundIdentifier` source, `WHERE` + alias interaction, unknown source column. + +## Hygiene + +### F8 — mislabeled debug log +`query_interceptor.rs:1082` (inside `handle_pg_namespace_query`) says `"pg_type query"`. → `"pg_namespace query"`. + +### F9 — stray test file +Delete `tests/test_batch_7b.py`. Confirmed unreferenced by any runner. + +## GUC mechanism + +`pgsqlite.legacy_result_columns` is a normal session parameter: +- **Stored** in `SessionState.parameters` (`HashMap`), the existing GUC store. +- **Set** via the existing `SetHandler::handle_set_command` (`src/query/set_handler.rs`), which already stores arbitrary `SET key = value`. +- **Read** by `ProjectionResolver` (hot path) and the catalog handlers (F6) via a session lookup; default `off` (conformant). +- No new infrastructure. + +## Testing + +### Unit tests — `src/query/projection_resolver.rs` (new, pure) +- Schema match wins over function shape: raw `"price(usd)"` in schema → keeps full name + schema type. *(F1)* +- `fn_shape`: `max(created_at)`→`("max","created_at")`, `count(*)`→`("count","*")`, `COALESCE(max(id),0)`→`("COALESCE",...)`. *(F2 root)* +- T3: `min` over timestamp → timestamp OID + `datetime_flag=true`; over int4 → int4; `count(*)` → int8. *(F2/F5)* +- Alias: `AS Foo`→`foo`, `AS "Foo"`→`Foo`; GUC on→`Foo`. *(F6)* +- Unnamed non-function expr → `?column?`, duplicate → `?column?2`. *(F4)* +- GUC off lowercases unquoted; on preserves. *(C2)* + +### Unit tests — catalog +- `extract_selected_columns` with `SELECT *, oid` → trailing `oid` retained. *(F7)* +- `extract_projection_source_column` on `Expr::Function(count)` → resolves. *(F-new)* +- `SELECT oid AS Did` → `did` (GUC off), `Did` (GUC on). *(F6)* + +### Integration tests (shell runner / Python, new SQL files) +- `CREATE TABLE t ("price(usd)" INT); SELECT "price(usd)" FROM t;` → column `price(usd)`, int4. *(F1)* +- `SELECT max(created_at) FROM t;` → formatted timestamp, not microseconds. *(F2)* +- `SELECT upper(name) AS count FROM t;` → column `count` typed **text**, decodes. *(F5)* +- `SELECT 1+1;` → column `?column?`. *(F4)* +- `SELECT count(*) FROM pg_catalog.pg_namespace;` → `2`. *(F-new)* +- `SELECT *, oid FROM pg_catalog.pg_namespace;` → 3 columns. *(F7)* +- `SET pgsqlite.legacy_result_columns = on; SELECT MyCol FROM t;` → `MyCol` (legacy); default → `mycol`. *(C2)* + +### Existing-test update +The casing default changes, so existing tests asserting as-written unquoted column case get updated to the lowercased expectation. The GUC-on path is used only in a dedicated legacy-compat test — not to keep the old suite passing. + +### Regression guard +The F2 repro (`max(timestamp)` returns raw microseconds) becomes a test that fails on current HEAD and passes after the fix. + +## Findings addressed + +| Finding | Severity (adjudicated) | Fixed by | +|---|---|---| +| F1 — sanitize corrupts paren-columns + type lookup | Important | Resolver precedence step 1 (schema match keeps raw name) | +| F2 — `MAX/MIN(datetime)` → raw microseconds | Important | `fn_shape` on raw name + T3 datetime_flag | +| F3 — ReadOnly vs DbHandler name divergence | Minor (config-path) | Path convergence via single `resolve_columns` helper | +| F4 — leading `(` → empty wire name | Minor | Resolver step 4 (`?column?`) | +| F5 — bare-name OID block mis-types aliases | Important | Delete bare block; resolver function→type table from expression, not alias name | +| F6 — alias case not folded | Minor | Resolver step 3 + catalog `quoted` flag | +| F7 — `break` on wildcard drops trailing items | Minor | Remove `break` in both catalog extractors | +| F8 — mislabeled debug log | Trivial | One-line log fix | +| F9 — stray `tests/test_batch_7b.py` | Minor | Delete | +| F10 — `catalog_alias_test.rs` gaps | Minor (test-only) | New tests | +| F-new — catalog aggregate projection dropped | Important | `Expr::Function` arm + `count(*)` dataset row count | + +## Risks & mitigations + +- **Casing default change breaks a client.** Mitigated by `pgsqlite.legacy_result_columns = on` (C2). +- **Parse cost on aliased queries.** Mitigated by caching `projection_metadata` on `PreparedStatement`; non-`AS` queries never parse. +- **T3 inner-column resolution miss.** Falls back to the function's generic OID (e.g. `max`→TEXT); documented behavior. +- **Catalog `count(*)` minimal path misses non-count aggregates.** Accepted; broader path deferred (future work). + +## Out of scope / future work + +- General catalog aggregate execution engine (F-new broader path). +- Lowercasing consistency audit for DDL identifiers. +- Migrating the remaining catalog handlers off `get_mut_connection` (pre-existing known limitation). From fe52bfef5094a581d9c7f7c19abf2c8b12e573e8 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 12:24:00 -0700 Subject: [PATCH 02/21] docs: implementation plan for result column conformance 10-task TDD plan implementing the spec in 2026-07-02-result-column-conformance-design.md. Covers all 11 findings (F1-F10 + F-new) with bite-sized test-first steps. --- .../2026-07-02-result-column-conformance.md | 976 ++++++++++++++++++ 1 file changed, 976 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-02-result-column-conformance.md diff --git a/docs/superpowers/plans/2026-07-02-result-column-conformance.md b/docs/superpowers/plans/2026-07-02-result-column-conformance.md new file mode 100644 index 0000000..137e6a3 --- /dev/null +++ b/docs/superpowers/plans/2026-07-02-result-column-conformance.md @@ -0,0 +1,976 @@ +# Result Column Conformance Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make pgsqlite's SELECT result column names and types fully PostgreSQL-conformant, fixing the 11 confirmed findings (F1–F10 + F-new) from the adversarial review of range `204380d..c5814eb`. + +**Architecture:** A central `ProjectionResolver` turns SQLite's `column_name(i)` into `(wire_name, type_oid, datetime_flag)` using schema-stored effective names as the truth for real columns and parsing the projection AST only for queries containing `AS` (cached on `PreparedStatement`). All three SELECT paths (`DbHandler`, `ReadOnlyDbHandler`, extended/portal) converge on it. A session GUC `pgsqlite.legacy_result_columns` (default off) toggles legacy casing. Catalog extractors get localized fixes. + +**Tech Stack:** Rust, sqlparser 0.57 (`Ident.quote_style: Option`), rusqlite, tokio (`RwLock`), regex. Tests via `cargo test` (unit) and `tests/runner/run_ssl_tests.sh` + `tests/sql//*.sql` (integration). + +## Global Constraints + +- Pre-commit checklist (run before any commit): `cargo check` (no warnings), `cargo clippy`, `cargo build`, `cargo test` (all pass). +- Project rule: NEVER infer types from column names — only explicit PG type declarations, PRAGMA `table_info`, explicit casts, value-based inference as last resort. The resolver's schema match (step 1) and argument-preserving function typing (step 2) honor this. +- DateTime storage: DATE=days, TIME/TIMETZ=micros-since-midnight, TIMESTAMP/TIMESTAMPTZ=micros-since-epoch, INTERVAL=micros (INTEGER). The resolver's `datetime_flag` feeds the existing converter. +- Zero-perf-impact design: non-`AS` queries must never parse sqlparser; alias view is cached on `PreparedStatement`. +- GUC `pgsqlite.legacy_result_columns` stored in `SessionState.parameters` (existing `RwLock>`), set via existing `SetHandler`, default `off` (conformant). The paren-corruption, empty-name, and datetime fixes are NOT behind the GUC — only casing in steps 3 & 5. +- Spec: `docs/superpowers/specs/2026-07-02-result-column-conformance-design.md`. + +--- + +## File Structure + +**Created:** +- `src/query/projection_resolver.rs` — `ProjectionResolver`, `ColumnMeta`, `AliasItem`, `fn_shape`, function→type table. The single convergence point. Pure logic, no DB/async. +- `tests/sql/features/column_conformance.sql` — integration SQL tests for F1/F2/F5/F4/F-new/F7/C2. + +**Modified:** +- `src/session/state.rs` — add `projection_metadata: Option>` to `PreparedStatement`; add a `legacy_result_columns()` reader helper. +- `src/session/db_handler.rs` — replace the ~10 `sanitize_column_name(stmt.column_name(i)?)` sites with `resolve_columns(...)`; thread `session`/GUC into the executor call. +- `src/session/read_only_handler.rs` — route column construction through `resolve_columns` (fixes F3). +- `src/query/executor.rs` — use `ColumnMeta` from the resolver to set `field_descriptions[i].type_oid` and populate `datetime_columns` (fixes F2); remove the bare-name OID block dependency. +- `src/types/schema_type_mapper.rs` — delete the bare-name OID block at `:344+`; keep the query-regex fallback below it (fixes F5). +- `src/query/mod.rs` — `pub mod projection_resolver;` +- `src/catalog/query_interceptor.rs` — F7 (remove `break`), F-new (`Expr::Function` arm + `count(*)` dataset count), F6 (case-fold via `quote_style`), F8 (log label). +- `src/catalog/pg_roles.rs` — F7 (remove `break`), F6 (case-fold via `quote_style`). +- `tests/catalog_alias_test.rs` — add F6/F7/F-new/CAST/CompoundIdentifier/WHERE/unknown-source tests (fixes F10). +- `src/query/column_sanitizer.rs` — delete (last task, after all callers migrated). + +--- + +### Task 1: GUC plumbing — `legacy_result_columns` reader + default + +**Files:** +- Modify: `src/session/state.rs` (struct `SessionState` at `:21`, `new()` at `:50`) +- Test: `src/session/state.rs` (new `#[cfg(test)]` module) + +**Interfaces:** +- Produces: `impl SessionState { pub async fn legacy_result_columns(&self) -> bool }` — reads `self.parameters`, returns `true` iff the key `"pgsqlite.legacy_result_columns"` exists with value `"on"` (case-insensitive). Default `false` (conformant). + +- [ ] **Step 1: Write the failing test** + +Add to `src/session/state.rs`: + +```rust +#[cfg(test)] +mod legacy_guc_tests { + use super::*; + + #[tokio::test] + async fn test_legacy_default_off() { + let s = SessionState::new("db".into(), "user".into()); + assert!(!s.legacy_result_columns().await); + } + + #[tokio::test] + async fn test_legacy_on_when_set_on() { + let s = SessionState::new("db".into(), "user".into()); + s.parameters.write().await.insert( + "pgsqlite.legacy_result_columns".to_string(), "on".to_string()); + assert!(s.legacy_result_columns().await); + } + + #[tokio::test] + async fn test_legacy_off_explicit() { + let s = SessionState::new("db".into(), "user".into()); + s.parameters.write().await.insert( + "pgsqlite.legacy_result_columns".to_string(), "off".to_string()); + assert!(!s.legacy_result_columns().await); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib session::state::legacy_guc_tests` +Expected: FAIL — `legacy_result_columns` method not found. + +- [ ] **Step 3: Write minimal implementation** + +Add to `impl SessionState` in `src/session/state.rs` (after `new`): + +```rust + /// Read the legacy result-column GUC. Default off (conformant). + /// Only casing in resolver steps 3 & 5 honors this; the paren-corruption, + /// empty-name, and datetime fixes are always applied. + pub async fn legacy_result_columns(&self) -> bool { + self.parameters.read().await + .get("pgsqlite.legacy_result_columns") + .map(|v| v.eq_ignore_ascii_case("on")) + .unwrap_or(false) + } +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib session::state::legacy_guc_tests` +Expected: PASS (3 tests). + +- [ ] **Step 5: Pre-commit + commit** + +```bash +cargo check && cargo clippy --quiet && cargo test --lib session::state::legacy_guc_tests +git add src/session/state.rs +git commit -m "feat: add pgsqlite.legacy_result_columns GUC reader (default off)" +``` + +--- + +### Task 2: `ColumnMeta` + `fn_shape` + function→type table (pure, no parse) + +**Files:** +- Create: `src/query/projection_resolver.rs` +- Modify: `src/query/mod.rs` (add `pub mod projection_resolver;`) +- Test: `src/query/projection_resolver.rs` (in-file `#[cfg(test)]`) + +**Interfaces:** +- Produces: + - `pub struct ColumnMeta { pub wire_name: String, pub type_oid: i32, pub datetime_flag: bool }` + - `pub fn fn_shape(raw_name: &str) -> Option` where `pub struct FnShape { pub name: String, pub inner: String }` — parses SQLite result names: `count(*)`→`{name:"count", inner:"*"}`, `max(created_at)`→`{name:"max", inner:"created_at"}`, `COALESCE(max(id), 0)`→`{name:"COALESCE", inner:"max(id), 0"}`. Returns `None` when no `(` present. + - `pub fn function_generic_oid(fn_name_lower: &str) -> Option` — the T3 generic table (count→int8, avg→numeric, json_extract→text, array_length→int4, now→timestamptz, …). Returns `None` for `min`/`max`/`sum` (argument-preserving — handled by the resolver with schema) and unknown names. + - `pub fn is_arg_preserving(fn_name_lower: &str) -> bool` — true for `min`/`max`/`sum`/`first`/`last`. +- Consumes: `crate::types::PgType` for OIDs. + +- [ ] **Step 1: Write the failing tests** + +Create `src/query/projection_resolver.rs`: + +```rust +use crate::types::PgType; + +#[derive(Debug, Clone, PartialEq)] +pub struct ColumnMeta { + pub wire_name: String, + pub type_oid: i32, + pub datetime_flag: bool, +} + +#[derive(Debug, Clone, PartialEq)] +pub struct FnShape { + pub name: String, + pub inner: String, +} + +/// Parse a SQLite result-column name into function name + inner argument text. +/// `count(*)` -> { count, * }; `max(created_at)` -> { max, created_at }; +/// `COALESCE(max(id), 0)` -> { COALESCE, "max(id), 0" }. +/// Returns None when the name contains no `(`. +pub fn fn_shape(raw_name: &str) -> Option { + let open = raw_name.find('(')?; + let last_close = raw_name.rfind(')')?; + if last_close <= open { return None; } + Some(FnShape { + name: raw_name[..open].to_string(), + inner: raw_name[open + 1..last_close].to_string(), + }) +} + +/// T3 generic return OID for functions whose type does not depend on the argument. +pub fn function_generic_oid(fn_name_lower: &str) -> Option { + Some(match fn_name_lower { + "count" => PgType::Int8.to_oid(), + "avg" => PgType::Numeric.to_oid(), + "json_extract" | "json_agg" | "jsonb_agg" | "json_object" | "json_object_agg" + | "jsonb_object_agg" | "row_to_json" | "json_array" | "json_group_array" + | "json_extract_path" | "json_extract_path_text" => PgType::Text.to_oid(), + "array_length" | "array_upper" | "array_lower" | "array_ndims" + | "array_position" => PgType::Int4.to_oid(), + "array_append" | "array_prepend" | "array_cat" | "array_remove" + | "array_replace" | "array_slice" | "string_to_array" | "array_positions" + | "array_to_string" | "unnest" | "array_agg" => PgType::Text.to_oid(), + "array_contains" | "array_contained" | "array_overlap" => PgType::Bool.to_oid(), + "now" | "current_timestamp" => PgType::Timestamptz.to_oid(), + "current_date" => PgType::Text.to_oid(), + "current_time" => PgType::Time.to_oid(), + "extract" => PgType::Float8.to_oid(), + "date_trunc" | "to_timestamp" => PgType::Timestamp.to_oid(), + "make_date" => PgType::Date.to_oid(), + "make_time" => PgType::Time.to_oid(), + "age" => PgType::Interval.to_oid(), + "decimal_add" | "decimal_sub" | "decimal_mul" | "decimal_div" + | "decimal_from_text" => PgType::Numeric.to_oid(), + _ => return None, + }) +} + +/// min/max/sum/first/last take the argument column's type (T3). +pub fn is_arg_preserving(fn_name_lower: &str) -> bool { + matches!(fn_name_lower, "min" | "max" | "sum" | "first" | "last") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn fn_shape_count_star() { + assert_eq!(fn_shape("count(*)"), Some(FnShape { name: "count".into(), inner: "*".into() })); + } + #[test] + fn fn_shape_max() { + assert_eq!(fn_shape("max(created_at)"), Some(FnShape { name: "max".into(), inner: "created_at".into() })); + } + #[test] + fn fn_shape_coalesce_nested() { + let s = fn_shape("COALESCE(max(id), 0)").unwrap(); + assert_eq!(s.name, "COALESCE"); + assert_eq!(s.inner, "max(id), 0"); + } + #[test] + fn fn_shape_no_parens_is_none() { + assert!(fn_shape("my_column").is_none()); + assert!(fn_shape("").is_none()); + } + #[test] + fn generic_oid_count_int8() { + assert_eq!(function_generic_oid("count"), Some(PgType::Int8.to_oid())); + } + #[test] + fn generic_oid_min_max_none() { + assert!(function_generic_oid("min").is_none()); + assert!(function_generic_oid("max").is_none()); + assert!(function_generic_oid("sum").is_none()); + } + #[test] + fn arg_preserving_flags() { + assert!(is_arg_preserving("max") && is_arg_preserving("sum")); + assert!(!is_arg_preserving("count")); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib query::projection_resolver` +Expected: FAIL — module not declared in `mod.rs`. + +- [ ] **Step 3: Register the module** + +In `src/query/mod.rs` add: + +```rust +pub mod projection_resolver; +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib query::projection_resolver` +Expected: PASS (7 tests). + +- [ ] **Step 5: Pre-commit + commit** + +```bash +cargo check && cargo clippy --quiet && cargo test --lib query::projection_resolver +git add src/query/projection_resolver.rs src/query/mod.rs +git commit -m "feat: add ColumnMeta, fn_shape, and function->type table" +``` + +--- + +### Task 3: `AliasItem` + cached alias-view parse on `PreparedStatement` + +**Files:** +- Modify: `src/query/projection_resolver.rs` (add `AliasItem`, `parse_alias_view`) +- Modify: `src/session/state.rs` (struct `PreparedStatement` at `:32`, `new()` helper if any) +- Test: `src/query/projection_resolver.rs` + +**Interfaces:** +- Produces: + - `pub struct AliasItem { pub position: usize, pub alias: String, pub is_quoted: bool, pub source_expr: sqlparser::ast::Expr }` — `is_quoted` from `Ident::quote_style.is_some()`. + - `pub fn parse_alias_view(query: &str) -> Option>` — returns `None` when the query contains no `AS` (case-insensitive). Otherwise parses with `sqlparser::parser::Parser::parse_sql(&PostgreSqlDialect{}, query)`, walks `Statement::Query` → `SetExpr::Select`, and for each `SelectItem::ExprWithAlias { expr, alias }` pushes one `AliasItem` (position = index into projection). Returns `Some(vec![])` if parsed but no aliases found; `None` on parse error treated as `None` (resolver falls back to no-alias path). +- Consumes: `sqlparser` (already a dependency), `sqlparser::dialect::PostgreSqlDialect`, `sqlparser::ast::{Statement, SetExpr, SelectItem, Expr}`. + +- [ ] **Step 1: Write the failing tests** + +Append to `src/query/projection_resolver.rs` tests module: + +```rust + #[test] + fn alias_view_none_when_no_as() { + assert!(parse_alias_view("SELECT id, name FROM users").is_none()); + } + #[test] + fn alias_view_unquoted_alias() { + let v = parse_alias_view("SELECT id AS user_id FROM users").unwrap(); + assert_eq!(v.len(), 1); + assert_eq!(v[0].alias, "user_id"); + assert!(!v[0].is_quoted); + } + #[test] + fn alias_view_quoted_alias_preserved() { + let v = parse_alias_view(r#"SELECT id AS "UserId" FROM users"#).unwrap(); + assert_eq!(v.len(), 1); + assert_eq!(v[0].alias, "UserId"); + assert!(v[0].is_quoted); + } + #[test] + fn alias_view_position_indexed() { + let v = parse_alias_view("SELECT id AS a, name AS b FROM users").unwrap(); + assert_eq!(v[0].position, 0); + assert_eq!(v[1].position, 1); + } +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib query::projection_resolver::tests::alias_view` +Expected: FAIL — `parse_alias_view` / `AliasItem` not found. + +- [ ] **Step 3: Implement** + +Add to `src/query/projection_resolver.rs` (top, after `use crate::types::PgType;`): + +```rust +use sqlparser::ast::{Expr, SelectItem, SetExpr, Statement}; +use sqlparser::dialect::PostgreSqlDialect; +use sqlparser::parser::Parser; + +#[derive(Debug, Clone)] +pub struct AliasItem { + pub position: usize, + pub alias: String, + pub is_quoted: bool, + pub source_expr: Expr, +} + +/// Parse the projection alias view. Returns None when the query has no `AS` +/// (so the zero-alloc fast path never pays a parse). On parse error, None. +pub fn parse_alias_view(query: &str) -> Option> { + if !query.to_uppercase().contains(" AS ") { return None; } + let parsed = Parser::parse_sql(&PostgreSqlDialect {}, query).ok()?; + let stmt = parsed.into_iter().next()?; + let body = match stmt { Statement::Query(q) => q, _ => return None }; + let select = match &*body.body { SetExpr::Select(s) => s, _ => return None }; + let items: Vec = select.projection.iter().enumerate() + .filter_map(|(i, item)| match item { + SelectItem::ExprWithAlias { expr, alias } => Some(AliasItem { + position: i, + alias: alias.value.clone(), + is_quoted: alias.quote_style.is_some(), + source_expr: expr.clone(), + }), + _ => None, + }) + .collect(); + Some(items) +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib query::projection_resolver` +Expected: PASS (all 11 tests). + +- [ ] **Step 5: Add cache field to `PreparedStatement`** + +In `src/session/state.rs`, struct `PreparedStatement`, add after `translation_metadata`: + +```rust + pub projection_metadata: Option>, +``` + +Find every `PreparedStatement { ... }` construction site (search `PreparedStatement {`); add `projection_metadata: None,` to each. (These are typically in prepared-statement registration; verify with `grep -rn "PreparedStatement {" src/`.) + +- [ ] **Step 6: Pre-commit + commit** + +```bash +cargo check && cargo clippy --quiet && cargo test --lib query::projection_resolver +git add src/query/projection_resolver.rs src/session/state.rs +git commit -m "feat: add AliasItem + cached alias-view parse (AS-gated)" +``` + +--- + +### Task 4: `ProjectionResolver::resolve` — the precedence core + +**Files:** +- Modify: `src/query/projection_resolver.rs` (add `ProjectionResolver`, `ResolveCtx`) +- Test: `src/query/projection_resolver.rs` + +**Interfaces:** +- Produces: + - `pub struct ResolveCtx<'a> { pub schema_types: &'a HashMap, pub hints: &'a crate::translator::TranslationMetadata, pub alias_view: Option<&'a [AliasItem]>, pub legacy: bool }` + - `pub struct ProjectionResolver;` + - `impl ProjectionResolver { pub fn resolve(raw_name: &str, position: usize, ctx: &ResolveCtx) -> ColumnMeta }` + - The 5-step precedence (schema match → function shape → alias → unnamed `?column?` → fallback). Argument-preserving fn types resolved via `ctx.schema_types` keyed by the inner column name; datetime iff resolved type uppercased contains `TIMESTAMP`/`DATE`/`TIME`. `?column?` duplicate naming: track a static `usize` counter within a single `resolve_columns` call (see Task 5), NOT a global — pass the running count via `position`-relative dedup in Task 5. For the unit test here, test `?column?` for the first unnamed expr and accept that duplicate numbering is wired in Task 5. +- Consumes: `crate::translator::TranslationMetadata` (`get_hint(name) -> Option` with `Hint { source_column: Option, expression_type: Option, suggested_type: Option }`), `crate::types::SchemaTypeMapper::pg_type_string_to_oid(&str) -> i32`. + +- [ ] **Step 1: Write the failing tests** + +Append to the tests module: + +```rust + use std::collections::HashMap; + use crate::translator::TranslationMetadata; + + fn ctx_no_aliases<'a>(schema: &'a HashMap, legacy: bool) -> ResolveCtx<'a> { + static EMPTY: once_cell::sync::Lazy = once_cell::sync::Lazy::new(TranslationMetadata::new); + ResolveCtx { schema_types: schema, hints: &EMPTY, alias_view: None, legacy } + } + + #[test] + fn schema_match_keeps_paren_name() { + let mut schema = HashMap::new(); + schema.insert("price(usd)".to_string(), "INT4".to_string()); + let m = ProjectionResolver::resolve("price(usd)", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "price(usd)"); + assert_eq!(m.type_oid, PgType::Int4.to_oid()); + } + #[test] + fn function_shape_lowers_and_types() { + let schema = HashMap::new(); + let m = ProjectionResolver::resolve("count(*)", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "count"); + assert_eq!(m.type_oid, PgType::Int8.to_oid()); + assert!(!m.datetime_flag); + } + #[test] + fn min_over_timestamp_is_datetime() { + let mut schema = HashMap::new(); + schema.insert("created_at".to_string(), "TIMESTAMP".to_string()); + let m = ProjectionResolver::resolve("max(created_at)", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "max"); + assert_eq!(m.type_oid, PgType::Timestamp.to_oid()); + assert!(m.datetime_flag); + } + #[test] + fn min_over_int4_no_datetime() { + let mut schema = HashMap::new(); + schema.insert("qty".to_string(), "INT4".to_string()); + let m = ProjectionResolver::resolve("max(qty)", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.type_oid, PgType::Int4.to_oid()); + assert!(!m.datetime_flag); + } + #[test] + fn unnamed_expr_gets_question_column() { + let schema = HashMap::new(); + let m = ProjectionResolver::resolve("1+1", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "?column?"); + } +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib query::projection_resolver::tests` +Expected: FAIL — `ProjectionResolver`/`ResolveCtx` not found. + +- [ ] **Step 3: Implement the resolver** + +Add to `src/query/projection_resolver.rs`: + +```rust +use std::collections::HashMap; +use crate::translator::TranslationMetadata; +use crate::types::SchemaTypeMapper; + +pub struct ResolveCtx<'a> { + pub schema_types: &'a HashMap, + pub hints: &'a TranslationMetadata, + pub alias_view: Option<&'a [AliasItem]>, + pub legacy: bool, +} + +pub struct ProjectionResolver; + +impl ProjectionResolver { + pub fn resolve(raw_name: &str, position: usize, ctx: &ResolveCtx) -> ColumnMeta { + // Step 1: schema match on the raw name (real column — correct case already). + if let Some(pg_type) = ctx.schema_types.get(raw_name) { + return ColumnMeta { + wire_name: raw_name.to_string(), + type_oid: SchemaTypeMapper::pg_type_string_to_oid(pg_type), + datetime_flag: is_datetime_type(pg_type), + }; + } + + // Step 2: function-call shape (raw name contains `(`). + if let Some(shape) = fn_shape(raw_name) { + let lower = shape.name.to_lowercase(); + let wire_name = if ctx.legacy { shape.name.clone() } else { lower.clone() }; + let (type_oid, datetime_flag) = resolve_function_type(&lower, &shape.inner, ctx); + return ColumnMeta { wire_name, type_oid, datetime_flag }; + } + + // Step 3: alias (position maps to an ExprWithAlias item). + if let Some(view) = ctx.alias_view { + if let Some(item) = view.iter().find(|a| a.position == position) { + let wire_name = if item.is_quoted || ctx.legacy { + item.alias.clone() + } else { + item.alias.to_lowercase() + }; + let type_oid = ctx.hints.get_hint(&item.alias) + .and_then(|h| h.suggested_type.map(|t| t.to_oid())) + .unwrap_or(PgType::Text.to_oid()); + return ColumnMeta { wire_name, type_oid, datetime_flag: false }; + } + } + + // Step 4: unnamed non-function expression -> ?column?. + if raw_name.is_empty() || looks_unnamed_expr(raw_name) { + return ColumnMeta { + wire_name: "?column?".to_string(), + type_oid: ctx.hints.get_hint(raw_name) + .and_then(|h| h.suggested_type.map(|t| t.to_oid())) + .unwrap_or(PgType::Text.to_oid()), + datetime_flag: false, + }; + } + + // Step 5: fallback — raw name, lowercased unless legacy. + let wire_name = if ctx.legacy { raw_name.to_string() } else { raw_name.to_lowercase() }; + ColumnMeta { wire_name, type_oid: PgType::Text.to_oid(), datetime_flag: false } + } +} + +fn is_datetime_type(pg_type: &str) -> bool { + let u = pg_type.to_uppercase(); + u.contains("TIMESTAMP") || u.contains("DATE") || u.contains("TIME") +} + +fn looks_unnamed_expr(name: &str) -> bool { + // SQLite emits the verbatim expression text for unaliased expressions. + // Heuristic: contains an operator, a space, or is purely numeric/literal. + name.chars().any(|c| matches!(c, '+'|'-'|'*'|'/'|' '|'=')) || name.parse::().is_ok() +} + +fn resolve_function_type(lower_fn: &str, inner: &str, ctx: &ResolveCtx) -> (i32, bool) { + if is_arg_preserving(lower_fn) { + if let Some(pg_type) = ctx.schema_types.get(inner.trim()) { + let oid = SchemaTypeMapper::pg_type_string_to_oid(pg_type); + return (oid, is_datetime_type(pg_type)); + } + // sum generic fallback is numeric; min/max fallback is text. + return (function_generic_oid(lower_fn).unwrap_or(PgType::Text.to_oid()), false); + } + (function_generic_oid(lower_fn).unwrap_or(PgType::Text.to_oid()), false) +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib query::projection_resolver` +Expected: PASS (all tests, incl. the 5 new ones). + +- [ ] **Step 5: Pre-commit + commit** + +```bash +cargo check && cargo clippy --quiet && cargo test --lib query::projection_resolver +git add src/query/projection_resolver.rs +git commit -m "feat: ProjectionResolver precedence core (schema>fn>alias>?column?>fallback)" +``` + +--- + +### Task 5: `resolve_columns` helper + `?column?N` dedup + integration hook + +**Files:** +- Modify: `src/query/projection_resolver.rs` (add `resolve_columns`) +- Modify: `src/session/db_handler.rs` (one representative site: `:1721-1725`) +- Test: `src/query/projection_resolver.rs` + +**Interfaces:** +- Produces: `pub async fn resolve_columns(stmt: &rusqlite::Statement, query: &str, schema_types: &HashMap, hints: &TranslationMetadata, session: &crate::session::SessionState) -> anyhow::Result>` — reads `stmt.column_name(i)` for each column, reads `session.legacy_result_columns().await`, fetches/caches alias view via `parse_alias_view(query)`, calls `ProjectionResolver::resolve` per column, then applies `?column?N` dedup (second+ unnamed → `?column?2`, `?column?3`, …). +- Consumes: `rusqlite::Statement::column_name`/`column_count`, `SessionState::legacy_result_columns`. + +- [ ] **Step 1: Write the failing test** + +Append to tests module: + +```rust + #[test] + fn question_column_dedup_numbering() { + // Two unnamed expressions at positions 0 and 1 should both yield ?column? + // in the single-column resolve() (dedup happens at resolve_columns level), + // but resolve_columns is async+DB-bound; tested via integration in Task 8. + // Here we only assert the base name for one unnamed expr: + let schema = HashMap::new(); + let m = ProjectionResolver::resolve("a+b", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "?column?"); + } +``` + +- [ ] **Step 2: Run test to verify it fails/passes baseline** + +Run: `cargo test --lib query::projection_resolver::tests::question_column` +Expected: PASS (baseline — the real dedup is integration-tested in Task 8). + +- [ ] **Step 3: Implement `resolve_columns`** + +Add to `src/query/projection_resolver.rs`: + +```rust +use anyhow::Result; + +/// Resolve all columns of a prepared statement into PG-conformant metadata. +/// Convergence point for DbHandler, ReadOnlyDbHandler, and extended paths. +pub async fn resolve_columns( + stmt: &rusqlite::Statement, + query: &str, + schema_types: &HashMap, + hints: &TranslationMetadata, + session: &crate::session::SessionState, +) -> Result> { + let legacy = session.legacy_result_columns().await; + let alias_view = parse_alias_view(query); + let view_ref = alias_view.as_deref(); + let count = stmt.column_count(); + let mut out = Vec::with_capacity(count); + let mut qcol_seen: std::collections::HashMap = std::collections::HashMap::new(); + for i in 0..count { + let raw = stmt.column_name(i)?.to_string(); + let ctx = ResolveCtx { schema_types, hints, alias_view: view_ref, legacy }; + let mut meta = ProjectionResolver::resolve(&raw, i, &ctx); + if meta.wire_name == "?column?" { + let n = qcol_seen.entry("?column?".to_string()).or_insert(0); + *n += 1; + if *n > 1 { meta.wire_name = format!("?column?{}", *n); } + } + out.push(meta); + } + Ok(out) +} +``` + +- [ ] **Step 4: Migrate ONE representative db_handler.rs site** + +In `src/session/db_handler.rs` at `:1721-1725` replace: + +```rust + let mut columns = Vec::with_capacity(column_count); + for i in 0..column_count { + columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); + } +``` +with (note: this site is inside `execute_with_session`; `session`/`schema_types` availability is verified in Task 6 — for this task, if the site lacks `session`, leave the migration to Task 6 and only commit `resolve_columns`): + +```rust + let metas = crate::query::projection_resolver::resolve_columns( + &stmt, &processed_query, &schema_types, &translation_metadata, session).await?; + let mut columns: Vec = metas.iter().map(|m| m.wire_name.clone()).collect(); +``` +If `session`/`schema_types`/`translation_metadata` are not in scope at this site, revert this edit and commit only the new `resolve_columns` function (Task 6 does the full migration). Document in the commit which. + +- [ ] **Step 5: Pre-commit + commit** + +```bash +cargo check && cargo clippy --quiet && cargo test --lib query::projection_resolver +git add src/query/projection_resolver.rs src/session/db_handler.rs +git commit -m "feat: resolve_columns helper + ?column?N dedup (first site migrated if in scope)" +``` + +--- + +### Task 6: Migrate all `db_handler.rs` sanitize sites + read_only_handler.rs (fixes F3) + +**Files:** +- Modify: `src/session/db_handler.rs` (sites `:530, :665, :778, :896, :1243, :1454, :1723, :2327, :2425`) +- Modify: `src/session/read_only_handler.rs` (`:72`, `:120`) +- Test: `cargo build` + `cargo test` (existing suite stays green) + +**Interfaces:** +- Consumes: `resolve_columns` (Task 5), `ColumnMeta`. +- Produces: all SELECT paths emit `columns` via the resolver; `read_only_handler` uses the same helper. + +- [ ] **Step 1: Inventory and characterize each site** + +Run: `grep -n "sanitize_column_name" src/session/db_handler.rs src/session/read_only_handler.rs` +For each site, note whether `session: &Arc`, `schema_types`, and `translation_metadata` are in scope. Most `db_handler` SELECT sites run inside `execute_with_session(session_id, ...)` closures where `session` is available via `self.get_session(session_id)`; where it is not, add a minimal fetch. `read_only_handler.rs:72,120` build `column_names` directly — these need `session` threaded in from the caller (the `QueryRouter` path has it). + +- [ ] **Step 2: Migrate db_handler.rs sites** + +For each `columns.push(sanitize_column_name(stmt.column_name(i)?).to_string());` loop, replace with: + +```rust + let metas = crate::query::projection_resolver::resolve_columns( + &stmt, query, &schema_types, &translation_metadata, session).await?; + let mut columns: Vec = metas.iter().map(|m| m.wire_name.clone()).collect(); +``` +Where `translation_metadata` is not built at that site, construct `crate::translator::TranslationMetadata::new()` as the hints source (the resolver handles empty hints). Where `schema_types` is not built, pass an empty `HashMap::new()`. Where `session` is not in scope, fetch via `self.get_session(&session_id).await` (existing helper) and pass `&session`. + +- [ ] **Step 3: Migrate read_only_handler.rs** + +In `src/session/read_only_handler.rs`, change `query` (and the analogous second method) to accept `session: &Arc` and build column names via `resolve_columns(&stmt, query, &HashMap::new(), &TranslationMetadata::new(), session).await?` instead of `stmt.column_names().map(|s| s.to_string())`. Update the `QueryRouter` call site to pass `session`. + +- [ ] **Step 4: Build + run full unit suite** + +Run: `cargo build && cargo test --lib` +Expected: BUILD OK; all existing tests PASS (casing may change for some — update assertions in the same commit to the lowercased expectation, since the default is now conformant). + +- [ ] **Step 5: Pre-commit + commit** + +```bash +cargo check && cargo clippy --quiet && cargo build && cargo test --lib +git add src/session/db_handler.rs src/session/read_only_handler.rs +git commit -m "refactor: route all SELECT paths through resolve_columns (fixes F3)" +``` + +--- + +### Task 7: Wire `ColumnMeta.type_oid` + `datetime_flag` into the executor (fixes F2/F5) + +**Files:** +- Modify: `src/query/executor.rs` (`:1191` field build, `:1289-1422` datetime_columns population) +- Modify: `src/types/schema_type_mapper.rs` (delete bare-name OID block `:344-371`) +- Test: `src/query/executor.rs` (new `#[cfg(test)]` for the type path) + `cargo test` + +**Interfaces:** +- Consumes: `resolve_columns` output (`Vec`) — the executor should receive it (passed from the db_handler site, or re-resolved). Decision: the `execute_select` path that builds `field_descriptions` (`:1191`) receives `metas: &[ColumnMeta]` from the caller and uses `metas[i].type_oid` directly instead of the 5-priority `if/else` chain, and inserts into `datetime_columns` when `metas[i].datetime_flag`. + +- [ ] **Step 1: Write the failing test for the datetime conversion (regression guard for F2)** + +Create `tests/sql/features/column_conformance.sql` with (integration; run in Task 8). For the unit guard, add to `src/query/executor.rs` a test asserting that when a `ColumnMeta` has `datetime_flag=true`, the column name is inserted into `datetime_columns`. If `execute_select` internals are hard to unit-test, defer this assertion to the integration test in Task 8 and here only assert the schema_type_mapper bare block removal (next step). + +- [ ] **Step 2: Delete the bare-name OID block (fixes F5)** + +In `src/types/schema_type_mapper.rs`, delete lines `:344-371` (the `match upper.as_str() { "COUNT" => ... }` block added by commit `3b9c49f`), keeping the surrounding `if !function_name.contains('(') && !function_name.contains(' ') {` guard and the query-regex fallback at `:379+`. After deletion, the bare-name path falls through to the query-regex (which reads the *query* text, not the alias name) and returns `None` if unmatched. + +Run: `cargo test --lib types::schema_type_mapper` +Expected: any test asserting the bare block returns OIDs now fails — those tests are the regression (they encoded the buggy behavior). Update them to assert the new behavior (bare `count` with no query context → `None`, since the resolver handles it via `function_generic_oid`). + +- [ ] **Step 3: Use `ColumnMeta` for field_descriptions in execute_select** + +In `src/query/executor.rs:1191`, the `fields: Vec = response.columns.iter().enumerate().map(|(i, name)| { ... 5-priority if/else ... })`. Replace the type_oid computation to prefer the resolver-provided meta: thread `metas: &[ColumnMeta]` into `execute_select` (add parameter), and set `type_oid = metas.get(i).map(|m| m.type_oid).unwrap_or_else(|| /* existing fallback chain */ )`. For `datetime_columns`: after the field loop, `for m in metas { if m.datetime_flag { datetime_columns.insert(m.wire_name.clone(), ); } }`. + +- [ ] **Step 4: Build + test** + +Run: `cargo build && cargo test --lib` +Expected: PASS (existing suite + updated schema_type_mapper tests). + +- [ ] **Step 5: Pre-commit + commit** + +```bash +cargo check && cargo clippy --quiet && cargo build && cargo test --lib +git add src/query/executor.rs src/types/schema_type_mapper.rs +git commit -m "fix: use ColumnMeta for types + datetime_flag; remove bare-name OID block (F2,F5)" +``` + +--- + +### Task 8: Catalog fixes — F7 (break removal), F-new (Function arm + count(*)), F6 (case-fold), F8 (log) + +**Files:** +- Modify: `src/catalog/query_interceptor.rs` (`:1756-1761` break, `:1767` extract_projection_source_column, `:1749-1754` alias case, `:1082` log) +- Modify: `src/catalog/pg_roles.rs` (`:77-83`,`:102-106` break, `:95-100` alias case) +- Test: `tests/catalog_alias_test.rs` (new tests — Task 9) + +**Interfaces:** +- Consumes: `sqlparser::ast::{Expr, SelectItem, Ident}`; session GUC for F6. + +- [ ] **Step 1: F7 — remove `break` in both extractors** + +In `src/catalog/query_interceptor.rs:1756-1761`, change the `Wildcard | QualifiedWildcard` arm: + +```rust + SelectItem::Wildcard(_) | SelectItem::QualifiedWildcard(_, _) => { + cols.extend_from_slice(all_columns); + indices.extend(0..all_columns.len()); + break; + } +``` +to: +```rust + SelectItem::Wildcard(_) => { + cols.extend_from_slice(all_columns); + indices.extend(0..all_columns.len()); + } + SelectItem::QualifiedWildcard(_, _) => { + cols.extend_from_slice(all_columns); + indices.extend(0..all_columns.len()); + } +``` +(No `break`; continue. QualifiedWildcard currently uses all_columns — acceptable per spec; named-relation filtering is a refinement, noted as future work.) + +Repeat the same `break` removal in `src/catalog/pg_roles.rs` for both `Wildcard` (`:77-83`) and `QualifiedWildcard` (`:102-106`). + +- [ ] **Step 2: F-new — add `Expr::Function` arm to `extract_projection_source_column`** + +In `src/catalog/query_interceptor.rs:1767`, the function currently handles `Identifier`/`CompoundIdentifier`/`Cast`/`Nested` and `_ => None`. Add before the `_`: + +```rust + Expr::Function(f) => { + f.name.0.last().map(|ident| ident.value.to_lowercase()) + } +``` +Then, in each catalog handler that calls `extract_selected_columns` (e.g. `handle_pg_namespace_query` at `:1057`), add detection: if the resulting `(cols, indices)` is empty AND the projection contains `UnnamedExpr(Expr::Function(_))` whose name is `count`, compute the row count of the handler's static dataset and return a single-row single-column `DbResponse { columns: ["count"], rows: [[Some(count.to_string().into_bytes())]], rows_affected: 1 }`. For `pg_namespace` the dataset size is `schemas.len()` (2); for `pg_roles` it's the roles list length. (Minimal scope per spec.) + +- [ ] **Step 3: F6 — case-fold unquoted aliases** + +In `src/catalog/query_interceptor.rs:1749-1754` (`ExprWithAlias` arm) and `src/catalog/pg_roles.rs:95-100`, change `cols.push(alias.value.clone())` to: + +```rust + let name = if alias.quote_style.is_some() || legacy { + alias.value.clone() + } else { + alias.value.to_lowercase() + }; + cols.push(name); +``` +where `legacy` is read from the session GUC `pgsqlite.legacy_result_columns`. Thread `legacy: bool` into `extract_selected_columns` and `get_selected_columns` (add parameter; pass from the handler which has session access, or default `false` where the handler lacks it — catalog queries are typically session-scoped). + +- [ ] **Step 4: F8 — fix the debug log label** + +In `src/catalog/query_interceptor.rs:1082`, change `"pg_type query"` to `"pg_namespace query"`. + +- [ ] **Step 5: Build + test** + +Run: `cargo build && cargo test --lib catalog` +Expected: PASS (new behavior; existing catalog tests may need assertion updates for case-folding — update in this commit). + +- [ ] **Step 6: Pre-commit + commit** + +```bash +cargo check && cargo clippy --quiet && cargo build && cargo test --lib catalog +git add src/catalog/query_interceptor.rs src/catalog/pg_roles.rs +git commit -m "fix: catalog wildcard break (F7), Function projection (F-new), alias case-fold (F6), log (F8)" +``` + +--- + +### Task 9: Expand `catalog_alias_test.rs` (fixes F10) + delete stray file (F9) + +**Files:** +- Modify: `tests/catalog_alias_test.rs` +- Delete: `tests/test_batch_7b.py` +- Test: `cargo test --test catalog_alias_test` + +**Interfaces:** +- Consumes: the catalog extractor behaviors from Task 8. + +- [ ] **Step 1: Add the failing tests** + +Append to `tests/catalog_alias_test.rs` (follow the existing test style; these call the handlers' projection logic). Add tests for: +1. Uppercase alias `SELECT oid AS Did FROM pg_catalog.pg_namespace` → output column `did` (legacy off). +2. Mid-list wildcard `SELECT *, oid FROM pg_catalog.pg_namespace` → 3 columns (`oid, nspname, oid`). +3. `SELECT count(*) FROM pg_catalog.pg_namespace` → 1 row, value `2`. +4. `SELECT CAST(oid AS text) AS o FROM pg_catalog.pg_namespace` → resolves via the `Cast` arm. +5. `SELECT n.nspname FROM pg_catalog.pg_namespace AS n` (CompoundIdentifier) → resolves `nspname`. +6. `SELECT rolname FROM pg_roles WHERE rolname = 'postgres'` (WHERE + alias-free projection) → filtered. +7. `SELECT nonexistent FROM pg_catalog.pg_namespace` → no columns (unknown source). + +- [ ] **Step 2: Run to verify failures** + +Run: `cargo test --test catalog_alias_test` +Expected: some FAIL (the behaviors from Task 8 must be in place; if Task 8 done, these pass; if any fail, fix in Task 8's extractor code). + +- [ ] **Step 3: Delete the stray file (F9)** + +```bash +git rm tests/test_batch_7b.py +``` +Confirm no runner references it: `grep -rn "test_batch_7b" tests/` → expect no hits. + +- [ ] **Step 4: Run + commit** + +```bash +cargo check && cargo clippy --quiet && cargo test --test catalog_alias_test +git add tests/catalog_alias_test.rs +git commit -m "test: expand catalog_alias_test coverage (F10); remove stray test_batch_7b.py (F9)" +``` + +--- + +### Task 10: Integration SQL tests + final sanitizer deletion (F1/F2/F5/F4 end-to-end, remove column_sanitizer.rs) + +**Files:** +- Create: `tests/sql/features/column_conformance.sql` +- Delete: `src/query/column_sanitizer.rs` (and remove `pub mod column_sanitizer;` from `src/query/mod.rs`) +- Test: `./tests/runner/run_ssl_tests.sh` (or the project's integration entry) + +**Interfaces:** +- Consumes: full resolver pipeline (Tasks 5–7) + GUC (Task 1). + +- [ ] **Step 1: Write the integration SQL file** + +Create `tests/sql/features/column_conformance.sql` (follow the existing `tests/sql/features/*.sql` format — statements + `-- EXPECT:` comments the runner checks, or psql `\echo` markers per the runner's convention; inspect an existing file in that dir first): + +```sql +-- F1: quoted paren-containing column name preserved +CREATE TABLE t1 ("price(usd)" INT); +INSERT INTO t1 VALUES (5); +SELECT "price(usd)" FROM t1; +-- EXPECT: column name "price(usd)", type int4, value 5 + +-- F2: unaliased MAX(timestamp) returns formatted timestamp +CREATE TABLE t2 (created_at TIMESTAMP); +INSERT INTO t2 VALUES ('2023-11-14 22:13:20'); +SELECT max(created_at) FROM t2; +-- EXPECT: column "max", value "2023-11-14 22:13:20.000000" (not raw microseconds) + +-- F5: aliased non-aggregate not mistyped +CREATE TABLE t3 (name TEXT); +INSERT INTO t3 VALUES ('alice'); +SELECT upper(name) AS count FROM t3; +-- EXPECT: column "count" type text, value "ALICE" (decodes without error) + +-- F4: unnamed expression -> ?column? +SELECT 1+1; +-- EXPECT: column "?column?", value 2 + +-- F-new: catalog count +SELECT count(*) FROM pg_catalog.pg_namespace; +-- EXPECT: 1 row, value 2 + +-- F7: mid-list wildcard +SELECT *, oid FROM pg_catalog.pg_namespace; +-- EXPECT: 3 columns + +-- C2: legacy GUC off-switch +SET pgsqlite.legacy_result_columns = on; +SELECT MyCol FROM t1; -- t1 has no MyCol; use a real unquoted col if needed +-- EXPECT: legacy casing preserved where applicable +SET pgsqlite.legacy_result_columns = off; +``` +(Adjust the `-- EXPECT` syntax to match the runner's actual convention found by inspecting a sibling file. Replace the C2 example with a real unquoted column from a table created above.) + +- [ ] **Step 2: Run the integration suite** + +Run: `./tests/runner/run_ssl_tests.sh` (from project root) +Expected: the new cases PASS. If the runner doesn't auto-pick `tests/sql/features/*.sql`, wire it in per the runner's include mechanism (check `run_ssl_tests.sh` for the `SQL_FILE` / category loop). + +- [ ] **Step 3: Delete `column_sanitizer.rs` (final cleanup)** + +```bash +grep -rn "sanitize_column_name\|column_sanitizer" src/ # must be zero outside the file itself +git rm src/query/column_sanitizer.rs +``` +Remove `pub mod column_sanitizer;` from `src/query/mod.rs`. + +- [ ] **Step 4: Full pre-commit checklist** + +```bash +cargo check && cargo clippy --quiet && cargo build && cargo test && ./tests/runner/run_ssl_tests.sh +``` +Expected: ALL PASS, no warnings. + +- [ ] **Step 5: Commit** + +```bash +git add tests/sql/features/column_conformance.sql src/query/mod.rs +git commit -m "test: end-to-end column conformance (F1,F2,F4,F5,F-new,F7,C2); remove column_sanitizer.rs" +``` + +--- + +## Self-Review + +**1. Spec coverage:** +- F1 (paren-corruption) → Task 4 step 1 test + Task 6 migration. ✅ +- F2 (MAX/MIN datetime) → Task 4 (`min_over_timestamp_is_datetime`) + Task 7 (`datetime_flag` wiring). ✅ +- F3 (path divergence) → Task 6 (`read_only_handler` migration). ✅ +- F4 (empty name → `?column?`) → Task 4 + Task 5 (dedup). ✅ +- F5 (bare-name OID block) → Task 7 step 2 (delete block). ✅ +- F6 (alias case-fold) → Task 8 step 3. ✅ +- F7 (wildcard break) → Task 8 step 1. ✅ +- F8 (log label) → Task 8 step 4. ✅ +- F9 (stray file) → Task 9 step 3. ✅ +- F10 (test gaps) → Task 9. ✅ +- F-new (catalog aggregate) → Task 8 step 2. ✅ +- C2 GUC → Task 1 + applied in Tasks 4/8. ✅ +- T3 (arg-preserving + datetime) → Task 4 (`resolve_function_type`) + Task 7. ✅ +- Removal of `column_sanitizer.rs` → Task 10 step 3. ✅ + +**2. Placeholder scan:** No TBD/TODO/"implement later". One caveat flagged inline: Task 7 step 1 notes the unit-vs-integration test split for the datetime guard (deferred to Task 8 integration) — this is a deliberate test-placement note, not a placeholder. Task 5 step 4 conditionally migrates one site depending on scope availability, with explicit fallback to Task 6 — documented, not vague. + +**3. Type consistency:** `ColumnMeta { wire_name: String, type_oid: i32, datetime_flag: bool }` consistent across Tasks 2/4/5/7. `FnShape { name, inner }` consistent. `AliasItem { position, alias, is_quoted, source_expr }` consistent. `resolve_columns(stmt, query, schema_types, hints, session)` signature consistent across Tasks 5/6/7. `legacy_result_columns() -> bool` consistent. `Ident.quote_style.is_some()` used for `is_quoted` (verified against sqlparser 0.57 API via the codebase's existing `alias.value` usage). From 9427c818b286890dbfcd0ebf211c8798dfbeb7e0 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 12:34:32 -0700 Subject: [PATCH 03/21] feat: add pgsqlite.legacy_result_columns GUC reader (default off) --- src/session/state.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/session/state.rs b/src/session/state.rs index 05887af..a9bff9d 100644 --- a/src/session/state.rs +++ b/src/session/state.rs @@ -81,6 +81,16 @@ impl SessionState { } } + /// Read the legacy result-column GUC. Default off (conformant). + /// Only casing in resolver steps 3 & 5 honors this; the paren-corruption, + /// empty-name, and datetime fixes are always applied. + pub async fn legacy_result_columns(&self) -> bool { + self.parameters.read().await + .get("pgsqlite.legacy_result_columns") + .map(|v| v.eq_ignore_ascii_case("on")) + .unwrap_or(false) + } + /// Create a new session with default database and user (for testing) #[cfg(test)] pub fn new_test() -> Self { @@ -161,4 +171,31 @@ impl Drop for SessionState { // Decrement active session count when session is destroyed ACTIVE_SESSION_COUNT.fetch_sub(1, Ordering::Relaxed); } +} + +#[cfg(test)] +mod legacy_guc_tests { + use super::*; + + #[tokio::test] + async fn test_legacy_default_off() { + let s = SessionState::new("db".into(), "user".into()); + assert!(!s.legacy_result_columns().await); + } + + #[tokio::test] + async fn test_legacy_on_when_set_on() { + let s = SessionState::new("db".into(), "user".into()); + s.parameters.write().await.insert( + "pgsqlite.legacy_result_columns".to_string(), "on".to_string()); + assert!(s.legacy_result_columns().await); + } + + #[tokio::test] + async fn test_legacy_off_explicit() { + let s = SessionState::new("db".into(), "user".into()); + s.parameters.write().await.insert( + "pgsqlite.legacy_result_columns".to_string(), "off".to_string()); + assert!(!s.legacy_result_columns().await); + } } \ No newline at end of file From 2c804f463150f14af127ca9e3e6f64b66ef1cabc Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 12:44:14 -0700 Subject: [PATCH 04/21] feat: add ColumnMeta, fn_shape, and function->type table --- src/query/mod.rs | 1 + src/query/projection_resolver.rs | 101 +++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 src/query/projection_resolver.rs diff --git a/src/query/mod.rs b/src/query/mod.rs index 7badf65..c48b804 100644 --- a/src/query/mod.rs +++ b/src/query/mod.rs @@ -16,6 +16,7 @@ pub mod pattern_optimizer; pub mod query_handler; pub mod join_type_inference; pub mod column_sanitizer; +pub mod projection_resolver; pub use executor::QueryExecutor; pub use query_handler::{QueryHandler, QueryHandlerImpl}; diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs new file mode 100644 index 0000000..7ee71a7 --- /dev/null +++ b/src/query/projection_resolver.rs @@ -0,0 +1,101 @@ +use crate::types::PgType; + +#[derive(Debug, Clone, PartialEq)] +pub struct ColumnMeta { + pub wire_name: String, + pub type_oid: i32, + pub datetime_flag: bool, +} + +#[derive(Debug, Clone, PartialEq)] +pub struct FnShape { + pub name: String, + pub inner: String, +} + +/// Parse a SQLite result-column name into function name + inner argument text. +/// `count(*)` -> { count, * }; `max(created_at)` -> { max, created_at }; +/// `COALESCE(max(id), 0)` -> { COALESCE, "max(id), 0" }. +/// Returns None when the name contains no `(`. +pub fn fn_shape(raw_name: &str) -> Option { + let open = raw_name.find('(')?; + let last_close = raw_name.rfind(')')?; + if last_close <= open { return None; } + Some(FnShape { + name: raw_name[..open].to_string(), + inner: raw_name[open + 1..last_close].to_string(), + }) +} + +/// T3 generic return OID for functions whose type does not depend on the argument. +pub fn function_generic_oid(fn_name_lower: &str) -> Option { + Some(match fn_name_lower { + "count" => PgType::Int8.to_oid(), + "avg" => PgType::Numeric.to_oid(), + "json_extract" | "json_agg" | "jsonb_agg" | "json_object" | "json_object_agg" + | "jsonb_object_agg" | "row_to_json" | "json_array" | "json_group_array" + | "json_extract_path" | "json_extract_path_text" => PgType::Text.to_oid(), + "array_length" | "array_upper" | "array_lower" | "array_ndims" + | "array_position" => PgType::Int4.to_oid(), + "array_append" | "array_prepend" | "array_cat" | "array_remove" + | "array_replace" | "array_slice" | "string_to_array" | "array_positions" + | "array_to_string" | "unnest" | "array_agg" => PgType::Text.to_oid(), + "array_contains" | "array_contained" | "array_overlap" => PgType::Bool.to_oid(), + "now" | "current_timestamp" => PgType::Timestamptz.to_oid(), + "current_date" => PgType::Text.to_oid(), + "current_time" => PgType::Time.to_oid(), + "extract" => PgType::Float8.to_oid(), + "date_trunc" | "to_timestamp" => PgType::Timestamp.to_oid(), + "make_date" => PgType::Date.to_oid(), + "make_time" => PgType::Time.to_oid(), + "age" => PgType::Interval.to_oid(), + "decimal_add" | "decimal_sub" | "decimal_mul" | "decimal_div" + | "decimal_from_text" => PgType::Numeric.to_oid(), + _ => return None, + }) +} + +/// min/max/sum/first/last take the argument column's type (T3). +pub fn is_arg_preserving(fn_name_lower: &str) -> bool { + matches!(fn_name_lower, "min" | "max" | "sum" | "first" | "last") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn fn_shape_count_star() { + assert_eq!(fn_shape("count(*)"), Some(FnShape { name: "count".into(), inner: "*".into() })); + } + #[test] + fn fn_shape_max() { + assert_eq!(fn_shape("max(created_at)"), Some(FnShape { name: "max".into(), inner: "created_at".into() })); + } + #[test] + fn fn_shape_coalesce_nested() { + let s = fn_shape("COALESCE(max(id), 0)").unwrap(); + assert_eq!(s.name, "COALESCE"); + assert_eq!(s.inner, "max(id), 0"); + } + #[test] + fn fn_shape_no_parens_is_none() { + assert!(fn_shape("my_column").is_none()); + assert!(fn_shape("").is_none()); + } + #[test] + fn generic_oid_count_int8() { + assert_eq!(function_generic_oid("count"), Some(PgType::Int8.to_oid())); + } + #[test] + fn generic_oid_min_max_none() { + assert!(function_generic_oid("min").is_none()); + assert!(function_generic_oid("max").is_none()); + assert!(function_generic_oid("sum").is_none()); + } + #[test] + fn arg_preserving_flags() { + assert!(is_arg_preserving("max") && is_arg_preserving("sum")); + assert!(!is_arg_preserving("count")); + } +} From 28d2f513a9675ab27665f6e23c8eb4e105d81014 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 12:54:22 -0700 Subject: [PATCH 05/21] feat: add AliasItem + cached alias-view parse (AS-gated) --- src/query/extended.rs | 3 ++ src/query/projection_resolver.rs | 57 ++++++++++++++++++++++++++++++++ src/session/state.rs | 1 + 3 files changed, 61 insertions(+) diff --git a/src/query/extended.rs b/src/query/extended.rs index 695b294..e6af494 100644 --- a/src/query/extended.rs +++ b/src/query/extended.rs @@ -127,6 +127,7 @@ impl ExtendedQueryHandler { param_formats: vec![0; cached_info.param_types.len()], field_descriptions: Vec::new(), // Will be populated during bind/execute translation_metadata: None, + projection_metadata: None, }; // Store as unnamed statement @@ -276,6 +277,7 @@ impl ExtendedQueryHandler { vec![] }, translation_metadata: None, // SET commands don't need translation metadata + projection_metadata: None, }; session.prepared_statements.write().await.insert(name.clone(), stmt); @@ -1018,6 +1020,7 @@ impl ExtendedQueryHandler { } else { Some(translation_metadata) }, + projection_metadata: None, }; session.prepared_statements.write().await.insert(name.clone(), stmt); diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs index 7ee71a7..b2ec452 100644 --- a/src/query/projection_resolver.rs +++ b/src/query/projection_resolver.rs @@ -1,4 +1,37 @@ use crate::types::PgType; +use sqlparser::ast::{Expr, SelectItem, SetExpr, Statement}; +use sqlparser::dialect::PostgreSqlDialect; +use sqlparser::parser::Parser; + +#[derive(Debug, Clone)] +pub struct AliasItem { + pub position: usize, + pub alias: String, + pub is_quoted: bool, + pub source_expr: Expr, +} + +/// Parse the projection alias view. Returns None when the query has no `AS` +/// (so the zero-alloc fast path never pays a parse). On parse error, None. +pub fn parse_alias_view(query: &str) -> Option> { + if !query.to_uppercase().contains(" AS ") { return None; } + let parsed = Parser::parse_sql(&PostgreSqlDialect {}, query).ok()?; + let stmt = parsed.into_iter().next()?; + let body = match stmt { Statement::Query(q) => q, _ => return None }; + let select = match &*body.body { SetExpr::Select(s) => s, _ => return None }; + let items: Vec = select.projection.iter().enumerate() + .filter_map(|(i, item)| match item { + SelectItem::ExprWithAlias { expr, alias } => Some(AliasItem { + position: i, + alias: alias.value.clone(), + is_quoted: alias.quote_style.is_some(), + source_expr: expr.clone(), + }), + _ => None, + }) + .collect(); + Some(items) +} #[derive(Debug, Clone, PartialEq)] pub struct ColumnMeta { @@ -98,4 +131,28 @@ mod tests { assert!(is_arg_preserving("max") && is_arg_preserving("sum")); assert!(!is_arg_preserving("count")); } + #[test] + fn alias_view_none_when_no_as() { + assert!(parse_alias_view("SELECT id, name FROM users").is_none()); + } + #[test] + fn alias_view_unquoted_alias() { + let v = parse_alias_view("SELECT id AS user_id FROM users").unwrap(); + assert_eq!(v.len(), 1); + assert_eq!(v[0].alias, "user_id"); + assert!(!v[0].is_quoted); + } + #[test] + fn alias_view_quoted_alias_preserved() { + let v = parse_alias_view(r#"SELECT id AS "UserId" FROM users"#).unwrap(); + assert_eq!(v.len(), 1); + assert_eq!(v[0].alias, "UserId"); + assert!(v[0].is_quoted); + } + #[test] + fn alias_view_position_indexed() { + let v = parse_alias_view("SELECT id AS a, name AS b FROM users").unwrap(); + assert_eq!(v[0].position, 0); + assert_eq!(v[1].position, 1); + } } diff --git a/src/session/state.rs b/src/session/state.rs index a9bff9d..36202ed 100644 --- a/src/session/state.rs +++ b/src/session/state.rs @@ -39,6 +39,7 @@ pub struct PreparedStatement { pub param_formats: Vec, pub field_descriptions: Vec, pub translation_metadata: Option, // Type hints from query translation + pub projection_metadata: Option>, } #[derive(Clone)] From dff130d44a3e590514eebeaad641457f4945de03 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 13:24:11 -0700 Subject: [PATCH 06/21] feat: ProjectionResolver precedence core (schema>fn>alias>?column?>fallback) --- src/query/projection_resolver.rs | 144 ++++++++++++++++++++++++++++++- 1 file changed, 142 insertions(+), 2 deletions(-) diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs index b2ec452..e239c3a 100644 --- a/src/query/projection_resolver.rs +++ b/src/query/projection_resolver.rs @@ -1,4 +1,6 @@ -use crate::types::PgType; +use std::collections::HashMap; +use crate::types::{PgType, SchemaTypeMapper}; +use crate::translator::TranslationMetadata; use sqlparser::ast::{Expr, SelectItem, SetExpr, Statement}; use sqlparser::dialect::PostgreSqlDialect; use sqlparser::parser::Parser; @@ -14,7 +16,7 @@ pub struct AliasItem { /// Parse the projection alias view. Returns None when the query has no `AS` /// (so the zero-alloc fast path never pays a parse). On parse error, None. pub fn parse_alias_view(query: &str) -> Option> { - if !query.to_uppercase().contains(" AS ") { return None; } + if !query.split_whitespace().any(|w| w.eq_ignore_ascii_case("AS")) { return None; } let parsed = Parser::parse_sql(&PostgreSqlDialect {}, query).ok()?; let stmt = parsed.into_iter().next()?; let body = match stmt { Statement::Query(q) => q, _ => return None }; @@ -93,6 +95,88 @@ pub fn is_arg_preserving(fn_name_lower: &str) -> bool { matches!(fn_name_lower, "min" | "max" | "sum" | "first" | "last") } +pub struct ResolveCtx<'a> { + pub schema_types: &'a HashMap, + pub hints: &'a TranslationMetadata, + pub alias_view: Option<&'a [AliasItem]>, + pub legacy: bool, +} + +pub struct ProjectionResolver; + +impl ProjectionResolver { + pub fn resolve(raw_name: &str, position: usize, ctx: &ResolveCtx) -> ColumnMeta { + // Step 1: schema match on the raw name (real column — correct case already). + if let Some(pg_type) = ctx.schema_types.get(raw_name) { + return ColumnMeta { + wire_name: raw_name.to_string(), + type_oid: SchemaTypeMapper::pg_type_string_to_oid(pg_type), + datetime_flag: is_datetime_type(pg_type), + }; + } + + // Step 2: function-call shape (raw name contains `(`). + if let Some(shape) = fn_shape(raw_name) { + let name = shape.name.trim(); + let lower = name.to_lowercase(); + let wire_name = if ctx.legacy { name.to_string() } else { lower.clone() }; + let (type_oid, datetime_flag) = resolve_function_type(&lower, &shape.inner, ctx); + return ColumnMeta { wire_name, type_oid, datetime_flag }; + } + + // Step 3: alias (position maps to an ExprWithAlias item). + if let Some(item) = ctx.alias_view.and_then(|view| view.iter().find(|a| a.position == position)) { + let wire_name = if item.is_quoted || ctx.legacy { + item.alias.clone() + } else { + item.alias.to_lowercase() + }; + let type_oid = ctx.hints.get_hint(&item.alias) + .and_then(|h| h.suggested_type.map(|t| t.to_oid())) + .unwrap_or(PgType::Text.to_oid()); + return ColumnMeta { wire_name, type_oid, datetime_flag: false }; + } + + // Step 4: unnamed non-function expression -> ?column?. + if raw_name.is_empty() || looks_unnamed_expr(raw_name) { + return ColumnMeta { + wire_name: "?column?".to_string(), + type_oid: ctx.hints.get_hint(raw_name) + .and_then(|h| h.suggested_type.map(|t| t.to_oid())) + .unwrap_or(PgType::Text.to_oid()), + datetime_flag: false, + }; + } + + // Step 5: fallback — raw name, lowercased unless legacy. + let wire_name = if ctx.legacy { raw_name.to_string() } else { raw_name.to_lowercase() }; + ColumnMeta { wire_name, type_oid: PgType::Text.to_oid(), datetime_flag: false } + } +} + +fn is_datetime_type(pg_type: &str) -> bool { + let u = pg_type.to_uppercase(); + u.contains("TIMESTAMP") || u.contains("DATE") || u.contains("TIME") +} + +fn looks_unnamed_expr(name: &str) -> bool { + // SQLite emits the verbatim expression text for unaliased expressions. + // Heuristic: contains an operator, a space, or is purely numeric/literal. + name.chars().any(|c| matches!(c, '+'|'-'|'*'|'/'|' '|'=')) || name.parse::().is_ok() +} + +fn resolve_function_type(lower_fn: &str, inner: &str, ctx: &ResolveCtx) -> (i32, bool) { + if is_arg_preserving(lower_fn) { + if let Some(pg_type) = ctx.schema_types.get(inner.trim()) { + let oid = SchemaTypeMapper::pg_type_string_to_oid(pg_type); + return (oid, is_datetime_type(pg_type)); + } + // sum generic fallback is numeric; min/max fallback is text. + return (function_generic_oid(lower_fn).unwrap_or(PgType::Text.to_oid()), false); + } + (function_generic_oid(lower_fn).unwrap_or(PgType::Text.to_oid()), false) +} + #[cfg(test)] mod tests { use super::*; @@ -155,4 +239,60 @@ mod tests { assert_eq!(v[0].position, 0); assert_eq!(v[1].position, 1); } + #[test] + fn alias_view_as_with_tab_newline_whitespace() { + // Hardened AS-gate must match `AS` separated by tab/newline, not just spaces. + let v = parse_alias_view("SELECT id\tAS\nuser_id FROM users").unwrap(); + assert_eq!(v.len(), 1); + assert_eq!(v[0].alias, "user_id"); + assert!(!v[0].is_quoted); + } + + use std::collections::HashMap; + use crate::translator::TranslationMetadata; + + fn ctx_no_aliases<'a>(schema: &'a HashMap, legacy: bool) -> ResolveCtx<'a> { + static EMPTY: once_cell::sync::Lazy = once_cell::sync::Lazy::new(TranslationMetadata::new); + ResolveCtx { schema_types: schema, hints: &EMPTY, alias_view: None, legacy } + } + + #[test] + fn schema_match_keeps_paren_name() { + let mut schema = HashMap::new(); + schema.insert("price(usd)".to_string(), "INT4".to_string()); + let m = ProjectionResolver::resolve("price(usd)", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "price(usd)"); + assert_eq!(m.type_oid, PgType::Int4.to_oid()); + } + #[test] + fn function_shape_lowers_and_types() { + let schema = HashMap::new(); + let m = ProjectionResolver::resolve("count(*)", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "count"); + assert_eq!(m.type_oid, PgType::Int8.to_oid()); + assert!(!m.datetime_flag); + } + #[test] + fn min_over_timestamp_is_datetime() { + let mut schema = HashMap::new(); + schema.insert("created_at".to_string(), "TIMESTAMP".to_string()); + let m = ProjectionResolver::resolve("max(created_at)", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "max"); + assert_eq!(m.type_oid, PgType::Timestamp.to_oid()); + assert!(m.datetime_flag); + } + #[test] + fn min_over_int4_no_datetime() { + let mut schema = HashMap::new(); + schema.insert("qty".to_string(), "INT4".to_string()); + let m = ProjectionResolver::resolve("max(qty)", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.type_oid, PgType::Int4.to_oid()); + assert!(!m.datetime_flag); + } + #[test] + fn unnamed_expr_gets_question_column() { + let schema = HashMap::new(); + let m = ProjectionResolver::resolve("1+1", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "?column?"); + } } From 906229e744431bd7145c7f3968ccd07e9e682aa2 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 13:48:42 -0700 Subject: [PATCH 07/21] fix: step 2 casing always conformant; add legacy=true + alias + array_agg tests --- src/query/projection_resolver.rs | 54 +++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs index e239c3a..525abf4 100644 --- a/src/query/projection_resolver.rs +++ b/src/query/projection_resolver.rs @@ -119,7 +119,7 @@ impl ProjectionResolver { if let Some(shape) = fn_shape(raw_name) { let name = shape.name.trim(); let lower = name.to_lowercase(); - let wire_name = if ctx.legacy { name.to_string() } else { lower.clone() }; + let wire_name = lower.clone(); let (type_oid, datetime_flag) = resolve_function_type(&lower, &shape.inner, ctx); return ColumnMeta { wire_name, type_oid, datetime_flag }; } @@ -256,6 +256,11 @@ mod tests { ResolveCtx { schema_types: schema, hints: &EMPTY, alias_view: None, legacy } } + fn ctx_with_aliases<'a>(schema: &'a HashMap, alias_view: &'a [AliasItem], legacy: bool) -> ResolveCtx<'a> { + static EMPTY: once_cell::sync::Lazy = once_cell::sync::Lazy::new(TranslationMetadata::new); + ResolveCtx { schema_types: schema, hints: &EMPTY, alias_view: Some(alias_view), legacy } + } + #[test] fn schema_match_keeps_paren_name() { let mut schema = HashMap::new(); @@ -295,4 +300,51 @@ mod tests { let m = ProjectionResolver::resolve("1+1", 0, &ctx_no_aliases(&schema, false)); assert_eq!(m.wire_name, "?column?"); } + + #[test] + fn step2_function_call_ignores_legacy() { + // Guard: legacy affects only steps 3 & 5. A COUNT(*) projection must + // always emit conformant wire-name `count` even when legacy=true. + let schema = HashMap::new(); + let m = ProjectionResolver::resolve("COUNT(*)", 0, &ctx_no_aliases(&schema, true)); + assert_eq!(m.wire_name, "count", "legacy must not leak into step 2 casing"); + assert_eq!(m.type_oid, PgType::Int8.to_oid()); + } + + #[test] + fn step3_unquoted_alias_legacy_preserves_case() { + // Step 3 honors legacy: an unquoted alias is preserved as-written when + // legacy=true, and lowercased when legacy=false. + let schema = HashMap::new(); + let items = vec![AliasItem { + position: 0, + alias: "MyAlias".to_string(), + is_quoted: false, + source_expr: Expr::Identifier(sqlparser::ast::Ident::new("id")), + }]; + let legacy_m = ProjectionResolver::resolve("id", 0, &ctx_with_aliases(&schema, &items, true)); + assert_eq!(legacy_m.wire_name, "MyAlias", "legacy preserves unquoted alias as-written"); + let conformant_m = ProjectionResolver::resolve("id", 0, &ctx_with_aliases(&schema, &items, false)); + assert_eq!(conformant_m.wire_name, "myalias", "conformant lowercases unquoted alias"); + } + + #[test] + fn step5_fallback_legacy_preserves_case() { + // Step 5 honors legacy: fallback casing preserves the raw name when + // legacy=true and lowercases it when legacy=false. + let schema = HashMap::new(); + let legacy_m = ProjectionResolver::resolve("MyCol", 0, &ctx_no_aliases(&schema, true)); + assert_eq!(legacy_m.wire_name, "MyCol", "legacy preserves fallback name as-written"); + let conformant_m = ProjectionResolver::resolve("MyCol", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(conformant_m.wire_name, "mycol", "conformant lowercases fallback name"); + } + + #[test] + fn array_agg_returns_text_oid() { + // MUST-FIX #3: array_agg returns Text (JSON array storage). + let schema = HashMap::new(); + let m = ProjectionResolver::resolve("array_agg(x)", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "array_agg"); + assert_eq!(m.type_oid, PgType::Text.to_oid()); + } } From 2f552b47360ad09bc91699996d5a0c2257656869 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 14:03:57 -0700 Subject: [PATCH 08/21] feat: resolve_columns + resolve_columns_from_names helpers with ?column?N dedup Add two async convergence-point helpers to projection_resolver: - resolve_columns(stmt, ...): reads rusqlite::Statement column names - resolve_columns_from_names(names, ...): resolves DbResponse.columns strings (used by execute_select in Task 7, which has only column-name strings) Both share a single DRY dedup_question_columns() implementation: the first unnamed expression stays ?column?; the second becomes ?column?2, third ?column?3, etc. Each reads session.legacy_result_columns().await, fetches the alias view via parse_alias_view(query), and calls ProjectionResolver::resolve per column. Unit test resolve_columns_from_names_dedup_numbering constructs ['a+b','c+d'] with an empty schema + empty hints + SessionState and asserts the second becomes ?column?2. question_column_dedup_numbering guards the base ?column? name for a single unnamed expr. db_handler.rs:1721 site NOT migrated this task: it lives inside a sync closure in query_with_session(&self, query, session_id) which lacks session/ schema_types/translation_metadata in scope and cannot .await. Deferred to Task 6 per the brief's conditional-migration instruction. --- src/query/projection_resolver.rs | 91 ++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs index 525abf4..3f8f3ff 100644 --- a/src/query/projection_resolver.rs +++ b/src/query/projection_resolver.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use anyhow::Result; use crate::types::{PgType, SchemaTypeMapper}; use crate::translator::TranslationMetadata; use sqlparser::ast::{Expr, SelectItem, SetExpr, Statement}; @@ -177,6 +178,67 @@ fn resolve_function_type(lower_fn: &str, inner: &str, ctx: &ResolveCtx) -> (i32, (function_generic_oid(lower_fn).unwrap_or(PgType::Text.to_oid()), false) } +/// Resolve all columns of a prepared statement into PG-conformant metadata. +/// Convergence point for DbHandler, ReadOnlyDbHandler, and extended paths. +pub async fn resolve_columns( + stmt: &rusqlite::Statement<'_>, + query: &str, + schema_types: &HashMap, + hints: &TranslationMetadata, + session: &crate::session::SessionState, +) -> Result> { + let legacy = session.legacy_result_columns().await; + let alias_view = parse_alias_view(query); + let view_ref = alias_view.as_deref(); + let count = stmt.column_count(); + let mut out = Vec::with_capacity(count); + for i in 0..count { + let raw = stmt.column_name(i)?.to_string(); + let ctx = ResolveCtx { schema_types, hints, alias_view: view_ref, legacy }; + let meta = ProjectionResolver::resolve(&raw, i, &ctx); + out.push(meta); + } + dedup_question_columns(&mut out); + Ok(out) +} + +/// Resolve a list of column-name strings (from DbResponse.columns) into PG-conformant +/// metadata. Used by execute_select, which only has the column-name strings, not the stmt. +/// Same precedence and ?column?N dedup as [`resolve_columns`]. +pub async fn resolve_columns_from_names( + names: &[String], + query: &str, + schema_types: &HashMap, + hints: &TranslationMetadata, + session: &crate::session::SessionState, +) -> Result> { + let legacy = session.legacy_result_columns().await; + let alias_view = parse_alias_view(query); + let view_ref = alias_view.as_deref(); + let mut out = Vec::with_capacity(names.len()); + for (i, raw) in names.iter().enumerate() { + let ctx = ResolveCtx { schema_types, hints, alias_view: view_ref, legacy }; + let meta = ProjectionResolver::resolve(raw, i, &ctx); + out.push(meta); + } + dedup_question_columns(&mut out); + Ok(out) +} + +/// Apply PostgreSQL's `?column?N` dedup to consecutive unnamed-expression columns. +/// First unnamed stays `?column?`; the second becomes `?column?2`, third `?column?3`, etc. +fn dedup_question_columns(metas: &mut [ColumnMeta]) { + let mut count: usize = 0; + for meta in metas.iter_mut() { + if meta.wire_name == "?column?" { + count += 1; + if count > 1 { + meta.wire_name = format!("?column?{}", count); + } + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -347,4 +409,33 @@ mod tests { assert_eq!(m.wire_name, "array_agg"); assert_eq!(m.type_oid, PgType::Text.to_oid()); } + + #[test] + fn question_column_dedup_numbering() { + // Two unnamed expressions at positions 0 and 1 should both yield ?column? + // in the single-column resolve() (dedup happens at resolve_columns level), + // but resolve_columns is async+DB-bound; tested via integration in Task 8. + // Here we only assert the base name for one unnamed expr: + let schema = HashMap::new(); + let m = ProjectionResolver::resolve("a+b", 0, &ctx_no_aliases(&schema, false)); + assert_eq!(m.wire_name, "?column?"); + } + + #[tokio::test] + async fn resolve_columns_from_names_dedup_numbering() { + // Unit test for the shared ?column?N dedup: two unnamed expressions + // -> first is ?column?, second is ?column?2. + let schema = HashMap::new(); + let hints = TranslationMetadata::new(); + let session = crate::session::SessionState::new("db".into(), "user".into()); + let names = vec!["a+b".to_string(), "c+d".to_string()]; + let metas = resolve_columns_from_names( + &names, "SELECT a+b, c+d FROM t", &schema, &hints, &session, + ) + .await + .unwrap(); + assert_eq!(metas.len(), 2); + assert_eq!(metas[0].wire_name, "?column?"); + assert_eq!(metas[1].wire_name, "?column?2"); + } } From 50fe95eac4483ed1343bb11cd1fc326ee02becf0 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 15:36:33 -0700 Subject: [PATCH 09/21] refactor: route all SELECT paths through resolve_columns (fixes F3) Migrate every sanitize_column_name site in db_handler.rs (9 sites) and both ReadOnlyDbHandler query paths to the ProjectionResolver so all SELECT paths emit PG-conformant column names identically. - Add sync resolve_columns_with_legacy() to projection_resolver (caller supplies legacy bool); async resolve_columns() delegates to it. This lets the db_handler sites that run inside execute_with_session sync closures resolve inline where the stmt lives, with no .await and no &SessionState. - db_handler.rs: replace all 9 sanitize loops with resolve_columns_with_legacy (legacy=false conformant default, empty schema_types/hints). Remove the now-unused sanitize_column_name import. - read_only_handler.rs: thread &SessionState into query()/query_with_params() and build column names via the resolver (legacy from the real session GUC). Compute legacy before pool.acquire() so the non-Send rusqlite::Statement is never held across an await. Update the QueryRouter call sites + the test. - Fixes F3: pooling-on (ReadOnlyDbHandler) and pooling-off (DbHandler) now produce identical, conformant column names (verified end-to-end). --- src/query/projection_resolver.rs | 18 +++++++- src/session/db_handler.rs | 74 ++++++++++++++++---------------- src/session/query_router.rs | 4 +- src/session/read_only_handler.rs | 26 ++++++++--- 4 files changed, 74 insertions(+), 48 deletions(-) diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs index 3f8f3ff..cacbcd5 100644 --- a/src/query/projection_resolver.rs +++ b/src/query/projection_resolver.rs @@ -188,6 +188,21 @@ pub async fn resolve_columns( session: &crate::session::SessionState, ) -> Result> { let legacy = session.legacy_result_columns().await; + Ok(resolve_columns_with_legacy(stmt, query, schema_types, hints, legacy)?) +} + +/// Sync, session-free variant of [`resolve_columns`]: the caller supplies the +/// `legacy` flag directly instead of a session handle. Used by DbHandler SELECT +/// sites that run inside `execute_with_session` sync closures (no `.await`, no +/// `&SessionState` in scope). The GUC default is conformant (`legacy = false`); +/// Task 7/8 wires the real session at the executor level for GUC honoring. +pub fn resolve_columns_with_legacy( + stmt: &rusqlite::Statement<'_>, + query: &str, + schema_types: &HashMap, + hints: &TranslationMetadata, + legacy: bool, +) -> rusqlite::Result> { let alias_view = parse_alias_view(query); let view_ref = alias_view.as_deref(); let count = stmt.column_count(); @@ -195,8 +210,7 @@ pub async fn resolve_columns( for i in 0..count { let raw = stmt.column_name(i)?.to_string(); let ctx = ResolveCtx { schema_types, hints, alias_view: view_ref, legacy }; - let meta = ProjectionResolver::resolve(&raw, i, &ctx); - out.push(meta); + out.push(ProjectionResolver::resolve(&raw, i, &ctx)); } dedup_question_columns(&mut out); Ok(out) diff --git a/src/session/db_handler.rs b/src/session/db_handler.rs index 2b4b816..7f6afd8 100644 --- a/src/session/db_handler.rs +++ b/src/session/db_handler.rs @@ -14,7 +14,7 @@ use crate::session::ConnectionManager; use crate::ddl::CommentDdlHandler; use crate::PgSqliteError; use crate::security::{events, SqlInjectionDetector}; -use crate::query::column_sanitizer::sanitize_column_name; +use crate::query::projection_resolver::resolve_columns_with_legacy; use tracing::{debug, info, error, warn}; /// Security limits for query validation @@ -525,10 +525,10 @@ impl DbHandler { let result = match query_type { QueryType::Select => { let column_count = stmt.column_count(); - let mut columns = Vec::with_capacity(column_count); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, query, &std::collections::HashMap::new(), + &crate::translator::TranslationMetadata::new(), false, + )?.iter().map(|m| m.wire_name.clone()).collect(); let rows: Result, _> = stmt.query_map(rusqlite::params_from_iter(values.iter()), |row| { let mut row_data = Vec::with_capacity(column_count); @@ -660,10 +660,10 @@ impl DbHandler { // Now execute the actual query against the temp table let mut stmt = temp_conn.prepare(query)?; let column_count = stmt.column_count(); - let mut columns = Vec::new(); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, query, &std::collections::HashMap::new(), + &crate::translator::TranslationMetadata::new(), false, + )?.iter().map(|m| m.wire_name.clone()).collect(); let rows_result: rusqlite::Result>>>> = stmt.query_map([], |row| { let mut values = Vec::new(); @@ -773,10 +773,10 @@ impl DbHandler { // Now execute the original query against the temp table let mut stmt = temp_conn.prepare(query)?; let column_count = stmt.column_count(); - let mut columns = Vec::with_capacity(column_count); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, query, &std::collections::HashMap::new(), + &crate::translator::TranslationMetadata::new(), false, + )?.iter().map(|m| m.wire_name.clone()).collect(); let rows: Result, _> = stmt.query_map([], |row| { let mut row_data = Vec::with_capacity(column_count); @@ -891,10 +891,10 @@ impl DbHandler { let mut stmt = conn.prepare(&processed_query)?; let column_count = stmt.column_count(); - let mut columns = Vec::with_capacity(column_count); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, query, &std::collections::HashMap::new(), + &crate::translator::TranslationMetadata::new(), false, + )?.iter().map(|m| m.wire_name.clone()).collect(); let rows: Result, _> = stmt.query_map([], |row| { let mut row_data = Vec::with_capacity(column_count); @@ -1238,10 +1238,10 @@ impl DbHandler { let mut stmt = conn.prepare(&processed_query)?; let column_count = stmt.column_count(); - let mut columns = Vec::with_capacity(column_count); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, query, &std::collections::HashMap::new(), + &crate::translator::TranslationMetadata::new(), false, + )?.iter().map(|m| m.wire_name.clone()).collect(); let rows: Result, _> = stmt.query_map([], |row| { let mut row_data = Vec::with_capacity(column_count); @@ -1449,10 +1449,10 @@ impl DbHandler { let processed_query = process_query(query, conn, &self.schema_cache)?; let mut stmt = conn.prepare(&processed_query)?; let column_count = stmt.column_count(); - let mut columns = Vec::with_capacity(column_count); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, query, &std::collections::HashMap::new(), + &crate::translator::TranslationMetadata::new(), false, + )?.iter().map(|m| m.wire_name.clone()).collect(); let rows: Result, _> = stmt.query_map([], |row| { let mut row_data = Vec::with_capacity(column_count); @@ -1718,10 +1718,10 @@ impl DbHandler { let mut stmt = conn.prepare(&processed_query)?; let column_count = stmt.column_count(); - let mut columns = Vec::with_capacity(column_count); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, query, &std::collections::HashMap::new(), + &crate::translator::TranslationMetadata::new(), false, + )?.iter().map(|m| m.wire_name.clone()).collect(); let rows: Result, _> = stmt.query_map([], |row| { let mut row_data = Vec::with_capacity(column_count); @@ -2322,10 +2322,10 @@ impl DbHandler { let response: Result = match query_type { QueryType::Select => { let column_count = stmt.column_count(); - let mut column_names = Vec::with_capacity(column_count); - for i in 0..column_count { - column_names.push(sanitize_column_name(stmt.column_name(i).unwrap_or("")).to_string()); - } + let column_names: Vec = resolve_columns_with_legacy( + &stmt, query, &std::collections::HashMap::new(), + &crate::translator::TranslationMetadata::new(), false, + )?.iter().map(|m| m.wire_name.clone()).collect(); // Build datetime column info for conversion let mut datetime_columns = std::collections::HashMap::new(); @@ -2420,10 +2420,10 @@ impl DbHandler { if query.contains("RETURNING") { // Handle RETURNING clause let column_count = stmt.column_count(); - let mut column_names = Vec::with_capacity(column_count); - for i in 0..column_count { - column_names.push(sanitize_column_name(stmt.column_name(i).unwrap_or("")).to_string()); - } + let column_names: Vec = resolve_columns_with_legacy( + &stmt, query, &std::collections::HashMap::new(), + &crate::translator::TranslationMetadata::new(), false, + )?.iter().map(|m| m.wire_name.clone()).collect(); // Build datetime column info for conversion let mut datetime_columns = std::collections::HashMap::new(); diff --git a/src/session/query_router.rs b/src/session/query_router.rs index 7a97bab..ac974fc 100644 --- a/src/session/query_router.rs +++ b/src/session/query_router.rs @@ -80,7 +80,7 @@ impl QueryRouter { match route { QueryRoute::ReadOnly => { info!("Executing query via read-only pool"); - let result = self.read_handler.query(sql).await?; + let result = self.read_handler.query(sql, session_state).await?; Ok(result) } QueryRoute::Write | QueryRoute::WriteTransaction => { @@ -103,7 +103,7 @@ impl QueryRouter { match route { QueryRoute::ReadOnly => { - let result = self.read_handler.query_with_params(sql, params).await?; + let result = self.read_handler.query_with_params(sql, params, session_state).await?; Ok(result) } QueryRoute::Write | QueryRoute::WriteTransaction => { diff --git a/src/session/read_only_handler.rs b/src/session/read_only_handler.rs index 3dd499b..8a5c6ce 100644 --- a/src/session/read_only_handler.rs +++ b/src/session/read_only_handler.rs @@ -4,6 +4,10 @@ use crate::config::Config; use std::sync::Arc; use std::time::Duration; use thiserror::Error; +use crate::session::SessionState; +use crate::query::projection_resolver::resolve_columns_with_legacy; +use crate::translator::TranslationMetadata; +use std::collections::HashMap; #[derive(Error, Debug)] pub enum ReadOnlyError { @@ -59,19 +63,22 @@ impl ReadOnlyDbHandler { } /// Execute a SELECT query using a pooled connection - pub async fn query(&self, sql: &str) -> Result { + pub async fn query(&self, sql: &str, session: &SessionState) -> Result { // Ensure this is a read-only operation if !is_read_only_query(sql) { return Err(ReadOnlyError::WriteNotAllowed); } + let legacy = session.legacy_result_columns().await; let conn = self.pool.acquire().await?; // Execute query using rusqlite let mut stmt = conn.prepare(sql)?; - let column_names: Vec = stmt.column_names() + let column_names: Vec = resolve_columns_with_legacy( + &stmt, sql, &HashMap::new(), &TranslationMetadata::new(), legacy, + )? .iter() - .map(|s| s.to_string()) + .map(|m| m.wire_name.clone()) .collect(); let rows = stmt.query_map([], |row| { @@ -107,19 +114,23 @@ impl ReadOnlyDbHandler { pub async fn query_with_params( &self, sql: &str, - params: &[&dyn rusqlite::ToSql] + params: &[&dyn rusqlite::ToSql], + session: &SessionState ) -> Result { // Ensure this is a read-only operation if !is_read_only_query(sql) { return Err(ReadOnlyError::WriteNotAllowed); } + let legacy = session.legacy_result_columns().await; let conn = self.pool.acquire().await?; let mut stmt = conn.prepare(sql)?; - let column_names: Vec = stmt.column_names() + let column_names: Vec = resolve_columns_with_legacy( + &stmt, sql, &HashMap::new(), &TranslationMetadata::new(), legacy, + )? .iter() - .map(|s| s.to_string()) + .map(|m| m.wire_name.clone()) .collect(); let rows = stmt.query_map(params, |row| { @@ -206,7 +217,8 @@ mod tests { let config = Arc::new(Config::load()); let handler = ReadOnlyDbHandler::new(":memory:", config).unwrap(); - let result = handler.query("INSERT INTO test VALUES (1)").await; + let session = SessionState::new_test(); + let result = handler.query("INSERT INTO test VALUES (1)", &session).await; assert!(matches!(result, Err(ReadOnlyError::WriteNotAllowed))); } } \ No newline at end of file From c1fe35937b850af0344eded7c3dc0481ee3b4447 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 17:16:38 -0700 Subject: [PATCH 10/21] fix: F2 datetime regression + F5 bare-name OID mis-typing (Task 7) F2 (executor.rs): match the direct MAX/MIN aggregate pattern against the original query text (parens intact) instead of the sanitized column name (stripped to bare 'max'), so MAX(created_at) gets datetime conversion. One-line change re.captures(col_name) -> re.captures(query), matching the adjacent scalar-subquery block. Added a regex-contract regression test. F5 (schema_type_mapper.rs): delete the bare-name OID match block added by 3b9c49f that ran before the query-regex fallback and mis-typed aliased non-aggregates (e.g. upper(name) AS count -> int8). Keep the bare-name guard and the query-regex fallback; bare names now fall through to None without query context. Added 3 guard tests (None without query, resolves with query, aliased non-aggregate not mistyped). Cargo.lock: incidental sync pgsqlite 0.0.21 -> 0.0.22 to match Cargo.toml. Tests: 528 lib tests pass (524 baseline + 4 new). --- Cargo.lock | 2 +- src/query/executor.rs | 23 ++++++++- src/types/schema_type_mapper.rs | 85 ++++++++++++++++++++------------- 3 files changed, 74 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3ed7115..3d1a550 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1254,7 +1254,7 @@ dependencies = [ [[package]] name = "pgsqlite" -version = "0.0.21" +version = "0.0.22" dependencies = [ "anyhow", "arbitrary", diff --git a/src/query/executor.rs b/src/query/executor.rs index 4b5b285..121188e 100644 --- a/src/query/executor.rs +++ b/src/query/executor.rs @@ -1322,8 +1322,9 @@ impl QueryExecutor { // Also check for direct MAX/MIN without subquery // Pattern: MAX(created_at) or MIN(created_at) let direct_pattern = r"(?i)(?:MAX|MIN)\s*\(\s*(\w+)\s*\)"; + // Match against the query (parens intact), not the sanitized col_name (F2). if let Ok(re) = regex::Regex::new(direct_pattern) - && let Some(captures) = re.captures(col_name) + && let Some(captures) = re.captures(query) && let Some(inner_col) = captures.get(1) { let inner_col_name = inner_col.as_str(); info!("Non-ultra path: Found direct aggregate: {}", col_name); @@ -2922,4 +2923,24 @@ mod tests { assert_eq!(&caps[2], "hex"); assert_eq!(&caps[3], "false"); } + + // Regression guard for F2: SELECT max(created_at) must still receive datetime + // conversion. Column-name sanitization strips parens, so the wire name is the + // bare "max"; the aggregate pattern must therefore be matched against the + // original query text (parens intact), not the sanitized column name. + #[test] + fn test_direct_aggregate_pattern_matches_query_not_sanitized_name() { + let direct_pattern = r"(?i)(?:MAX|MIN)\s*\(\s*(\w+)\s*\)"; + let re = regex::Regex::new(direct_pattern).unwrap(); + + // Matching the original query extracts the inner datetime column. + let caps = re.captures("SELECT max(created_at) FROM t") + .expect("direct aggregate pattern must match the query text"); + assert_eq!(caps.get(1).unwrap().as_str(), "created_at"); + + // The sanitized column name ("max", parens stripped) can never match — + // this is the F2 regression: matching the stripped name drops conversion. + assert!(re.captures("max").is_none()); + assert!(re.captures("MAX").is_none()); + } } \ No newline at end of file diff --git a/src/types/schema_type_mapper.rs b/src/types/schema_type_mapper.rs index 7c030e7..cc7f314 100644 --- a/src/types/schema_type_mapper.rs +++ b/src/types/schema_type_mapper.rs @@ -336,41 +336,11 @@ impl SchemaTypeMapper { ) -> Option { let upper = function_name.to_uppercase(); - // Handle bare function names (without parentheses). - // Column names may arrive without parentheses after sanitization strips them, - // e.g. "json_extract" instead of "json_extract(data, '$[0]')". - // Match known function names by exact equality to avoid false positives - // on columns named like "year_col" or "hour_trunc". + // Bare function names (no parens/spaces) arrive after column-name sanitization + // strips parentheses. Without query context we cannot safely type them, so fall + // through to the query-regex below (or None when unmatched / no query). + // NOTE: legacy=on db_handler-site type-chain consistency is tracked for later. if !function_name.contains('(') && !function_name.contains(' ') { - match upper.as_str() { - "COUNT" => return Some(PgType::Int8.to_oid()), - "SUM" | "AVG" => return Some(PgType::Numeric.to_oid()), - "MAX" | "MIN" => { - // Need schema lookup for proper type — fall through to query-based detection below - } - "JSON_ARRAY_LENGTH" => return Some(PgType::Int4.to_oid()), - "JSON_GROUP_ARRAY" | "JSON_ARRAY" | "JSON_OBJECT" | "JSON_EXTRACT" - | "JSON_AGG" | "JSON_OBJECT_AGG" | "JSONB_AGG" | "JSONB_OBJECT_AGG" | "ROW_TO_JSON" - | "JSON_EXTRACT_PATH" | "JSON_EXTRACT_PATH_TEXT" => return Some(PgType::Text.to_oid()), - "ARRAY_LENGTH" | "ARRAY_UPPER" | "ARRAY_LOWER" | "ARRAY_NDIMS" | "ARRAY_POSITION" => return Some(PgType::Int4.to_oid()), - "ARRAY_APPEND" | "ARRAY_PREPEND" | "ARRAY_CAT" | "ARRAY_REMOVE" - | "ARRAY_REPLACE" | "ARRAY_SLICE" | "STRING_TO_ARRAY" - | "ARRAY_POSITIONS" | "ARRAY_TO_STRING" | "UNNEST" - | "ARRAY_AGG" => return Some(PgType::Text.to_oid()), - "ARRAY_CONTAINS" | "ARRAY_CONTAINED" | "ARRAY_OVERLAP" => return Some(PgType::Bool.to_oid()), - "NOW" => return Some(PgType::Timestamptz.to_oid()), - "CURRENT_TIMESTAMP" => return Some(PgType::Timestamptz.to_oid()), - "CURRENT_DATE" => return Some(PgType::Text.to_oid()), - "CURRENT_TIME" => return Some(PgType::Time.to_oid()), - "EXTRACT" => return Some(PgType::Float8.to_oid()), - "DATE_TRUNC" | "TO_TIMESTAMP" => return Some(PgType::Timestamp.to_oid()), - "MAKE_DATE" => return Some(PgType::Date.to_oid()), - "MAKE_TIME" => return Some(PgType::Time.to_oid()), - "AGE" => return Some(PgType::Interval.to_oid()), - "EPOCH" => return Some(PgType::Timestamp.to_oid()), - "DECIMAL_ADD" | "DECIMAL_SUB" | "DECIMAL_MUL" | "DECIMAL_DIV" | "DECIMAL_FROM_TEXT" => return Some(PgType::Numeric.to_oid()), - _ => {} - } // If we have the query, try to find what function produces this alias if let Some(q) = query { @@ -551,4 +521,51 @@ impl SchemaTypeMapper { None } } +} + +#[cfg(test)] +mod tests { + use super::*; + + // F5: bare function names with NO query context must return None. The old + // bare-name OID block mis-typed aliased non-aggregates (e.g. `upper(name) AS + // count`) as int8. With the block deleted, bare names fall through to the + // query-regex (None when there's no query / no match). + #[test] + fn test_bare_aggregate_no_query_context_returns_none() { + for name in ["count", "sum", "avg", "max", "min", "json_extract", + "array_length", "now", "current_timestamp", "date_trunc", "age"] { + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query(name, None, None, None), + None, + "bare '{name}' without query context should resolve to None" + ); + } + } + + // The query-regex fallback (kept) still resolves aliased aggregates with query context. + #[test] + fn test_bare_aggregate_with_query_context_still_resolves() { + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "count", None, None, Some("SELECT count(*) AS count FROM t")), + Some(PgType::Int8.to_oid()) + ); + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "total", None, None, Some("SELECT sum(amount) AS total FROM t")), + Some(PgType::Numeric.to_oid()) + ); + } + + // F5 root cause: an aliased NON-aggregate like `upper(name) AS count` must NOT + // be typed as int8 just because the alias happens to be "count". + #[test] + fn test_aliased_nonaggregate_bare_name_not_mistyped() { + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "count", None, None, Some("SELECT upper(name) AS count FROM t")), + None + ); + } } \ No newline at end of file From a7ac55908befac79d07e8add8c8ac0fef38237dc Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 18:33:16 -0700 Subject: [PATCH 11/21] fix: extend F2/F5 fixes to extended-protocol path (psycopg3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 7 fixed F2 (max(timestamp) datetime regression) and F5 (bare-name count OID mis-typing) only on the simple-query path. psycopg3-binary is the RECOMMENDED driver, so the extended path needs the same fixes after Task 6 sanitization stripped parens from result column names. Fix A (F5): gate the COUNT(*) int8 backstop on the query containing 'count(' so upper(name) AS count is not mis-typed as int8. Two parallel instances in extended.rs (handle_parse @807 uses cleaned_query; execute_select @4856 uses portal.query) — both gated identically. count(*) FROM t stays int8; upper(name) AS count falls through to TEXT. Fix B (F2): widen the async-lookup gate to fire on sanitized bare max/min/sum/avg; add a direct-pattern regex matched against the QUERY (mirrors executor.rs F2) so SELECT max(created_at) FROM t resolves the inner column's datetime type_oid into aggregate_types, which feeds field_types and triggers the downstream microseconds->timestamp conversion. Non-datetime inner columns fall through (no regression). Added 3 contract-guard unit tests (extended.rs had none); live handlers are async (end-to-end deferred to Task 8). 531 lib tests pass. --- src/query/extended.rs | 112 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 106 insertions(+), 6 deletions(-) diff --git a/src/query/extended.rs b/src/query/extended.rs index e6af494..087c700 100644 --- a/src/query/extended.rs +++ b/src/query/extended.rs @@ -800,8 +800,12 @@ impl ExtendedQueryHandler { continue; // Important: continue here to prevent value-based inference from overriding } - // Special case for COUNT(*) which might have a different column name - if col_lower == "count(*)" || col_lower == "count" { + // Special case for COUNT(*) whose column name is sanitized to "count". + // Gate on the query so a non-aggregate aliased to "count" (e.g. upper(name) AS count) + // is NOT mis-typed as int8 (F5). Only a real count(...) aggregate gets int8. + // Parallel instance at ~line 4820 (execute_select) is gated the same way. + if (col_lower == "count(*)" || col_lower == "count") + && cleaned_query.to_lowercase().contains("count(") { info!("Column '{}' is COUNT aggregate, using INT8 type", col_name); inferred_types.push(PgType::Int8.to_oid()); continue; @@ -4567,8 +4571,14 @@ impl ExtendedQueryHandler { let mut async_lookups_needed = Vec::new(); for (i, col_name) in response.columns.iter().enumerate() { let col_lower = col_name.to_lowercase(); - if col_lower.contains("max(") || col_lower.contains("min(") || - col_lower.contains("sum(") || col_lower.contains("avg(") { + // After Task 6 sanitization the wire name for max(col)/min(col)/sum(col)/ + // avg(col) is bare "max"/"min"/"sum"/"avg" (parens stripped). Trigger the async + // lookup on those bare names too, so direct-aggregate handling below resolves the + // inner column's type from the QUERY text (F2 on the extended path). + if col_lower.contains("max(") || col_lower.contains("min(") || + col_lower.contains("sum(") || col_lower.contains("avg(") || + col_lower == "max" || col_lower == "min" || + col_lower == "sum" || col_lower == "avg" { async_lookups_needed.push((i, col_name.clone())); } } @@ -4579,6 +4589,8 @@ impl ExtendedQueryHandler { // Pre-compile regex patterns to avoid recompilation in loop let max_regex = regex::Regex::new(r"\(\s*SELECT\s+MAX\s*\(\s*(\w+)\s*\)\s+FROM\s+(\w+)\s*\)").ok(); let min_regex = regex::Regex::new(r"\(\s*SELECT\s+MIN\s*\(\s*(\w+)\s*\)\s+FROM\s+(\w+)\s*\)").ok(); + // Direct (non-subquery) MAX/MIN, matched against the QUERY (parens intact) — F2. + let direct_regex = regex::Regex::new(r"(?i)(?:MAX|MIN)\s*\(\s*(\w+)\s*\)").ok(); for (idx, col_name) in async_lookups_needed { // Extract the aggregate function and column @@ -4626,6 +4638,27 @@ impl ExtendedQueryHandler { } } + // Direct (non-subquery) MAX/MIN datetime handling (F2 on the extended path): + // SELECT max(created_at) FROM t has no scalar subquery, so extract the inner + // column from the QUERY and, if it's a datetime type, record its oid so the + // downstream value converter (gated on type_oid) reformats INTEGER microseconds. + if let Some(re) = &direct_regex + && let Some(captures) = re.captures(query) + && let Some(inner_col) = captures.get(1) { + let inner_col_name = inner_col.as_str(); + if let Some(ref table) = table_name + && let Ok(Some(pg_type)) = db.get_schema_type_with_session(&session.id, table, inner_col_name).await { + let upper_type = pg_type.to_uppercase(); + if upper_type.contains("TIMESTAMP") || upper_type.contains("DATE") || upper_type.contains("TIME") { + let type_oid = crate::types::SchemaTypeMapper::pg_type_string_to_oid(&pg_type); + info!("Direct aggregate {}({}) from table {} has datetime type {} (OID {})", + col_lower, inner_col_name, table, pg_type, type_oid); + aggregate_types.insert(idx, type_oid); + continue; + } + } + } + // Fallback to generic aggregate type detection if let Some(oid) = crate::types::SchemaTypeMapper::get_aggregate_return_type_with_query( &col_lower, None, lookup_table.as_deref(), Some(query) @@ -4816,8 +4849,12 @@ impl ExtendedQueryHandler { continue; } - // Special case for COUNT(*) which might have a different column name - if col_lower == "count(*)" || col_lower == "count" { + // Special case for COUNT(*) whose column name is sanitized to "count". + // Gate on the query (portal.query here) so a non-aggregate aliased to "count" + // (e.g. upper(name) AS count) is NOT mis-typed as int8 (F5). Parallel instance + // of the handle_parse backstop above (~line 804). + if (col_lower == "count(*)" || col_lower == "count") + && portal.query.to_lowercase().contains("count(") { info!("Column '{}' is COUNT aggregate, using INT8 type", col_name); field_types.push(PgType::Int8.to_oid()); continue; @@ -6277,4 +6314,67 @@ fn extract_table_name_from_create(query: &str) -> Option { } else { None } +} + +#[cfg(test)] +mod tests { + use regex::Regex; + + // Fix B (F2): the direct-aggregate regex must be matched against the QUERY text + // (parens intact), never the sanitized column name ("max"/"min", parens stripped by + // Task 6). Mirrors executor.rs::test_direct_aggregate_pattern_matches_query_not_sanitized_name. + // The live extended handler is async + needs a session/DB (deferred to Task 8); this + // guards the regex contract the direct-pattern handling relies on. + #[test] + fn test_extended_direct_aggregate_pattern_matches_query() { + let re = Regex::new(r"(?i)(?:MAX|MIN)\s*\(\s*(\w+)\s*\)").unwrap(); + + let caps = re.captures("SELECT max(created_at) FROM t") + .expect("direct aggregate pattern must match the query text"); + assert_eq!(caps.get(1).unwrap().as_str(), "created_at"); + + // The sanitized names can never match — matching the stripped name would drop + // datetime conversion (the F2 regression on the extended path). + assert!(re.captures("max").is_none()); + assert!(re.captures("MIN").is_none()); + } + + // Fix B (F2): after Task 6 sanitization the wire names for max/min/sum/avg are bare + // (parens stripped); the async-lookup gate must fire on those bare names so the + // direct-aggregate handling runs. The pre-fix (parens-in-name) gate must NOT. + #[test] + fn test_extended_aggregate_gate_triggers_on_sanitized_bare_names() { + for n in ["max", "min", "sum", "avg"] { + let col_lower = n.to_lowercase(); + let widened = col_lower.contains("max(") || col_lower.contains("min(") + || col_lower.contains("sum(") || col_lower.contains("avg(") + || col_lower == "max" || col_lower == "min" + || col_lower == "sum" || col_lower == "avg"; + assert!(widened, "gate must fire on sanitized bare name {:?}", n); + + let pre_fix = col_lower.contains("max(") || col_lower.contains("min(") + || col_lower.contains("sum(") || col_lower.contains("avg("); + assert!(!pre_fix, "pre-fix gate must not fire on bare name {:?}", n); + } + } + + // Fix A (F5): the COUNT(*) backstop must only fire when the query actually contains a + // count(...) aggregate, so a non-aggregate aliased to "count" (e.g. upper(name) AS count) + // is NOT mis-typed as int8. Applied at both the handle_parse site (cleaned_query) and the + // execute_select site (portal.query) — same predicate, different in-scope query variable. + #[test] + fn test_extended_count_backstop_gated_on_query_count_paren() { + let backstop_applies = |col_lower: &str, query: &str| -> bool { + (col_lower == "count(*)" || col_lower == "count") + && query.to_lowercase().contains("count(") + }; + + // Real count(*) stays int8. + assert!(backstop_applies("count", "SELECT count(*) FROM t")); + assert!(backstop_applies("count(*)", "SELECT COUNT(*) FROM t")); + + // Aliased non-aggregate upper(name) AS count must NOT be int8. + assert!(!backstop_applies("count", "SELECT upper(name) AS count FROM t")); + assert!(!backstop_applies("count", "SELECT max(x) AS count FROM t")); + } } \ No newline at end of file From 158d58db86855427d8eb90b385c9bf4e1611ce1e Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 19:15:15 -0700 Subject: [PATCH 12/21] fix: place F2 direct-pattern handler in handle_parse (prepared-statement path) --- src/query/extended.rs | 159 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 128 insertions(+), 31 deletions(-) diff --git a/src/query/extended.rs b/src/query/extended.rs index 087c700..ec30b80 100644 --- a/src/query/extended.rs +++ b/src/query/extended.rs @@ -794,6 +794,21 @@ impl ExtendedQueryHandler { // Third priority: Check for aggregate functions let col_lower = col_name.to_lowercase(); + + // Direct (non-subquery) MAX/MIN datetime handling for prepared statements (F2): + // handle_parse populates field_descriptions; BLOCK 2 later copies those oids into + // field_types for encode_row, which performs INTEGER microseconds -> datetime text + // conversion based on the datetime type oid. + if let Some(inner_col) = direct_aggregate_inner_column(&col_lower, &cleaned_query) + && let Some(ref table) = table_name + && let Ok(Some(pg_type)) = db.get_schema_type_with_session(&session.id, table, &inner_col).await + && let Some(type_oid) = datetime_oid_from_type_str(&pg_type) { + info!("PARSE: Direct aggregate {}({}) from table {} has datetime type {} (OID {})", + col_lower, inner_col, table, pg_type, type_oid); + inferred_types.push(type_oid); + continue; + } + if let Some(oid) = crate::types::SchemaTypeMapper::get_aggregate_return_type_with_query(&col_lower, None, None, Some(&cleaned_query)) { info!("Column '{}' identified with type OID {} from aggregate detection", col_name, oid); inferred_types.push(oid); @@ -804,8 +819,7 @@ impl ExtendedQueryHandler { // Gate on the query so a non-aggregate aliased to "count" (e.g. upper(name) AS count) // is NOT mis-typed as int8 (F5). Only a real count(...) aggregate gets int8. // Parallel instance at ~line 4820 (execute_select) is gated the same way. - if (col_lower == "count(*)" || col_lower == "count") - && cleaned_query.to_lowercase().contains("count(") { + if count_backstop_applies(&col_lower, &cleaned_query) { info!("Column '{}' is COUNT aggregate, using INT8 type", col_name); inferred_types.push(PgType::Int8.to_oid()); continue; @@ -4575,10 +4589,7 @@ impl ExtendedQueryHandler { // avg(col) is bare "max"/"min"/"sum"/"avg" (parens stripped). Trigger the async // lookup on those bare names too, so direct-aggregate handling below resolves the // inner column's type from the QUERY text (F2 on the extended path). - if col_lower.contains("max(") || col_lower.contains("min(") || - col_lower.contains("sum(") || col_lower.contains("avg(") || - col_lower == "max" || col_lower == "min" || - col_lower == "sum" || col_lower == "avg" { + if is_bare_aggregate_candidate(&col_lower) { async_lookups_needed.push((i, col_name.clone())); } } @@ -4853,8 +4864,7 @@ impl ExtendedQueryHandler { // Gate on the query (portal.query here) so a non-aggregate aliased to "count" // (e.g. upper(name) AS count) is NOT mis-typed as int8 (F5). Parallel instance // of the handle_parse backstop above (~line 804). - if (col_lower == "count(*)" || col_lower == "count") - && portal.query.to_lowercase().contains("count(") { + if count_backstop_applies(&col_lower, &portal.query) { info!("Column '{}' is COUNT aggregate, using INT8 type", col_name); field_types.push(PgType::Int8.to_oid()); continue; @@ -6316,9 +6326,58 @@ fn extract_table_name_from_create(query: &str) -> Option { } } +// --- Direct-aggregate / count-backstop helpers (F2/F5 on the extended path) --- +// Pure predicates extracted so the production handlers (handle_parse + execute_select) and +// the unit tests share one implementation (reviewer: prior tests asserted local copies). + +/// Direct (non-subquery) MAX/MIN detection for the F2 fix on the prepared-statement path. +/// `col_lower` is the lowercased result-column name (after Task 6 sanitization strips parens, +/// so "max"/"min"). The aggregate pattern is matched against the QUERY text (parens intact), +/// never the sanitized name. Returns the inner column name when matched, else None. +fn direct_aggregate_inner_column(col_lower: &str, query: &str) -> Option { + if col_lower != "max" && col_lower != "min" { + return None; + } + let re = regex::Regex::new(r"(?i)(?:MAX|MIN)\s*\(\s*(\w+)\s*\)").ok()?; + re.captures(query).and_then(|c| c.get(1).map(|m| m.as_str().to_string())) +} + +/// Map a resolved schema type string to a datetime OID when it is a DATE/TIME/TIMESTAMP +/// variant; returns None for non-datetime types so they fall through to the generic +/// aggregate path (preserves max(price) -> NUMERIC etc.). +fn datetime_oid_from_type_str(pg_type: &str) -> Option { + let upper = pg_type.to_uppercase(); + if upper.contains("TIMESTAMP") || upper.contains("DATE") || upper.contains("TIME") { + Some(crate::types::SchemaTypeMapper::pg_type_string_to_oid(pg_type)) + } else { + None + } +} + +/// F5 count(*) backstop: only a real count(...) aggregate (gate on the query containing +/// "count(") gets int8, so a non-aggregate aliased to "count" is NOT mis-typed. Shared by +/// handle_parse (cleaned_query) and execute_select (portal.query). +fn count_backstop_applies(col_lower: &str, query: &str) -> bool { + (col_lower == "count(*)" || col_lower == "count") && query.to_lowercase().contains("count(") +} + +/// Gate for the extended Execute-path aggregate async lookup: fires on the bare sanitized +/// names "max"/"min"/"sum"/"avg" (parens stripped by Task 6) and the parens-in-name form. +fn is_bare_aggregate_candidate(col_lower: &str) -> bool { + col_lower.contains("max(") || col_lower.contains("min(") + || col_lower.contains("sum(") || col_lower.contains("avg(") + || col_lower == "max" || col_lower == "min" + || col_lower == "sum" || col_lower == "avg" +} + #[cfg(test)] mod tests { use regex::Regex; + use super::{ + count_backstop_applies, datetime_oid_from_type_str, direct_aggregate_inner_column, + is_bare_aggregate_candidate, + }; + use crate::types::PgType; // Fix B (F2): the direct-aggregate regex must be matched against the QUERY text // (parens intact), never the sanitized column name ("max"/"min", parens stripped by @@ -6339,42 +6398,80 @@ mod tests { assert!(re.captures("MIN").is_none()); } - // Fix B (F2): after Task 6 sanitization the wire names for max/min/sum/avg are bare + // F2 production behavior: for SELECT max(created_at) FROM t the handle_parse direct-pattern + // handler must resolve a datetime oid into field_descriptions (so BLOCK 2's else-branch + // feeds it to encode_row -> microseconds->timestamp). handle_parse itself is async + DB + + // portal (end-to-end deferred to Task 8), so this asserts the pure production helpers the + // handler calls. FAILS if the datetime oid isn't produced for max(created_at). + #[test] + fn test_handle_parse_direct_aggregate_datetime_oid() { + // 1. The bare sanitized name is "max"; the inner column is captured from the QUERY. + let inner = direct_aggregate_inner_column("max", "SELECT max(created_at) FROM t") + .expect("direct-pattern must capture inner column from the query"); + assert_eq!(inner, "created_at"); + + // 2. A TIMESTAMP inner column resolves to the Timestamp oid (datetime conversion). + let oid = datetime_oid_from_type_str("TIMESTAMP") + .expect("TIMESTAMP must map to a datetime oid"); + assert_eq!(oid, PgType::Timestamp.to_oid()); + + // Other datetime variants also map. + assert_eq!(datetime_oid_from_type_str("TIMESTAMPTZ").unwrap(), PgType::Timestamptz.to_oid()); + assert_eq!(datetime_oid_from_type_str("DATE").unwrap(), PgType::Date.to_oid()); + assert_eq!(datetime_oid_from_type_str("TIME").unwrap(), PgType::Time.to_oid()); + + // 3. A non-datetime column must NOT produce a datetime oid — it falls through to the + // generic aggregate path, preserving max(price) -> NUMERIC etc. + assert!(datetime_oid_from_type_str("INTEGER").is_none()); + assert!(datetime_oid_from_type_str("NUMERIC").is_none()); + assert!(datetime_oid_from_type_str("TEXT").is_none()); + + // 4. min(created_at) likewise captures the inner column. + assert_eq!( + direct_aggregate_inner_column("min", "SELECT min(created_at) FROM t").unwrap(), + "created_at" + ); + + // 5. The sanitized bare name (parens stripped) must never match — matching it would + // silently drop datetime conversion (the F2 regression). + assert!(direct_aggregate_inner_column("max", "max").is_none()); + + // 6. Non max/min bare names are not direct-aggregate candidates here (sum/avg use the + // generic aggregate path, not the datetime handler). + assert!(direct_aggregate_inner_column("sum", "SELECT sum(x) FROM t").is_none()); + assert!(direct_aggregate_inner_column("avg", "SELECT avg(x) FROM t").is_none()); + } + + // Fix B (F2 gate): after Task 6 sanitization the wire names for max/min/sum/avg are bare // (parens stripped); the async-lookup gate must fire on those bare names so the - // direct-aggregate handling runs. The pre-fix (parens-in-name) gate must NOT. + // direct-aggregate handling runs. Asserts the PRODUCTION gate helper. #[test] fn test_extended_aggregate_gate_triggers_on_sanitized_bare_names() { for n in ["max", "min", "sum", "avg"] { - let col_lower = n.to_lowercase(); - let widened = col_lower.contains("max(") || col_lower.contains("min(") - || col_lower.contains("sum(") || col_lower.contains("avg(") - || col_lower == "max" || col_lower == "min" - || col_lower == "sum" || col_lower == "avg"; - assert!(widened, "gate must fire on sanitized bare name {:?}", n); - - let pre_fix = col_lower.contains("max(") || col_lower.contains("min(") - || col_lower.contains("sum(") || col_lower.contains("avg("); - assert!(!pre_fix, "pre-fix gate must not fire on bare name {:?}", n); + assert!(is_bare_aggregate_candidate(n), "gate must fire on sanitized bare name {:?}", n); + assert!( + is_bare_aggregate_candidate(&format!("{}(col)", n)), + "gate must fire on parens-in-name {:?}(col)", + n + ); } + // A non-aggregate bare name must not trigger the gate; count is handled by its backstop. + assert!(!is_bare_aggregate_candidate("name")); + assert!(!is_bare_aggregate_candidate("count")); } // Fix A (F5): the COUNT(*) backstop must only fire when the query actually contains a // count(...) aggregate, so a non-aggregate aliased to "count" (e.g. upper(name) AS count) - // is NOT mis-typed as int8. Applied at both the handle_parse site (cleaned_query) and the - // execute_select site (portal.query) — same predicate, different in-scope query variable. + // is NOT mis-typed as int8. Asserts the PRODUCTION backstop helper shared by handle_parse + // (cleaned_query) and execute_select (portal.query). #[test] fn test_extended_count_backstop_gated_on_query_count_paren() { - let backstop_applies = |col_lower: &str, query: &str| -> bool { - (col_lower == "count(*)" || col_lower == "count") - && query.to_lowercase().contains("count(") - }; - // Real count(*) stays int8. - assert!(backstop_applies("count", "SELECT count(*) FROM t")); - assert!(backstop_applies("count(*)", "SELECT COUNT(*) FROM t")); + assert!(count_backstop_applies("count", "SELECT count(*) FROM t")); + assert!(count_backstop_applies("count(*)", "SELECT COUNT(*) FROM t")); // Aliased non-aggregate upper(name) AS count must NOT be int8. - assert!(!backstop_applies("count", "SELECT upper(name) AS count FROM t")); - assert!(!backstop_applies("count", "SELECT max(x) AS count FROM t")); + assert!(!count_backstop_applies("count", "SELECT upper(name) AS count FROM t")); + assert!(!count_backstop_applies("count", "SELECT max(x) AS count FROM t")); } } \ No newline at end of file From d5448b883d8d27546f50858350f6259382ce3f1b Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 19:27:14 -0700 Subject: [PATCH 13/21] fix: catalog result column conformance --- src/catalog/pg_roles.rs | 76 +++++++++++++++++++++++---- src/catalog/query_interceptor.rs | 89 +++++++++++++++++++++++++++----- tests/catalog_alias_test.rs | 48 +++++++++++++++++ 3 files changed, 190 insertions(+), 23 deletions(-) diff --git a/src/catalog/pg_roles.rs b/src/catalog/pg_roles.rs index 30e2979..6cb502f 100644 --- a/src/catalog/pg_roles.rs +++ b/src/catalog/pg_roles.rs @@ -44,6 +44,14 @@ impl PgRolesHandler { roles }; + if selected_columns.is_empty() && Self::is_count_star_projection(&select.projection) { + return Ok(DbResponse { + columns: vec!["count".to_string()], + rows: vec![vec![Some(filtered_roles.len().to_string().into_bytes())]], + rows_affected: 1, + }); + } + // Build response let mut rows = Vec::new(); for role in filtered_roles { @@ -83,20 +91,19 @@ impl PgRolesHandler { .iter() .map(|column| (column.clone(), column.clone())), ); - break; } SelectItem::UnnamedExpr(expr) => { - if let Some(col_name) = Self::extract_source_column(expr) { - if all_columns.contains(&col_name) { - selected.push((col_name.clone(), col_name)); - } + if let Some(col_name) = Self::extract_source_column(expr) + && all_columns.contains(&col_name) + { + selected.push((col_name.clone(), col_name)); } } SelectItem::ExprWithAlias { expr, alias } => { - if let Some(col_name) = Self::extract_source_column(expr) { - if all_columns.contains(&col_name) { - selected.push((alias.value.clone(), col_name)); - } + if let Some(col_name) = Self::extract_source_column(expr) + && all_columns.contains(&col_name) + { + selected.push((Self::alias_output_name(alias), col_name)); } } SelectItem::QualifiedWildcard(_, _) => { @@ -106,7 +113,6 @@ impl PgRolesHandler { .iter() .map(|column| (column.clone(), column.clone())), ); - break; } } } @@ -120,10 +126,60 @@ impl PgRolesHandler { Expr::CompoundIdentifier(parts) => parts.last().map(|ident| ident.value.to_lowercase()), Expr::Cast { expr, .. } => Self::extract_source_column(expr), Expr::Nested(expr) => Self::extract_source_column(expr), + Expr::Function(function) => Self::function_name(function), _ => None, } } + fn alias_output_name(alias: &sqlparser::ast::Ident) -> String { + if alias.quote_style.is_some() { + alias.value.clone() + } else { + alias.value.to_lowercase() + } + } + + fn function_name(function: &sqlparser::ast::Function) -> Option { + function + .name + .0 + .last() + .and_then(|part| part.as_ident()) + .map(|ident| ident.value.to_lowercase()) + } + + fn is_count_star_projection(projection: &[SelectItem]) -> bool { + if projection.len() != 1 { + return false; + } + + let expr = match &projection[0] { + SelectItem::UnnamedExpr(expr) => expr, + SelectItem::ExprWithAlias { expr, .. } => expr, + _ => return false, + }; + + let Expr::Function(function) = expr else { + return false; + }; + + if Self::function_name(function).as_deref() != Some("count") { + return false; + } + + matches!( + &function.args, + sqlparser::ast::FunctionArguments::List(arg_list) + if arg_list.args.len() == 1 + && matches!( + &arg_list.args[0], + sqlparser::ast::FunctionArg::Unnamed( + sqlparser::ast::FunctionArgExpr::Wildcard + ) + ) + ) + } + fn get_default_roles() -> Vec>> { let mut roles = Vec::new(); diff --git a/src/catalog/query_interceptor.rs b/src/catalog/query_interceptor.rs index 0497afa..9ee67af 100644 --- a/src/catalog/query_interceptor.rs +++ b/src/catalog/query_interceptor.rs @@ -1068,6 +1068,10 @@ impl CatalogInterceptor { Some("public".to_string().into_bytes()), ], ]; + if columns.is_empty() && Self::is_count_star_projection(&select.projection) { + return Self::count_response(full_rows.len()); + } + let rows: Vec>>> = full_rows .into_iter() .map(|full_row| { @@ -1079,7 +1083,7 @@ impl CatalogInterceptor { .collect(); let rows_affected = rows.len(); - debug!("Returning {} rows for pg_type query with {} columns: {:?}", rows_affected, columns.len(), columns); + debug!("Returning {} rows for pg_namespace query with {} columns: {:?}", rows_affected, columns.len(), columns); DbResponse { columns, rows, @@ -1740,25 +1744,28 @@ impl CatalogInterceptor { for item in &select.projection { match item { SelectItem::UnnamedExpr(expr) => { - if let Some(col_name) = Self::extract_projection_source_column(expr) { - if let Some(idx) = all_columns.iter().position(|c| c == &col_name) { - cols.push(all_columns[idx].clone()); - indices.push(idx); - } + if let Some(col_name) = Self::extract_projection_source_column(expr) + && let Some(idx) = all_columns.iter().position(|c| c == &col_name) + { + cols.push(all_columns[idx].clone()); + indices.push(idx); } } SelectItem::ExprWithAlias { expr, alias } => { - if let Some(col_name) = Self::extract_projection_source_column(expr) { - if let Some(idx) = all_columns.iter().position(|c| c == &col_name) { - cols.push(alias.value.clone()); - indices.push(idx); - } + if let Some(col_name) = Self::extract_projection_source_column(expr) + && let Some(idx) = all_columns.iter().position(|c| c == &col_name) + { + cols.push(Self::alias_output_name(alias)); + indices.push(idx); } } - SelectItem::Wildcard(_) | SelectItem::QualifiedWildcard(_, _) => { + SelectItem::Wildcard(_) => { + cols.extend_from_slice(all_columns); + indices.extend(0..all_columns.len()); + } + SelectItem::QualifiedWildcard(_, _) => { cols.extend_from_slice(all_columns); indices.extend(0..all_columns.len()); - break; } } } @@ -1773,10 +1780,66 @@ impl CatalogInterceptor { } Expr::Cast { expr, .. } => Self::extract_projection_source_column(expr), Expr::Nested(expr) => Self::extract_projection_source_column(expr), + Expr::Function(f) => Self::function_name(f), _ => None, } } + fn alias_output_name(alias: &sqlparser::ast::Ident) -> String { + if alias.quote_style.is_some() { + alias.value.clone() + } else { + alias.value.to_lowercase() + } + } + + fn function_name(function: &sqlparser::ast::Function) -> Option { + function + .name + .0 + .last() + .and_then(|part| part.as_ident()) + .map(|ident| ident.value.to_lowercase()) + } + + fn is_count_star_projection(projection: &[SelectItem]) -> bool { + if projection.len() != 1 { + return false; + } + + let expr = match &projection[0] { + SelectItem::UnnamedExpr(expr) => expr, + SelectItem::ExprWithAlias { expr, .. } => expr, + _ => return false, + }; + + let Expr::Function(function) = expr else { + return false; + }; + + if Self::function_name(function).as_deref() != Some("count") { + return false; + } + + matches!( + &function.args, + sqlparser::ast::FunctionArguments::List(arg_list) + if arg_list.args.len() == 1 + && matches!( + &arg_list.args[0], + FunctionArg::Unnamed(FunctionArgExpr::Wildcard) + ) + ) + } + + fn count_response(count: usize) -> DbResponse { + DbResponse { + columns: vec!["count".to_string()], + rows: vec![vec![Some(count.to_string().into_bytes())]], + rows_affected: 1, + } + } + async fn handle_information_schema_schemata_query(select: &Select, _db: &DbHandler) -> DbResponse { debug!("Handling information_schema.schemata query"); diff --git a/tests/catalog_alias_test.rs b/tests/catalog_alias_test.rs index 6446bf1..61899db 100644 --- a/tests/catalog_alias_test.rs +++ b/tests/catalog_alias_test.rs @@ -44,6 +44,54 @@ async fn test_pg_catalog_namespace_alias_projection() { assert_eq!(text_cell(&response.rows[1], 0), "2200"); } +#[tokio::test] +async fn test_catalog_unquoted_aliases_fold_to_lowercase_and_quoted_aliases_preserve_case() { + let response = catalog_query( + "SELECT oid AS MixedAlias, nspname AS \"SchemaName\" FROM pg_catalog.pg_namespace", + ) + .await; + + assert_eq!(response.columns, vec!["mixedalias", "SchemaName"]); + assert_eq!(response.rows.len(), 2); + assert_eq!(text_cell(&response.rows[0], 0), "11"); + assert_eq!(text_cell(&response.rows[0], 1), "pg_catalog"); +} + +#[tokio::test] +async fn test_catalog_wildcard_keeps_trailing_projection_items() { + let namespace = catalog_query("SELECT *, oid FROM pg_catalog.pg_namespace").await; + + assert_eq!(namespace.columns, vec!["oid", "nspname", "oid"]); + assert_eq!(namespace.rows.len(), 2); + assert_eq!(text_cell(&namespace.rows[0], 0), "11"); + assert_eq!(text_cell(&namespace.rows[0], 2), "11"); + + let roles = catalog_query("SELECT *, oid FROM pg_catalog.pg_roles").await; + + assert_eq!(roles.columns.len(), 14); + assert_eq!(roles.columns[0], "oid"); + assert_eq!(roles.columns[13], "oid"); + assert_eq!(roles.rows.len(), 3); + assert_eq!(text_cell(&roles.rows[0], 0), "10"); + assert_eq!(text_cell(&roles.rows[0], 13), "10"); +} + +#[tokio::test] +async fn test_catalog_count_star_returns_static_dataset_count() { + let namespace = catalog_query("SELECT count(*) FROM pg_catalog.pg_namespace").await; + + assert_eq!(namespace.columns, vec!["count"]); + assert_eq!(namespace.rows.len(), 1); + assert_eq!(text_cell(&namespace.rows[0], 0), "2"); + + let roles = + catalog_query("SELECT count(*) FROM pg_catalog.pg_roles WHERE rolcanlogin = 't'").await; + + assert_eq!(roles.columns, vec!["count"]); + assert_eq!(roles.rows.len(), 1); + assert_eq!(text_cell(&roles.rows[0], 0), "2"); +} + #[tokio::test] async fn test_information_schema_schemata_alias_projection() { let response = catalog_query("SELECT catalog_name AS x FROM information_schema.schemata").await; From f30a85f999644a14f36368850366e18fc3a10c7b Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 19:32:33 -0700 Subject: [PATCH 14/21] test: expand catalog_alias_test coverage (F10); remove stray test_batch_7b.py (F9) --- tests/catalog_alias_test.rs | 51 +++++++++++++++++++++++++++++++++++++ tests/test_batch_7b.py | 45 -------------------------------- 2 files changed, 51 insertions(+), 45 deletions(-) delete mode 100644 tests/test_batch_7b.py diff --git a/tests/catalog_alias_test.rs b/tests/catalog_alias_test.rs index 61899db..ef08957 100644 --- a/tests/catalog_alias_test.rs +++ b/tests/catalog_alias_test.rs @@ -92,6 +92,57 @@ async fn test_catalog_count_star_returns_static_dataset_count() { assert_eq!(text_cell(&roles.rows[0], 0), "2"); } +#[tokio::test] +async fn test_pg_roles_unquoted_aliases_fold_to_lowercase_and_quoted_aliases_preserve_case() { + let response = catalog_query( + "SELECT rolname AS DisplayName, oid AS \"RoleOid\" FROM pg_roles", + ) + .await; + + assert_eq!(response.columns, vec!["displayname", "RoleOid"]); + assert_eq!(response.rows.len(), 3); + assert_eq!(text_cell(&response.rows[0], 0), "postgres"); + assert_eq!(text_cell(&response.rows[0], 1), "10"); +} + +#[tokio::test] +async fn test_pg_namespace_cast_projection_uses_inner_source_column() { + let response = catalog_query("SELECT CAST(oid AS text) AS o FROM pg_catalog.pg_namespace").await; + + assert_eq!(response.columns, vec!["o"]); + assert_eq!(response.rows.len(), 2); + assert_eq!(text_cell(&response.rows[0], 0), "11"); + assert_eq!(text_cell(&response.rows[1], 0), "2200"); +} + +#[tokio::test] +async fn test_pg_namespace_compound_identifier_projection_uses_leaf_column() { + let response = catalog_query("SELECT n.nspname FROM pg_catalog.pg_namespace AS n").await; + + assert_eq!(response.columns, vec!["nspname"]); + assert_eq!(response.rows.len(), 2); + assert_eq!(text_cell(&response.rows[0], 0), "pg_catalog"); + assert_eq!(text_cell(&response.rows[1], 0), "public"); +} + +#[tokio::test] +async fn test_pg_roles_where_filter_with_alias_free_projection() { + let response = catalog_query("SELECT rolname FROM pg_roles WHERE rolname = 'postgres'").await; + + assert_eq!(response.columns, vec!["rolname"]); + assert_eq!(response.rows.len(), 1); + assert_eq!(text_cell(&response.rows[0], 0), "postgres"); +} + +#[tokio::test] +async fn test_pg_namespace_unknown_source_column_projects_no_columns() { + let response = catalog_query("SELECT nonexistent FROM pg_catalog.pg_namespace").await; + + assert!(response.columns.is_empty()); + assert_eq!(response.rows.len(), 2); + assert!(response.rows.iter().all(Vec::is_empty)); +} + #[tokio::test] async fn test_information_schema_schemata_alias_projection() { let response = catalog_query("SELECT catalog_name AS x FROM information_schema.schemata").await; diff --git a/tests/test_batch_7b.py b/tests/test_batch_7b.py deleted file mode 100644 index 1c2ddea..0000000 --- a/tests/test_batch_7b.py +++ /dev/null @@ -1,45 +0,0 @@ -#!/usr/bin/env python3 -"""Quick batch test for 7B model""" -import time -import requests -import asyncio -import aiohttp - -API_URL = "http://localhost:8000/v1/chat/completions" - -async def send_request(session, idx): - payload = { - "model": "qwen2.5-7b-instruct", - "messages": [{"role": "user", "content": "What is Rust?"}], - "max_tokens": 50 - } - start = time.time() - async with session.post(API_URL, json=payload) as resp: - result = await resp.json() - elapsed = time.time() - start - tokens = result["usage"]["completion_tokens"] - return elapsed, tokens - -async def test_concurrent(): - print("Testing 3 concurrent requests...") - start = time.time() - - async with aiohttp.ClientSession() as session: - tasks = [send_request(session, i) for i in range(3)] - results = await asyncio.gather(*tasks) - - total_time = time.time() - start - - print(f"\nResults:") - for i, (elapsed, tokens) in enumerate(results): - print(f" Request {i+1}: {elapsed:.2f}s ({tokens} tokens)") - - total_tokens = sum(r[1] for r in results) - aggregate_throughput = total_tokens / total_time - - print(f"\nTotal time: {total_time:.2f}s") - print(f"Total tokens: {total_tokens}") - print(f"Aggregate throughput: {aggregate_throughput:.1f} tokens/sec") - -if __name__ == "__main__": - asyncio.run(test_concurrent()) From 56008963d87fe62cf1ac30c6adab0374059d7ca5 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 19:35:54 -0700 Subject: [PATCH 15/21] test: add nested catalog projection coverage --- tests/catalog_alias_test.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/catalog_alias_test.rs b/tests/catalog_alias_test.rs index ef08957..1047d2c 100644 --- a/tests/catalog_alias_test.rs +++ b/tests/catalog_alias_test.rs @@ -115,6 +115,15 @@ async fn test_pg_namespace_cast_projection_uses_inner_source_column() { assert_eq!(text_cell(&response.rows[1], 0), "2200"); } +#[tokio::test] +async fn test_pg_namespace_nested_projection_uses_inner_source_column() { + let response = catalog_query("SELECT (oid) AS o FROM pg_catalog.pg_namespace").await; + + assert_eq!(response.columns, vec!["o"]); + assert_eq!(response.rows.len(), 2); + assert_eq!(response.rows[0][0].as_deref(), Some(b"11".as_ref())); +} + #[tokio::test] async fn test_pg_namespace_compound_identifier_projection_uses_leaf_column() { let response = catalog_query("SELECT n.nspname FROM pg_catalog.pg_namespace AS n").await; From 424b54f5285853ae7375bba00a4fd6e0a9ec90d0 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 20:25:20 -0700 Subject: [PATCH 16/21] test: end-to-end column conformance; remove column_sanitizer.rs --- src/cache/statement_pool.rs | 20 +++--- src/catalog/query_interceptor.rs | 26 +++++--- src/optimization/read_only_optimizer.rs | 16 ++--- src/query/column_sanitizer.rs | 66 -------------------- src/query/extended.rs | 7 ++- src/query/fast_path.rs | 36 +++++++---- src/query/mod.rs | 1 - src/query/projection_resolver.rs | 71 +++++++++++++++++++-- src/query/set_handler.rs | 24 ++++++- src/translator/metadata.rs | 18 +++++- src/types/schema_type_mapper.rs | 23 +++++++ tests/sql/features/column_conformance.sql | 76 +++++++++++++++++++++++ 12 files changed, 266 insertions(+), 118 deletions(-) delete mode 100644 src/query/column_sanitizer.rs create mode 100644 tests/sql/features/column_conformance.sql diff --git a/src/cache/statement_pool.rs b/src/cache/statement_pool.rs index 35d9172..82c1552 100644 --- a/src/cache/statement_pool.rs +++ b/src/cache/statement_pool.rs @@ -3,7 +3,8 @@ use std::sync::Mutex; use rusqlite::{Connection, Statement, Params}; use once_cell::sync::Lazy; use crate::config::CONFIG; -use crate::query::column_sanitizer::sanitize_column_name; +use crate::query::projection_resolver::resolve_columns_with_legacy; +use crate::translator::TranslationMetadata; /// A pool of prepared SQLite statements for reuse /// This avoids the overhead of preparing the same statement multiple times @@ -190,16 +191,15 @@ impl StatementPool { /// Extract metadata from a prepared statement fn extract_metadata(&self, stmt: &Statement, query: &str) -> Result { - let column_count = stmt.column_count(); - let mut column_names = Vec::new(); + let column_names: Vec = resolve_columns_with_legacy( + stmt, + query, + &HashMap::new(), + &TranslationMetadata::new(), + false, + )?.into_iter().map(|m| m.wire_name).collect(); let mut column_types = Vec::new(); - - for i in 0..column_count { - column_names.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - // We can't easily get PostgreSQL types here, so we'll leave them as None - // They can be filled in later by the caller if needed - column_types.push(None); - } + column_types.resize(column_names.len(), None); let parameter_count = stmt.parameter_count(); let is_select = query.trim().to_uppercase().starts_with("SELECT"); diff --git a/src/catalog/query_interceptor.rs b/src/catalog/query_interceptor.rs index 9ee67af..b38d4dc 100644 --- a/src/catalog/query_interceptor.rs +++ b/src/catalog/query_interceptor.rs @@ -2,8 +2,8 @@ use crate::session::db_handler::{DbHandler, DbResponse}; use uuid::Uuid; use crate::session::SessionState; use crate::PgSqliteError; -use crate::translator::{RegexTranslator, SchemaPrefixTranslator}; -use crate::query::column_sanitizer::sanitize_column_name; +use crate::translator::{RegexTranslator, SchemaPrefixTranslator, TranslationMetadata}; +use crate::query::projection_resolver::resolve_columns_with_legacy; use sqlparser::ast::{Statement, TableFactor, Select, SetExpr, SelectItem, Expr, FunctionArg, FunctionArgExpr}; use sqlparser::dialect::PostgreSqlDialect; use sqlparser::parser::Parser; @@ -270,10 +270,13 @@ impl CatalogInterceptor { // Execute the translated query let mut stmt = conn.prepare(&query_str)?; let column_count = stmt.column_count(); - let mut columns = Vec::new(); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, + &query_str, + &HashMap::new(), + &TranslationMetadata::new(), + false, + )?.into_iter().map(|m| m.wire_name).collect(); let rows_result: rusqlite::Result>>>> = stmt.query_map([], |row| { let mut values = Vec::new(); @@ -411,10 +414,13 @@ impl CatalogInterceptor { // Execute the query directly with the session's connection let mut stmt = conn.prepare(&query_str)?; let column_count = stmt.column_count(); - let mut columns = Vec::new(); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, + &query_str, + &HashMap::new(), + &TranslationMetadata::new(), + false, + )?.into_iter().map(|m| m.wire_name).collect(); let rows_result: rusqlite::Result>>>> = stmt.query_map([], |row| { let mut values = Vec::new(); diff --git a/src/optimization/read_only_optimizer.rs b/src/optimization/read_only_optimizer.rs index de68923..4aa5aac 100644 --- a/src/optimization/read_only_optimizer.rs +++ b/src/optimization/read_only_optimizer.rs @@ -5,7 +5,8 @@ use rusqlite::Connection; use crate::cache::SchemaCache; use crate::session::db_handler::DbResponse; use crate::query::QueryComplexity; -use crate::query::column_sanitizer::sanitize_column_name; +use crate::query::projection_resolver::resolve_columns_with_legacy; +use crate::translator::TranslationMetadata; use tracing::{debug, info}; /// Direct read-only access optimizer for SELECT queries @@ -255,13 +256,14 @@ impl ReadOnlyOptimizer { // Prepare statement to get column information let stmt = conn.prepare(query)?; - let column_count = stmt.column_count(); - // Get column names - let mut columns = Vec::new(); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, + query, + &HashMap::new(), + &TranslationMetadata::new(), + false, + )?.into_iter().map(|m| m.wire_name).collect(); // Get column types from schema cache let mut column_types = Vec::new(); diff --git a/src/query/column_sanitizer.rs b/src/query/column_sanitizer.rs deleted file mode 100644 index 3f3253d..0000000 --- a/src/query/column_sanitizer.rs +++ /dev/null @@ -1,66 +0,0 @@ -pub fn sanitize_column_name(name: &str) -> &str { - match name.find('(') { - Some(pos) => &name[..pos], - None => name, - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_version_function() { - assert_eq!(sanitize_column_name("version()"), "version"); - } - - #[test] - fn test_count_star() { - assert_eq!(sanitize_column_name("count(*)"), "count"); - } - - #[test] - fn test_max_with_arg() { - assert_eq!(sanitize_column_name("max(id)"), "max"); - } - - #[test] - fn test_coalesce_nested() { - assert_eq!(sanitize_column_name("COALESCE(max(id), 0)"), "COALESCE"); - } - - #[test] - fn test_current_timestamp() { - assert_eq!(sanitize_column_name("current_timestamp"), "current_timestamp"); - } - - #[test] - fn test_plain_column() { - assert_eq!(sanitize_column_name("my_column"), "my_column"); - } - - #[test] - fn test_empty_string() { - assert_eq!(sanitize_column_name(""), ""); - } - - #[test] - fn test_just_parentheses() { - assert_eq!(sanitize_column_name("()"), ""); - } - - #[test] - fn test_open_paren_only() { - assert_eq!(sanitize_column_name("("), ""); - } - - #[test] - fn test_multiple_parens() { - assert_eq!(sanitize_column_name("func(arg1, arg2)"), "func"); - } - - #[test] - fn test_nested_parens() { - assert_eq!(sanitize_column_name("outer(inner())"), "outer"); - } -} \ No newline at end of file diff --git a/src/query/extended.rs b/src/query/extended.rs index ec30b80..0ca2db0 100644 --- a/src/query/extended.rs +++ b/src/query/extended.rs @@ -676,8 +676,9 @@ impl ExtendedQueryHandler { Self::cast_type_to_oid(cast_type) } // For parameter columns (NULL from SELECT $1), try to match with parameters - else if col_name == "NULL" || col_name == "?column?" { - // For queries like SELECT $1, $2, the columns correspond to parameters + else if col_name == "NULL" || (col_name == "?column?" && is_simple_param_select) { + // For queries like SELECT $1, $2, the columns correspond to parameters. + // Other ?column? expressions must continue to value/query inference below. if is_simple_param_select { // Count which parameter this column represents // For SELECT $1, $2, column 0 = param 0, column 1 = param 1 @@ -716,7 +717,7 @@ impl ExtendedQueryHandler { PgType::Text.to_oid() } } else { - // For other queries with NULL columns, default to TEXT + // For NULL literal columns, default to TEXT. PgType::Text.to_oid() } } diff --git a/src/query/fast_path.rs b/src/query/fast_path.rs index dacebd3..b6a2177 100644 --- a/src/query/fast_path.rs +++ b/src/query/fast_path.rs @@ -3,7 +3,8 @@ use regex::Regex; use once_cell::sync::Lazy; use crate::cache::SchemaCache; use crate::session::db_handler::DbResponse; -use crate::query::column_sanitizer::sanitize_column_name; +use crate::query::projection_resolver::resolve_columns_with_legacy; +use crate::translator::TranslationMetadata; use std::collections::HashMap; use std::sync::Mutex; @@ -373,10 +374,13 @@ pub fn query_fast_path( let column_count = stmt.column_count(); // Get column names - let mut columns = Vec::new(); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, + query, + &HashMap::new(), + &TranslationMetadata::new(), + false, + )?.into_iter().map(|m| m.wire_name).collect(); // Check for boolean columns in the schema using cache let mut column_types = Vec::new(); @@ -615,10 +619,13 @@ fn execute_fast_select_with_params( let column_count = stmt.column_count(); // Get column names - let mut columns = Vec::new(); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, + query, + &HashMap::new(), + &TranslationMetadata::new(), + false, + )?.into_iter().map(|m| m.wire_name).collect(); // Check for boolean columns in the schema using cache let mut column_types = Vec::new(); @@ -733,10 +740,13 @@ fn execute_fast_select( let column_count = stmt.column_count(); // Get column names - let mut columns = Vec::new(); - for i in 0..column_count { - columns.push(sanitize_column_name(stmt.column_name(i)?).to_string()); - } + let columns: Vec = resolve_columns_with_legacy( + &stmt, + query, + &HashMap::new(), + &TranslationMetadata::new(), + false, + )?.into_iter().map(|m| m.wire_name).collect(); // Check for boolean columns in the schema using cache let mut column_types = Vec::new(); diff --git a/src/query/mod.rs b/src/query/mod.rs index c48b804..68ab882 100644 --- a/src/query/mod.rs +++ b/src/query/mod.rs @@ -15,7 +15,6 @@ pub mod unified_processor; pub mod pattern_optimizer; pub mod query_handler; pub mod join_type_inference; -pub mod column_sanitizer; pub mod projection_resolver; pub use executor::QueryExecutor; diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs index cacbcd5..0632566 100644 --- a/src/query/projection_resolver.rs +++ b/src/query/projection_resolver.rs @@ -18,10 +18,7 @@ pub struct AliasItem { /// (so the zero-alloc fast path never pays a parse). On parse error, None. pub fn parse_alias_view(query: &str) -> Option> { if !query.split_whitespace().any(|w| w.eq_ignore_ascii_case("AS")) { return None; } - let parsed = Parser::parse_sql(&PostgreSqlDialect {}, query).ok()?; - let stmt = parsed.into_iter().next()?; - let body = match stmt { Statement::Query(q) => q, _ => return None }; - let select = match &*body.body { SetExpr::Select(s) => s, _ => return None }; + let select = parse_select_projection(query)?; let items: Vec = select.projection.iter().enumerate() .filter_map(|(i, item)| match item { SelectItem::ExprWithAlias { expr, alias } => Some(AliasItem { @@ -36,6 +33,33 @@ pub fn parse_alias_view(query: &str) -> Option> { Some(items) } +fn parse_select_projection(query: &str) -> Option> { + let parsed = Parser::parse_sql(&PostgreSqlDialect {}, query).ok()?; + let stmt = parsed.into_iter().next()?; + let body = match stmt { Statement::Query(q) => q, _ => return None }; + match *body.body { SetExpr::Select(s) => Some(s), _ => None } +} + +fn parse_quoted_projection_names(query: &str) -> Option>> { + let select = parse_select_projection(query)?; + Some(select.projection.iter().map(|item| match item { + SelectItem::UnnamedExpr(Expr::Identifier(ident)) if ident.quote_style.is_some() => { + Some(ident.value.clone()) + } + SelectItem::UnnamedExpr(Expr::CompoundIdentifier(idents)) => idents.last() + .filter(|ident| ident.quote_style.is_some()) + .map(|ident| ident.value.clone()), + _ => None, + }).collect()) +} + +fn quoted_projection_name<'a>(quoted_projection_names: Option<&'a [Option]>, position: usize, raw_name: &str) -> Option<&'a str> { + quoted_projection_names? + .get(position)? + .as_deref() + .filter(|quoted| *quoted == raw_name) +} + #[derive(Debug, Clone, PartialEq)] pub struct ColumnMeta { pub wire_name: String, @@ -205,10 +229,23 @@ pub fn resolve_columns_with_legacy( ) -> rusqlite::Result> { let alias_view = parse_alias_view(query); let view_ref = alias_view.as_deref(); + let quoted_projection_names = parse_quoted_projection_names(query); + let quoted_ref = quoted_projection_names.as_deref(); let count = stmt.column_count(); let mut out = Vec::with_capacity(count); for i in 0..count { let raw = stmt.column_name(i)?.to_string(); + if let Some(quoted_name) = quoted_projection_name(quoted_ref, i, &raw) { + let pg_type = schema_types.get(quoted_name); + out.push(ColumnMeta { + wire_name: quoted_name.to_string(), + type_oid: pg_type + .map(|pg_type| SchemaTypeMapper::pg_type_string_to_oid(pg_type)) + .unwrap_or(PgType::Text.to_oid()), + datetime_flag: pg_type.is_some_and(|pg_type| is_datetime_type(pg_type)), + }); + continue; + } let ctx = ResolveCtx { schema_types, hints, alias_view: view_ref, legacy }; out.push(ProjectionResolver::resolve(&raw, i, &ctx)); } @@ -229,8 +266,21 @@ pub async fn resolve_columns_from_names( let legacy = session.legacy_result_columns().await; let alias_view = parse_alias_view(query); let view_ref = alias_view.as_deref(); + let quoted_projection_names = parse_quoted_projection_names(query); + let quoted_ref = quoted_projection_names.as_deref(); let mut out = Vec::with_capacity(names.len()); for (i, raw) in names.iter().enumerate() { + if let Some(quoted_name) = quoted_projection_name(quoted_ref, i, raw) { + let pg_type = schema_types.get(quoted_name); + out.push(ColumnMeta { + wire_name: quoted_name.to_string(), + type_oid: pg_type + .map(|pg_type| SchemaTypeMapper::pg_type_string_to_oid(pg_type)) + .unwrap_or(PgType::Text.to_oid()), + datetime_flag: pg_type.is_some_and(|pg_type| is_datetime_type(pg_type)), + }); + continue; + } let ctx = ResolveCtx { schema_types, hints, alias_view: view_ref, legacy }; let meta = ProjectionResolver::resolve(raw, i, &ctx); out.push(meta); @@ -435,6 +485,19 @@ mod tests { assert_eq!(m.wire_name, "?column?"); } + #[test] + fn resolve_columns_preserves_quoted_paren_identifier_without_schema_map() { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute(r#"CREATE TABLE t1 ("price(usd)" INT)"#, []).unwrap(); + let stmt = conn.prepare(r#"SELECT "price(usd)" FROM t1"#).unwrap(); + let schema = HashMap::new(); + let hints = TranslationMetadata::new(); + let metas = resolve_columns_with_legacy( + &stmt, r#"SELECT "price(usd)" FROM t1"#, &schema, &hints, false, + ).unwrap(); + assert_eq!(metas[0].wire_name, "price(usd)"); + } + #[tokio::test] async fn resolve_columns_from_names_dedup_numbering() { // Unit test for the shared ?column?N dedup: two unnamed expressions diff --git a/src/query/set_handler.rs b/src/query/set_handler.rs index 597d0c2..8f14710 100644 --- a/src/query/set_handler.rs +++ b/src/query/set_handler.rs @@ -13,7 +13,7 @@ static SET_TIMEZONE_PATTERN: Lazy = Lazy::new(|| { }); static SET_PARAMETER_PATTERN: Lazy = Lazy::new(|| { - Regex::new(r"(?i)^\s*SET\s+(\w+)(?:\s*=\s*|\s+TO\s+)(.+)$").unwrap() + Regex::new(r"(?i)^\s*SET\s+([A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)*)(?:\s*=\s*|\s+TO\s+)(.+)$").unwrap() }); static SHOW_PARAMETER_PATTERN: Lazy = Lazy::new(|| { @@ -82,7 +82,7 @@ impl SetHandler { // Handle general SET parameter if let Some(caps) = SET_PARAMETER_PATTERN.captures(trimmed) { - let param_name = caps[1].to_uppercase(); + let param_name = Self::normalize_parameter_name(&caps[1]); let param_value = caps[2].trim().trim_matches('\'').trim_matches('"'); // Update session parameter @@ -99,7 +99,7 @@ impl SetHandler { // Handle SHOW parameter if let Some(caps) = SHOW_PARAMETER_PATTERN.captures(trimmed) { - let param_name = caps[1].to_uppercase(); + let param_name = Self::normalize_parameter_name(&caps[1]); info!("SHOW parameter: {}", param_name); // Handle special PostgreSQL SHOW commands @@ -155,6 +155,14 @@ impl SetHandler { Err(PgSqliteError::Protocol(format!("Unrecognized SET command: {query}"))) } + fn normalize_parameter_name(name: &str) -> String { + if name.contains('.') { + name.to_ascii_lowercase() + } else { + name.to_ascii_uppercase() + } + } + /// Set the session timezone async fn set_timezone(session: &Arc, timezone: &str) -> Result<(), PgSqliteError> { // Validate timezone (basic validation) @@ -246,6 +254,16 @@ mod tests { assert!(SET_PARAMETER_PATTERN.is_match("SET client_encoding TO 'UTF8'")); } + #[test] + fn test_set_parameter_pattern_dotted_pgsqlite_guc() { + let caps = SET_PARAMETER_PATTERN + .captures("SET pgsqlite.legacy_result_columns = on") + .unwrap(); + assert_eq!(&caps[1], "pgsqlite.legacy_result_columns"); + assert_eq!(&caps[2], "on"); + assert_eq!(SetHandler::normalize_parameter_name("pgsqlite.legacy_result_columns"), "pgsqlite.legacy_result_columns"); + } + #[test] fn test_set_parameter_pattern_captures() { let caps = SET_PARAMETER_PATTERN.captures("SET DateStyle=ISO").unwrap(); diff --git a/src/translator/metadata.rs b/src/translator/metadata.rs index 22cbc6e..78460bc 100644 --- a/src/translator/metadata.rs +++ b/src/translator/metadata.rs @@ -70,7 +70,12 @@ impl TranslationMetadata { /// Get type hint for a column (if any) pub fn get_hint(&self, column_name: &str) -> Option<&ColumnTypeHint> { - self.column_mappings.get(column_name) + self.column_mappings.get(column_name).or_else(|| { + self.column_mappings + .iter() + .find(|(key, _)| key.eq_ignore_ascii_case(column_name)) + .map(|(_, hint)| hint) + }) } /// Merge another metadata instance into this one @@ -168,6 +173,17 @@ mod tests { assert_eq!(hint.suggested_type, Some(PgType::Timestamp)); } + #[test] + fn test_get_hint_matches_conformant_lowercase_alias() { + let mut metadata = TranslationMetadata::new(); + metadata.add_hint( + "TotalPrice".to_string(), + ColumnTypeHint::arithmetic_on_float("price".to_string()), + ); + + assert!(metadata.get_hint("totalprice").is_some()); + } + #[test] fn test_metadata_merge() { let mut metadata1 = TranslationMetadata::new(); diff --git a/src/types/schema_type_mapper.rs b/src/types/schema_type_mapper.rs index cc7f314..0e66dde 100644 --- a/src/types/schema_type_mapper.rs +++ b/src/types/schema_type_mapper.rs @@ -363,6 +363,16 @@ impl SchemaTypeMapper { } } + // If the conformant column name is the bare function name for an + // unaliased call (e.g. json_extract(...)), recover the function + // return type from the query text. + let direct_call_pattern = format!(r"(?i)\b{}\s*\(", regex::escape(function_name)); + if let Ok(re) = regex::Regex::new(&direct_call_pattern) + && re.is_match(q) { + return Self::get_aggregate_return_type_with_query( + &format!("{function_name}("), conn, table_name, None); + } + // Also check for array concatenation operator pattern: column || array AS alias // NOTE: For now, we return TEXT instead of TextArray because: // 1. The data is stored as JSON strings in SQLite @@ -543,6 +553,19 @@ mod tests { } } + #[test] + fn test_bare_json_extract_with_query_context_resolves_text() { + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "json_extract", + None, + None, + Some("SELECT json_extract(data, '$[0]') FROM array_ops"), + ), + Some(PgType::Text.to_oid()), + ); + } + // The query-regex fallback (kept) still resolves aliased aggregates with query context. #[test] fn test_bare_aggregate_with_query_context_still_resolves() { diff --git a/tests/sql/features/column_conformance.sql b/tests/sql/features/column_conformance.sql new file mode 100644 index 0000000..44b4d27 --- /dev/null +++ b/tests/sql/features/column_conformance.sql @@ -0,0 +1,76 @@ +-- Column result conformance integration tests +-- The runner executes this through psql; ON_ERROR_STOP makes assertion failures abort. +\echo '=== Column conformance: F1 quoted paren column ===' +DROP TABLE IF EXISTS t1; +CREATE TABLE t1 ("price(usd)" INT); +INSERT INTO t1 VALUES (5); +-- Header should be exactly price(usd), not sanitized to price. +SELECT "price(usd)" FROM t1; +SELECT "price(usd)" AS f1_value FROM t1 \gset +SELECT (:'f1_value' = '5') AS f1_ok \gset +\if :f1_ok +\else +SELECT pgsqlite_column_conformance_f1_value_failed; +\endif + +\echo '=== Column conformance: F2 max(timestamp) formats as timestamp ===' +DROP TABLE IF EXISTS t2; +CREATE TABLE t2 (created_at TIMESTAMP); +INSERT INTO t2 VALUES ('2023-11-14 22:13:20'); +-- Header should be max and value should be 2023-11-14 22:13:20, not raw microseconds. +SELECT max(created_at) FROM t2; +\unset max +SELECT max(created_at) FROM t2 \gset +SELECT (:'max' = '2023-11-14 22:13:20') AS f2_ok \gset +\if :f2_ok +\else +SELECT pgsqlite_column_conformance_f2_format_failed; +\endif + +\echo '=== Column conformance: F5 alias count remains text ===' +DROP TABLE IF EXISTS t3; +CREATE TABLE t3 (name TEXT); +INSERT INTO t3 VALUES ('alice'); +-- Header should be count and value should decode as text ALICE (extended/binary covered by unit tests). +SELECT upper(name) AS count FROM t3; +SELECT upper(name) AS f5_value FROM t3 \gset +SELECT (:'f5_value' = 'ALICE') AS f5_ok \gset +\if :f5_ok +\else +SELECT pgsqlite_column_conformance_f5_text_failed; +\endif + +\echo '=== Column conformance: F4 unnamed expression ===' +-- Header should be ?column? and value should be 2. +SELECT 1+1; +SELECT 1+1 AS f4_value \gset +SELECT (:'f4_value' = '2') AS f4_ok \gset +\if :f4_ok +\else +SELECT pgsqlite_column_conformance_f4_value_failed; +\endif + +\echo '=== Column conformance: F-new catalog count and F7 wildcard ===' +-- Header should be count; value should be at least public + pg_catalog namespaces. +SELECT count(*) FROM pg_catalog.pg_namespace; +\unset count +SELECT count(*) FROM pg_catalog.pg_namespace \gset +SELECT (:'count' = '2') AS fnew_ok \gset +\if :fnew_ok +\else +SELECT pgsqlite_column_conformance_catalog_count_failed; +\endif +-- Expected columns include nspname, oid, oid (mid-list wildcard must not drop/rename columns incorrectly). +SELECT *, oid FROM pg_catalog.pg_namespace; + +\echo '=== Column conformance: C2 legacy_result_columns alias casing ===' +DROP TABLE IF EXISTS t4; +CREATE TABLE t4 (id INT); +INSERT INTO t4 VALUES (7); +SET pgsqlite.legacy_result_columns = on; +-- Smoke: SET accepts the dotted GUC. This psql/simple sync path still resolves columns +-- with conformant defaults, so resolver/unit tests cover legacy header behavior. +SELECT id AS MyAlias FROM t4; +SET pgsqlite.legacy_result_columns = off; +-- Legacy off/conformant default: unquoted alias casing is lower-case myalias. +SELECT id AS MyAlias FROM t4; From 2369cf3979e55fcedf8fa6b689e30136ba9a4594 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 20:42:44 -0700 Subject: [PATCH 17/21] fix: harden Task 10 conformance edge cases --- src/query/executor.rs | 18 ++++++- src/query/projection_resolver.rs | 59 +++++++++++++++++------ src/query/set_handler.rs | 27 ++++++++++- src/types/schema_type_mapper.rs | 24 ++++++++- tests/sql/features/column_conformance.sql | 19 ++++++-- 5 files changed, 125 insertions(+), 22 deletions(-) diff --git a/src/query/executor.rs b/src/query/executor.rs index 121188e..79e9740 100644 --- a/src/query/executor.rs +++ b/src/query/executor.rs @@ -326,12 +326,28 @@ impl QueryExecutor { match QueryTypeDetector::detect_query_type(query) { QueryType::Select => { // Route query through query router if available and appropriate - let response = if let Some(router) = query_router { + let mut response = if let Some(router) = query_router { router.execute_query(query, session).await.map_err(|e| PgSqliteError::Protocol(e.to_string()))? } else { let cached_conn = Self::get_or_cache_connection(session, db).await; db.query_with_session_cached(query, &session.id, cached_conn.as_ref()).await? }; + if session.legacy_result_columns().await { + let schema_types = std::collections::HashMap::new(); + let hints = crate::translator::TranslationMetadata::new(); + response.columns = crate::query::projection_resolver::resolve_columns_from_names( + &response.columns, + query, + &schema_types, + &hints, + session, + ) + .await + .map_err(|e| PgSqliteError::Protocol(e.to_string()))? + .into_iter() + .map(|m| m.wire_name) + .collect(); + } // Always check for type conversion to handle datetime columns let needs_type_conversion = true; diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs index 0632566..fc5b107 100644 --- a/src/query/projection_resolver.rs +++ b/src/query/projection_resolver.rs @@ -121,6 +121,7 @@ pub fn is_arg_preserving(fn_name_lower: &str) -> bool { } pub struct ResolveCtx<'a> { + pub query: &'a str, pub schema_types: &'a HashMap, pub hints: &'a TranslationMetadata, pub alias_view: Option<&'a [AliasItem]>, @@ -140,13 +141,23 @@ impl ProjectionResolver { }; } - // Step 2: function-call shape (raw name contains `(`). + // Step 2: function-call shape (raw name contains `(`), only when the + // original query actually contains that function call. SQLite also + // returns raw real-column names here for quoted identifiers like + // "price(usd)" in SELECT * projections. if let Some(shape) = fn_shape(raw_name) { let name = shape.name.trim(); - let lower = name.to_lowercase(); - let wire_name = lower.clone(); - let (type_oid, datetime_flag) = resolve_function_type(&lower, &shape.inner, ctx); - return ColumnMeta { wire_name, type_oid, datetime_flag }; + if query_contains_function_call(ctx.query, name) { + let lower = name.to_lowercase(); + let wire_name = lower.clone(); + let (type_oid, datetime_flag) = resolve_function_type(&lower, &shape.inner, ctx); + return ColumnMeta { wire_name, type_oid, datetime_flag }; + } + return ColumnMeta { + wire_name: raw_name.to_string(), + type_oid: PgType::Text.to_oid(), + datetime_flag: false, + }; } // Step 3: alias (position maps to an ExprWithAlias item). @@ -190,6 +201,14 @@ fn looks_unnamed_expr(name: &str) -> bool { name.chars().any(|c| matches!(c, '+'|'-'|'*'|'/'|' '|'=')) || name.parse::().is_ok() } +fn query_contains_function_call(query: &str, fn_name: &str) -> bool { + if query.is_empty() || fn_name.is_empty() { + return false; + } + let pattern = format!(r"(?i)(?:^|[^\w]){}\s*\(", regex::escape(fn_name)); + regex::Regex::new(&pattern).is_ok_and(|re| re.is_match(query)) +} + fn resolve_function_type(lower_fn: &str, inner: &str, ctx: &ResolveCtx) -> (i32, bool) { if is_arg_preserving(lower_fn) { if let Some(pg_type) = ctx.schema_types.get(inner.trim()) { @@ -246,7 +265,7 @@ pub fn resolve_columns_with_legacy( }); continue; } - let ctx = ResolveCtx { schema_types, hints, alias_view: view_ref, legacy }; + let ctx = ResolveCtx { query, schema_types, hints, alias_view: view_ref, legacy }; out.push(ProjectionResolver::resolve(&raw, i, &ctx)); } dedup_question_columns(&mut out); @@ -281,7 +300,7 @@ pub async fn resolve_columns_from_names( }); continue; } - let ctx = ResolveCtx { schema_types, hints, alias_view: view_ref, legacy }; + let ctx = ResolveCtx { query, schema_types, hints, alias_view: view_ref, legacy }; let meta = ProjectionResolver::resolve(raw, i, &ctx); out.push(meta); } @@ -378,13 +397,17 @@ mod tests { use crate::translator::TranslationMetadata; fn ctx_no_aliases<'a>(schema: &'a HashMap, legacy: bool) -> ResolveCtx<'a> { + ctx_no_aliases_for_query(schema, legacy, "") + } + + fn ctx_no_aliases_for_query<'a>(schema: &'a HashMap, legacy: bool, query: &'a str) -> ResolveCtx<'a> { static EMPTY: once_cell::sync::Lazy = once_cell::sync::Lazy::new(TranslationMetadata::new); - ResolveCtx { schema_types: schema, hints: &EMPTY, alias_view: None, legacy } + ResolveCtx { query, schema_types: schema, hints: &EMPTY, alias_view: None, legacy } } fn ctx_with_aliases<'a>(schema: &'a HashMap, alias_view: &'a [AliasItem], legacy: bool) -> ResolveCtx<'a> { static EMPTY: once_cell::sync::Lazy = once_cell::sync::Lazy::new(TranslationMetadata::new); - ResolveCtx { schema_types: schema, hints: &EMPTY, alias_view: Some(alias_view), legacy } + ResolveCtx { query: "", schema_types: schema, hints: &EMPTY, alias_view: Some(alias_view), legacy } } #[test] @@ -395,10 +418,18 @@ mod tests { assert_eq!(m.wire_name, "price(usd)"); assert_eq!(m.type_oid, PgType::Int4.to_oid()); } + + #[test] + fn star_projection_preserves_paren_column_without_schema_map() { + let schema = HashMap::new(); + let m = ProjectionResolver::resolve("price(usd)", 0, &ctx_no_aliases_for_query(&schema, false, "SELECT * FROM t")); + assert_eq!(m.wire_name, "price(usd)"); + } + #[test] fn function_shape_lowers_and_types() { let schema = HashMap::new(); - let m = ProjectionResolver::resolve("count(*)", 0, &ctx_no_aliases(&schema, false)); + let m = ProjectionResolver::resolve("count(*)", 0, &ctx_no_aliases_for_query(&schema, false, "SELECT count(*) FROM t")); assert_eq!(m.wire_name, "count"); assert_eq!(m.type_oid, PgType::Int8.to_oid()); assert!(!m.datetime_flag); @@ -407,7 +438,7 @@ mod tests { fn min_over_timestamp_is_datetime() { let mut schema = HashMap::new(); schema.insert("created_at".to_string(), "TIMESTAMP".to_string()); - let m = ProjectionResolver::resolve("max(created_at)", 0, &ctx_no_aliases(&schema, false)); + let m = ProjectionResolver::resolve("max(created_at)", 0, &ctx_no_aliases_for_query(&schema, false, "SELECT max(created_at) FROM t")); assert_eq!(m.wire_name, "max"); assert_eq!(m.type_oid, PgType::Timestamp.to_oid()); assert!(m.datetime_flag); @@ -416,7 +447,7 @@ mod tests { fn min_over_int4_no_datetime() { let mut schema = HashMap::new(); schema.insert("qty".to_string(), "INT4".to_string()); - let m = ProjectionResolver::resolve("max(qty)", 0, &ctx_no_aliases(&schema, false)); + let m = ProjectionResolver::resolve("max(qty)", 0, &ctx_no_aliases_for_query(&schema, false, "SELECT max(qty) FROM t")); assert_eq!(m.type_oid, PgType::Int4.to_oid()); assert!(!m.datetime_flag); } @@ -432,7 +463,7 @@ mod tests { // Guard: legacy affects only steps 3 & 5. A COUNT(*) projection must // always emit conformant wire-name `count` even when legacy=true. let schema = HashMap::new(); - let m = ProjectionResolver::resolve("COUNT(*)", 0, &ctx_no_aliases(&schema, true)); + let m = ProjectionResolver::resolve("COUNT(*)", 0, &ctx_no_aliases_for_query(&schema, true, "SELECT COUNT(*) FROM t")); assert_eq!(m.wire_name, "count", "legacy must not leak into step 2 casing"); assert_eq!(m.type_oid, PgType::Int8.to_oid()); } @@ -469,7 +500,7 @@ mod tests { fn array_agg_returns_text_oid() { // MUST-FIX #3: array_agg returns Text (JSON array storage). let schema = HashMap::new(); - let m = ProjectionResolver::resolve("array_agg(x)", 0, &ctx_no_aliases(&schema, false)); + let m = ProjectionResolver::resolve("array_agg(x)", 0, &ctx_no_aliases_for_query(&schema, false, "SELECT array_agg(x) FROM t")); assert_eq!(m.wire_name, "array_agg"); assert_eq!(m.type_oid, PgType::Text.to_oid()); } diff --git a/src/query/set_handler.rs b/src/query/set_handler.rs index 8f14710..96940a0 100644 --- a/src/query/set_handler.rs +++ b/src/query/set_handler.rs @@ -83,7 +83,7 @@ impl SetHandler { // Handle general SET parameter if let Some(caps) = SET_PARAMETER_PATTERN.captures(trimmed) { let param_name = Self::normalize_parameter_name(&caps[1]); - let param_value = caps[2].trim().trim_matches('\'').trim_matches('"'); + let param_value = Self::normalize_parameter_value(&caps[2]); // Update session parameter let mut params = session.parameters.write().await; @@ -156,6 +156,7 @@ impl SetHandler { } fn normalize_parameter_name(name: &str) -> String { + let name = name.trim().trim_end_matches(';').trim(); if name.contains('.') { name.to_ascii_lowercase() } else { @@ -163,6 +164,16 @@ impl SetHandler { } } + fn normalize_parameter_value(value: &str) -> String { + value + .trim() + .trim_end_matches(';') + .trim() + .trim_matches('\'') + .trim_matches('"') + .to_string() + } + /// Set the session timezone async fn set_timezone(session: &Arc, timezone: &str) -> Result<(), PgSqliteError> { // Validate timezone (basic validation) @@ -264,6 +275,20 @@ mod tests { assert_eq!(SetHandler::normalize_parameter_name("pgsqlite.legacy_result_columns"), "pgsqlite.legacy_result_columns"); } + #[test] + fn test_set_and_show_parameter_patterns_ignore_semicolon() { + let set_caps = SET_PARAMETER_PATTERN + .captures("SET pgsqlite.legacy_result_columns = on;") + .unwrap(); + assert_eq!(SetHandler::normalize_parameter_name(&set_caps[1]), "pgsqlite.legacy_result_columns"); + assert_eq!(SetHandler::normalize_parameter_value(&set_caps[2]), "on"); + + let show_caps = SHOW_PARAMETER_PATTERN + .captures("SHOW pgsqlite.legacy_result_columns;") + .unwrap(); + assert_eq!(SetHandler::normalize_parameter_name(&show_caps[1]), "pgsqlite.legacy_result_columns"); + } + #[test] fn test_set_parameter_pattern_captures() { let caps = SET_PARAMETER_PATTERN.captures("SET DateStyle=ISO").unwrap(); diff --git a/src/types/schema_type_mapper.rs b/src/types/schema_type_mapper.rs index 0e66dde..04d5ab5 100644 --- a/src/types/schema_type_mapper.rs +++ b/src/types/schema_type_mapper.rs @@ -366,7 +366,7 @@ impl SchemaTypeMapper { // If the conformant column name is the bare function name for an // unaliased call (e.g. json_extract(...)), recover the function // return type from the query text. - let direct_call_pattern = format!(r"(?i)\b{}\s*\(", regex::escape(function_name)); + let direct_call_pattern = format!(r"(?i)(?:^|\bSELECT\s+|,)\s*{}\s*\(", regex::escape(function_name)); if let Ok(re) = regex::Regex::new(&direct_call_pattern) && re.is_match(q) { return Self::get_aggregate_return_type_with_query( @@ -564,6 +564,28 @@ mod tests { ), Some(PgType::Text.to_oid()), ); + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "json_extract", + None, + None, + Some("SELECT id, json_extract(data, '$[0]') FROM array_ops"), + ), + Some(PgType::Text.to_oid()), + ); + } + + #[test] + fn test_bare_json_extract_alias_with_where_call_does_not_resolve_text() { + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "json_extract", + None, + None, + Some("SELECT upper(name) AS json_extract FROM t WHERE json_extract(data, '$.x') IS NOT NULL"), + ), + None, + ); } // The query-regex fallback (kept) still resolves aliased aggregates with query context. diff --git a/tests/sql/features/column_conformance.sql b/tests/sql/features/column_conformance.sql index 44b4d27..a4f6e10 100644 --- a/tests/sql/features/column_conformance.sql +++ b/tests/sql/features/column_conformance.sql @@ -68,9 +68,18 @@ DROP TABLE IF EXISTS t4; CREATE TABLE t4 (id INT); INSERT INTO t4 VALUES (7); SET pgsqlite.legacy_result_columns = on; --- Smoke: SET accepts the dotted GUC. This psql/simple sync path still resolves columns --- with conformant defaults, so resolver/unit tests cover legacy header behavior. -SELECT id AS MyAlias FROM t4; +\unset legacy_MyAlias +\unset legacy_myalias +SELECT id AS MyAlias FROM t4 \gset legacy_ +\if :{?legacy_MyAlias} +\else +SELECT pgsqlite_column_conformance_c2_legacy_alias_failed; +\endif SET pgsqlite.legacy_result_columns = off; --- Legacy off/conformant default: unquoted alias casing is lower-case myalias. -SELECT id AS MyAlias FROM t4; +\unset conform_MyAlias +\unset conform_myalias +SELECT id AS MyAlias FROM t4 \gset conform_ +\if :{?conform_myalias} +\else +SELECT pgsqlite_column_conformance_c2_conform_alias_failed; +\endif From 205819f455a09820004f9d51bb7acd281451bbce Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 20:59:55 -0700 Subject: [PATCH 18/21] fix: use projection AST for function-shaped result columns --- src/query/projection_resolver.rs | 101 +++++++++++++++++++++++++++---- src/types/schema_type_mapper.rs | 56 ++++++++++++++--- 2 files changed, 137 insertions(+), 20 deletions(-) diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs index fc5b107..67d1dd3 100644 --- a/src/query/projection_resolver.rs +++ b/src/query/projection_resolver.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use anyhow::Result; use crate::types::{PgType, SchemaTypeMapper}; use crate::translator::TranslationMetadata; -use sqlparser::ast::{Expr, SelectItem, SetExpr, Statement}; +use sqlparser::ast::{Expr, ObjectName, ObjectNamePart, SelectItem, SetExpr, Statement}; use sqlparser::dialect::PostgreSqlDialect; use sqlparser::parser::Parser; @@ -53,6 +53,20 @@ fn parse_quoted_projection_names(query: &str) -> Option>> { }).collect()) } +fn parse_function_projection_names(query: &str) -> Option>> { + let select = parse_select_projection(query)?; + Some(select.projection.iter().map(|item| match item { + SelectItem::UnnamedExpr(Expr::Function(function)) => object_name_last_part_lower(&function.name), + _ => None, + }).collect()) +} + +fn object_name_last_part_lower(name: &ObjectName) -> Option { + name.0.last().map(|part| match part { + ObjectNamePart::Identifier(ident) => ident.value.to_lowercase(), + }) +} + fn quoted_projection_name<'a>(quoted_projection_names: Option<&'a [Option]>, position: usize, raw_name: &str) -> Option<&'a str> { quoted_projection_names? .get(position)? @@ -125,6 +139,7 @@ pub struct ResolveCtx<'a> { pub schema_types: &'a HashMap, pub hints: &'a TranslationMetadata, pub alias_view: Option<&'a [AliasItem]>, + pub function_projection_names: Option<&'a [Option]>, pub legacy: bool, } @@ -142,12 +157,12 @@ impl ProjectionResolver { } // Step 2: function-call shape (raw name contains `(`), only when the - // original query actually contains that function call. SQLite also - // returns raw real-column names here for quoted identifiers like - // "price(usd)" in SELECT * projections. + // same SELECT projection position is actually that function call. + // SQLite also returns raw real-column names here for quoted identifiers + // like "price(usd)" in SELECT * projections. if let Some(shape) = fn_shape(raw_name) { let name = shape.name.trim(); - if query_contains_function_call(ctx.query, name) { + if projection_function_matches(ctx, position, name) { let lower = name.to_lowercase(); let wire_name = lower.clone(); let (type_oid, datetime_flag) = resolve_function_type(&lower, &shape.inner, ctx); @@ -201,12 +216,21 @@ fn looks_unnamed_expr(name: &str) -> bool { name.chars().any(|c| matches!(c, '+'|'-'|'*'|'/'|' '|'=')) || name.parse::().is_ok() } -fn query_contains_function_call(query: &str, fn_name: &str) -> bool { - if query.is_empty() || fn_name.is_empty() { +fn projection_function_matches(ctx: &ResolveCtx, position: usize, fn_name: &str) -> bool { + if fn_name.is_empty() { return false; } - let pattern = format!(r"(?i)(?:^|[^\w]){}\s*\(", regex::escape(fn_name)); - regex::Regex::new(&pattern).is_ok_and(|re| re.is_match(query)) + + if let Some(names) = ctx.function_projection_names { + return names + .get(position) + .and_then(|name| name.as_deref()) + .is_some_and(|projected| projected.eq_ignore_ascii_case(fn_name)); + } + + parse_function_projection_names(ctx.query) + .and_then(|names| names.get(position).cloned().flatten()) + .is_some_and(|projected| projected.eq_ignore_ascii_case(fn_name)) } fn resolve_function_type(lower_fn: &str, inner: &str, ctx: &ResolveCtx) -> (i32, bool) { @@ -250,6 +274,8 @@ pub fn resolve_columns_with_legacy( let view_ref = alias_view.as_deref(); let quoted_projection_names = parse_quoted_projection_names(query); let quoted_ref = quoted_projection_names.as_deref(); + let function_projection_names = parse_function_projection_names(query); + let function_ref = function_projection_names.as_deref(); let count = stmt.column_count(); let mut out = Vec::with_capacity(count); for i in 0..count { @@ -265,7 +291,7 @@ pub fn resolve_columns_with_legacy( }); continue; } - let ctx = ResolveCtx { query, schema_types, hints, alias_view: view_ref, legacy }; + let ctx = ResolveCtx { query, schema_types, hints, alias_view: view_ref, function_projection_names: function_ref, legacy }; out.push(ProjectionResolver::resolve(&raw, i, &ctx)); } dedup_question_columns(&mut out); @@ -287,6 +313,8 @@ pub async fn resolve_columns_from_names( let view_ref = alias_view.as_deref(); let quoted_projection_names = parse_quoted_projection_names(query); let quoted_ref = quoted_projection_names.as_deref(); + let function_projection_names = parse_function_projection_names(query); + let function_ref = function_projection_names.as_deref(); let mut out = Vec::with_capacity(names.len()); for (i, raw) in names.iter().enumerate() { if let Some(quoted_name) = quoted_projection_name(quoted_ref, i, raw) { @@ -300,7 +328,7 @@ pub async fn resolve_columns_from_names( }); continue; } - let ctx = ResolveCtx { query, schema_types, hints, alias_view: view_ref, legacy }; + let ctx = ResolveCtx { query, schema_types, hints, alias_view: view_ref, function_projection_names: function_ref, legacy }; let meta = ProjectionResolver::resolve(raw, i, &ctx); out.push(meta); } @@ -402,12 +430,12 @@ mod tests { fn ctx_no_aliases_for_query<'a>(schema: &'a HashMap, legacy: bool, query: &'a str) -> ResolveCtx<'a> { static EMPTY: once_cell::sync::Lazy = once_cell::sync::Lazy::new(TranslationMetadata::new); - ResolveCtx { query, schema_types: schema, hints: &EMPTY, alias_view: None, legacy } + ResolveCtx { query, schema_types: schema, hints: &EMPTY, alias_view: None, function_projection_names: None, legacy } } fn ctx_with_aliases<'a>(schema: &'a HashMap, alias_view: &'a [AliasItem], legacy: bool) -> ResolveCtx<'a> { static EMPTY: once_cell::sync::Lazy = once_cell::sync::Lazy::new(TranslationMetadata::new); - ResolveCtx { query: "", schema_types: schema, hints: &EMPTY, alias_view: Some(alias_view), legacy } + ResolveCtx { query: "", schema_types: schema, hints: &EMPTY, alias_view: Some(alias_view), function_projection_names: None, legacy } } #[test] @@ -546,4 +574,51 @@ mod tests { assert_eq!(metas[0].wire_name, "?column?"); assert_eq!(metas[1].wire_name, "?column?2"); } + + #[tokio::test] + async fn resolve_columns_from_names_preserves_wildcard_paren_column_without_schema_map() { + let schema = HashMap::new(); + let hints = TranslationMetadata::new(); + let session = crate::session::SessionState::new("db".into(), "user".into()); + let names = vec!["price(usd)".to_string()]; + let metas = resolve_columns_from_names( + &names, "SELECT * FROM t", &schema, &hints, &session, + ) + .await + .unwrap(); + assert_eq!(metas[0].wire_name, "price(usd)"); + assert_eq!(metas[0].type_oid, PgType::Text.to_oid()); + } + + #[tokio::test] + async fn resolve_columns_from_names_ignores_unrelated_where_function_for_wildcard_column() { + let schema = HashMap::new(); + let hints = TranslationMetadata::new(); + let session = crate::session::SessionState::new("db".into(), "user".into()); + let names = vec!["max(value)".to_string()]; + let metas = resolve_columns_from_names( + &names, "SELECT * FROM t WHERE max(other_value) > 0", &schema, &hints, &session, + ) + .await + .unwrap(); + assert_eq!(metas[0].wire_name, "max(value)"); + assert_eq!(metas[0].type_oid, PgType::Text.to_oid()); + } + + #[tokio::test] + async fn resolve_columns_from_names_resolves_matching_function_projection() { + let mut schema = HashMap::new(); + schema.insert("created_at".to_string(), "TIMESTAMP".to_string()); + let hints = TranslationMetadata::new(); + let session = crate::session::SessionState::new("db".into(), "user".into()); + let names = vec!["max(created_at)".to_string()]; + let metas = resolve_columns_from_names( + &names, "SELECT max(created_at) FROM t", &schema, &hints, &session, + ) + .await + .unwrap(); + assert_eq!(metas[0].wire_name, "max"); + assert_eq!(metas[0].type_oid, PgType::Timestamp.to_oid()); + assert!(metas[0].datetime_flag); + } } diff --git a/src/types/schema_type_mapper.rs b/src/types/schema_type_mapper.rs index 04d5ab5..379a865 100644 --- a/src/types/schema_type_mapper.rs +++ b/src/types/schema_type_mapper.rs @@ -2,6 +2,37 @@ use rusqlite::Connection; use crate::types::PgType; use crate::metadata::EnumMetadata; use regex; +use sqlparser::ast::{Expr, ObjectName, ObjectNamePart, SelectItem, SetExpr, Statement}; +use sqlparser::dialect::PostgreSqlDialect; +use sqlparser::parser::Parser; + +fn select_projection_has_unaliased_function(query: &str, function_name: &str) -> bool { + if function_name.is_empty() { + return false; + } + + let Ok(parsed) = Parser::parse_sql(&PostgreSqlDialect {}, query) else { + return false; + }; + let Some(Statement::Query(query)) = parsed.into_iter().next() else { + return false; + }; + let SetExpr::Select(select) = *query.body else { + return false; + }; + + select.projection.iter().any(|item| match item { + SelectItem::UnnamedExpr(Expr::Function(function)) => object_name_last_part_lower(&function.name) + .is_some_and(|projected| projected.eq_ignore_ascii_case(function_name)), + _ => false, + }) +} + +fn object_name_last_part_lower(name: &ObjectName) -> Option { + name.0.last().map(|part| match part { + ObjectNamePart::Identifier(ident) => ident.value.to_lowercase(), + }) +} /// Maps between PostgreSQL and SQLite types using actual schema information pub struct SchemaTypeMapper; @@ -365,13 +396,11 @@ impl SchemaTypeMapper { // If the conformant column name is the bare function name for an // unaliased call (e.g. json_extract(...)), recover the function - // return type from the query text. - let direct_call_pattern = format!(r"(?i)(?:^|\bSELECT\s+|,)\s*{}\s*\(", regex::escape(function_name)); - if let Ok(re) = regex::Regex::new(&direct_call_pattern) - && re.is_match(q) { - return Self::get_aggregate_return_type_with_query( - &format!("{function_name}("), conn, table_name, None); - } + // return type from the SELECT projection AST only. + if select_projection_has_unaliased_function(q, function_name) { + return Self::get_aggregate_return_type_with_query( + &format!("{function_name}("), conn, table_name, None); + } // Also check for array concatenation operator pattern: column || array AS alias // NOTE: For now, we return TEXT instead of TextArray because: @@ -588,6 +617,19 @@ mod tests { ); } + #[test] + fn test_bare_json_extract_alias_with_where_comma_call_does_not_resolve_text() { + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "json_extract", + None, + None, + Some("SELECT upper(name) AS json_extract FROM t WHERE x IN (1, json_extract(data, '$.x'))"), + ), + None, + ); + } + // The query-regex fallback (kept) still resolves aliased aggregates with query context. #[test] fn test_bare_aggregate_with_query_context_still_resolves() { From ad9c48d297f090e446d8592bc57110baf87721e5 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 21:15:02 -0700 Subject: [PATCH 19/21] fix: account for wildcard expansion in projection resolver --- src/query/projection_resolver.rs | 104 ++++++++++++++++++++++++++++--- 1 file changed, 96 insertions(+), 8 deletions(-) diff --git a/src/query/projection_resolver.rs b/src/query/projection_resolver.rs index 67d1dd3..f08030e 100644 --- a/src/query/projection_resolver.rs +++ b/src/query/projection_resolver.rs @@ -53,12 +53,58 @@ fn parse_quoted_projection_names(query: &str) -> Option>> { }).collect()) } -fn parse_function_projection_names(query: &str) -> Option>> { +fn parse_function_projection_names(query: &str, result_column_count: usize) -> Option>> { let select = parse_select_projection(query)?; - Some(select.projection.iter().map(|item| match item { + Some(function_projection_names_for_result_columns(&select.projection, result_column_count)) +} + +fn function_projection_names_for_result_columns(projection: &[SelectItem], result_column_count: usize) -> Vec> { + let non_wildcard_count = projection.iter().filter(|item| !is_wildcard_projection(item)).count(); + let wildcard_count = projection.len() - non_wildcard_count; + + if wildcard_count > 1 { + let mut names = Vec::with_capacity(result_column_count); + for item in projection { + if is_wildcard_projection(item) { + names.truncate(result_column_count); + names.resize(result_column_count, None); + return names; + } + names.push(function_projection_name(item)); + } + names.truncate(result_column_count); + names.resize(result_column_count, None); + return names; + } + + let wildcard_expansion = if wildcard_count == 1 { + result_column_count.saturating_sub(non_wildcard_count) + } else { + 0 + }; + + let mut names = Vec::with_capacity(result_column_count.max(projection.len())); + for item in projection { + if is_wildcard_projection(item) { + names.resize(names.len() + wildcard_expansion, None); + } else { + names.push(function_projection_name(item)); + } + } + names.truncate(result_column_count); + names.resize(result_column_count, None); + names +} + +fn function_projection_name(item: &SelectItem) -> Option { + match item { SelectItem::UnnamedExpr(Expr::Function(function)) => object_name_last_part_lower(&function.name), _ => None, - }).collect()) + } +} + +fn is_wildcard_projection(item: &SelectItem) -> bool { + matches!(item, SelectItem::Wildcard(_) | SelectItem::QualifiedWildcard(_, _)) } fn object_name_last_part_lower(name: &ObjectName) -> Option { @@ -228,8 +274,16 @@ fn projection_function_matches(ctx: &ResolveCtx, position: usize, fn_name: &str) .is_some_and(|projected| projected.eq_ignore_ascii_case(fn_name)); } - parse_function_projection_names(ctx.query) - .and_then(|names| names.get(position).cloned().flatten()) + let select = match parse_select_projection(ctx.query) { + Some(select) => select, + None => return false, + }; + if select.projection.iter().any(is_wildcard_projection) { + return false; + } + function_projection_names_for_result_columns(&select.projection, position + 1) + .get(position) + .and_then(|name| name.as_deref()) .is_some_and(|projected| projected.eq_ignore_ascii_case(fn_name)) } @@ -274,9 +328,9 @@ pub fn resolve_columns_with_legacy( let view_ref = alias_view.as_deref(); let quoted_projection_names = parse_quoted_projection_names(query); let quoted_ref = quoted_projection_names.as_deref(); - let function_projection_names = parse_function_projection_names(query); - let function_ref = function_projection_names.as_deref(); let count = stmt.column_count(); + let function_projection_names = parse_function_projection_names(query, count); + let function_ref = function_projection_names.as_deref(); let mut out = Vec::with_capacity(count); for i in 0..count { let raw = stmt.column_name(i)?.to_string(); @@ -313,7 +367,7 @@ pub async fn resolve_columns_from_names( let view_ref = alias_view.as_deref(); let quoted_projection_names = parse_quoted_projection_names(query); let quoted_ref = quoted_projection_names.as_deref(); - let function_projection_names = parse_function_projection_names(query); + let function_projection_names = parse_function_projection_names(query, names.len()); let function_ref = function_projection_names.as_deref(); let mut out = Vec::with_capacity(names.len()); for (i, raw) in names.iter().enumerate() { @@ -621,4 +675,38 @@ mod tests { assert_eq!(metas[0].type_oid, PgType::Timestamp.to_oid()); assert!(metas[0].datetime_flag); } + + #[tokio::test] + async fn resolve_columns_from_names_resolves_function_after_wildcard_expansion() { + let mut schema = HashMap::new(); + schema.insert("created_at".to_string(), "TIMESTAMP".to_string()); + let hints = TranslationMetadata::new(); + let session = crate::session::SessionState::new("db".into(), "user".into()); + let names = vec!["id".to_string(), "name".to_string(), "max(created_at)".to_string()]; + let metas = resolve_columns_from_names( + &names, "SELECT *, max(created_at) FROM t", &schema, &hints, &session, + ) + .await + .unwrap(); + assert_eq!(metas[2].wire_name, "max"); + assert_eq!(metas[2].type_oid, PgType::Timestamp.to_oid()); + assert!(metas[2].datetime_flag); + } + + #[tokio::test] + async fn resolve_columns_from_names_resolves_function_before_wildcard_expansion() { + let mut schema = HashMap::new(); + schema.insert("created_at".to_string(), "TIMESTAMP".to_string()); + let hints = TranslationMetadata::new(); + let session = crate::session::SessionState::new("db".into(), "user".into()); + let names = vec!["max(created_at)".to_string(), "id".to_string(), "name".to_string()]; + let metas = resolve_columns_from_names( + &names, "SELECT max(created_at), * FROM t", &schema, &hints, &session, + ) + .await + .unwrap(); + assert_eq!(metas[0].wire_name, "max"); + assert_eq!(metas[0].type_oid, PgType::Timestamp.to_oid()); + assert!(metas[0].datetime_flag); + } } From 9f6b3dd445f935214f35787232e44a7b23fc4c80 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 21:47:16 -0700 Subject: [PATCH 20/21] fix: align legacy tests with conformant result metadata --- src/types/schema_type_mapper.rs | 48 ++++++++++++++++++++++- tests/datetime_conversion_success_test.rs | 6 +-- tests/datetime_roundtrip_test.rs | 8 ++-- tests/datetime_translator_test.rs | 4 +- tests/insert_translator_unit_test.rs | 6 +-- tests/row_to_json_test.rs | 42 ++++++++++++-------- 6 files changed, 83 insertions(+), 31 deletions(-) diff --git a/src/types/schema_type_mapper.rs b/src/types/schema_type_mapper.rs index 379a865..5c5de52 100644 --- a/src/types/schema_type_mapper.rs +++ b/src/types/schema_type_mapper.rs @@ -397,9 +397,22 @@ impl SchemaTypeMapper { // If the conformant column name is the bare function name for an // unaliased call (e.g. json_extract(...)), recover the function // return type from the SELECT projection AST only. - if select_projection_has_unaliased_function(q, function_name) { + let function_name_lower = function_name.to_ascii_lowercase(); + let projection_matches = select_projection_has_unaliased_function(q, function_name) + || (function_name_lower == "current_timestamp" && select_projection_has_unaliased_function(q, "now")) + || (function_name_lower == "now" && select_projection_has_unaliased_function(q, "current_timestamp")); + + if projection_matches { + let function_call = if matches!( + function_name_lower.as_str(), + "now" | "current_timestamp" | "current_time" | "current_date" | "epoch" + ) { + format!("{function_name}()") + } else { + format!("{function_name}(") + }; return Self::get_aggregate_return_type_with_query( - &format!("{function_name}("), conn, table_name, None); + &function_call, conn, table_name, None); } // Also check for array concatenation operator pattern: column || array AS alias @@ -604,6 +617,37 @@ mod tests { ); } + #[test] + fn test_bare_zero_arg_datetime_functions_with_query_context_resolve_timestamp() { + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "now", + None, + None, + Some("SELECT now()"), + ), + Some(PgType::Timestamptz.to_oid()), + ); + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "current_timestamp", + None, + None, + Some("SELECT current_timestamp()"), + ), + Some(PgType::Timestamptz.to_oid()), + ); + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "current_timestamp", + None, + None, + Some("SELECT now()"), + ), + Some(PgType::Timestamptz.to_oid()), + ); + } + #[test] fn test_bare_json_extract_alias_with_where_call_does_not_resolve_text() { assert_eq!( diff --git a/tests/datetime_conversion_success_test.rs b/tests/datetime_conversion_success_test.rs index a213bcb..fe5f1b5 100644 --- a/tests/datetime_conversion_success_test.rs +++ b/tests/datetime_conversion_success_test.rs @@ -19,13 +19,13 @@ async fn test_datetime_conversion_success() { // Verify storage is INTEGER let storage_check = client.simple_query( - "SELECT typeof(date_col), typeof(time_col) FROM dt_test WHERE id = 1" + "SELECT typeof(date_col) AS date_storage_type, typeof(time_col) AS time_storage_type FROM dt_test WHERE id = 1" ).await.unwrap(); if let Some(msg) = storage_check.into_iter().find(|m| matches!(m, tokio_postgres::SimpleQueryMessage::Row(_))) && let tokio_postgres::SimpleQueryMessage::Row(data) = msg { - let date_type = data.get("typeof(date_col)").unwrap(); - let time_type = data.get("typeof(time_col)").unwrap(); + let date_type = data.get("date_storage_type").unwrap(); + let time_type = data.get("time_storage_type").unwrap(); println!("Storage check:"); println!(" Date type: {date_type}"); diff --git a/tests/datetime_roundtrip_test.rs b/tests/datetime_roundtrip_test.rs index 902a564..503805a 100644 --- a/tests/datetime_roundtrip_test.rs +++ b/tests/datetime_roundtrip_test.rs @@ -40,15 +40,15 @@ async fn test_datetime_roundtrip() { // Verify storage is INTEGER let storage = client.simple_query( - "SELECT typeof(date_col), typeof(time_col), typeof(timestamp_col) FROM roundtrip_test WHERE id = 1" + "SELECT typeof(date_col) AS date_storage_type, typeof(time_col) AS time_storage_type, typeof(timestamp_col) AS timestamp_storage_type FROM roundtrip_test WHERE id = 1" ).await.unwrap(); if let Some(msg) = storage.into_iter().find(|m| matches!(m, tokio_postgres::SimpleQueryMessage::Row(_))) { if let tokio_postgres::SimpleQueryMessage::Row(data) = msg { println!("\nStorage types:"); - println!(" date: {}", data.get("typeof(date_col)").unwrap()); - println!(" time: {}", data.get("typeof(time_col)").unwrap()); - println!(" timestamp: {}", data.get("typeof(timestamp_col)").unwrap()); + println!(" date: {}", data.get("date_storage_type").unwrap()); + println!(" time: {}", data.get("time_storage_type").unwrap()); + println!(" timestamp: {}", data.get("timestamp_storage_type").unwrap()); } } diff --git a/tests/datetime_translator_test.rs b/tests/datetime_translator_test.rs index 2bfa0d7..554ff71 100644 --- a/tests/datetime_translator_test.rs +++ b/tests/datetime_translator_test.rs @@ -112,12 +112,12 @@ async fn test_insert_translator_conversion() { // Check storage type let type_check = client.simple_query( - "SELECT typeof(date_col) FROM translator_test WHERE id = 1" + "SELECT typeof(date_col) AS date_storage_type FROM translator_test WHERE id = 1" ).await.unwrap(); if let Some(msg) = type_check.into_iter().find(|m| matches!(m, tokio_postgres::SimpleQueryMessage::Row(_))) && let tokio_postgres::SimpleQueryMessage::Row(data) = msg { - let col_type = data.get("typeof(date_col)").unwrap(); + let col_type = data.get("date_storage_type").unwrap(); println!("Storage type after InsertTranslator: {col_type}"); assert_eq!(col_type, "integer", "InsertTranslator should convert to INTEGER"); } diff --git a/tests/insert_translator_unit_test.rs b/tests/insert_translator_unit_test.rs index d32554c..05f1d34 100644 --- a/tests/insert_translator_unit_test.rs +++ b/tests/insert_translator_unit_test.rs @@ -25,13 +25,13 @@ async fn test_insert_execution_path() { // Check storage type let type_check = server.client.simple_query( - "SELECT typeof(date_col), typeof(time_col) FROM test_table WHERE id = 1" + "SELECT typeof(date_col) AS date_storage_type, typeof(time_col) AS time_storage_type FROM test_table WHERE id = 1" ).await.unwrap(); if let Some(row) = type_check.into_iter().find(|m| matches!(m, tokio_postgres::SimpleQueryMessage::Row(_))) && let tokio_postgres::SimpleQueryMessage::Row(data) = row { - let date_type = data.get("typeof(date_col)").unwrap(); - let time_type = data.get("typeof(time_col)").unwrap(); + let date_type = data.get("date_storage_type").unwrap(); + let time_type = data.get("time_storage_type").unwrap(); eprintln!("Storage types:"); eprintln!(" Date: {date_type}"); diff --git a/tests/row_to_json_test.rs b/tests/row_to_json_test.rs index de72cbf..687dffc 100644 --- a/tests/row_to_json_test.rs +++ b/tests/row_to_json_test.rs @@ -63,13 +63,15 @@ async fn test_row_to_json_basic_subquery() { ).await.expect("Failed to insert data"); // Test row_to_json with subquery - let rows = client.query( - "SELECT row_to_json(t) FROM (SELECT name, age FROM users WHERE id = 1) t", - &[] + let messages = client.simple_query( + "SELECT row_to_json(t) FROM (SELECT name, age FROM users WHERE id = 1) t" ).await.expect("Failed to execute query"); + let rows: Vec<_> = messages.into_iter() + .filter_map(|m| match m { tokio_postgres::SimpleQueryMessage::Row(row) => Some(row), _ => None }) + .collect(); assert_eq!(rows.len(), 1); - let json_result: String = rows[0].get(0); + let json_result = rows[0].get(0).unwrap().to_string(); // The result should be a JSON object with name and age assert!(json_result.contains("\"name\":\"Alice\"") || json_result.contains("\"name\": \"Alice\"")); @@ -97,13 +99,15 @@ async fn test_row_to_json_multiple_columns() { ).await.expect("Failed to insert data"); // Test row_to_json with multiple column types - let rows = client.query( - "SELECT row_to_json(p) FROM (SELECT id, name, price, in_stock FROM products WHERE id = 1) p", - &[] + let messages = client.simple_query( + "SELECT row_to_json(p) FROM (SELECT id, name, price, in_stock FROM products WHERE id = 1) p" ).await.expect("Failed to execute query"); + let rows: Vec<_> = messages.into_iter() + .filter_map(|m| match m { tokio_postgres::SimpleQueryMessage::Row(row) => Some(row), _ => None }) + .collect(); assert_eq!(rows.len(), 1); - let json_result: String = rows[0].get(0); + let json_result = rows[0].get(0).unwrap().to_string(); println!("Multi-column row to JSON result: {json_result}"); @@ -135,13 +139,15 @@ async fn test_row_to_json_with_aliases() { ).await.expect("Failed to insert data"); // Test row_to_json with column aliases - let rows = client.query( - "SELECT row_to_json(e) FROM (SELECT first_name AS fname, last_name AS lname, salary FROM employees WHERE id = 1) e", - &[] + let messages = client.simple_query( + "SELECT row_to_json(e) FROM (SELECT first_name AS fname, last_name AS lname, salary FROM employees WHERE id = 1) e" ).await.expect("Failed to execute query"); + let rows: Vec<_> = messages.into_iter() + .filter_map(|m| match m { tokio_postgres::SimpleQueryMessage::Row(row) => Some(row), _ => None }) + .collect(); assert_eq!(rows.len(), 1); - let json_result: String = rows[0].get(0); + let json_result = rows[0].get(0).unwrap().to_string(); // Verify aliases are used in the JSON assert!(json_result.contains("\"fname\":\"John\"") || json_result.contains("\"fname\": \"John\"")); @@ -193,15 +199,17 @@ async fn test_row_to_json_multiple_rows() { ).await.expect("Failed to insert data"); // Test row_to_json returning multiple rows - let rows = client.query( - "SELECT row_to_json(i) FROM (SELECT name, category FROM items ORDER BY id) i", - &[] + let messages = client.simple_query( + "SELECT row_to_json(i) FROM (SELECT name, category FROM items ORDER BY id) i" ).await.expect("Failed to execute query"); + let rows: Vec<_> = messages.into_iter() + .filter_map(|m| match m { tokio_postgres::SimpleQueryMessage::Row(row) => Some(row), _ => None }) + .collect(); assert_eq!(rows.len(), 2); - let json_result1: String = rows[0].get(0); - let json_result2: String = rows[1].get(0); + let json_result1 = rows[0].get(0).unwrap().to_string(); + let json_result2 = rows[1].get(0).unwrap().to_string(); // Verify both rows are correctly converted assert!(json_result1.contains("\"name\":\"Apple\"") || json_result1.contains("\"name\": \"Apple\"")); From 5923ac461dbaa6173b2e95a1d92de36ed6499f17 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Thu, 2 Jul 2026 22:03:49 -0700 Subject: [PATCH 21/21] fix: tighten final conformance test alignment --- Cargo.lock | 2 ++ Cargo.toml | 2 +- src/types/schema_type_mapper.rs | 37 ++++++++++++++++-------- tests/row_to_json_test.rs | 50 ++++++++++++++++----------------- 4 files changed, 53 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3d1a550..50c9227 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1408,6 +1408,8 @@ dependencies = [ "chrono", "fallible-iterator 0.2.0", "postgres-protocol", + "serde", + "serde_json", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index cf9851a..17376e8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ rust_decimal = { version = "1.35.0", features = ["serde", "db-postgres"] } once_cell = "1.20.0" # PostgreSQL client (for testing) -tokio-postgres = { version = "0.7.12", features = ["with-chrono-0_4"] } +tokio-postgres = { version = "0.7.12", features = ["with-chrono-0_4", "with-serde_json-1"] } env_logger = "0.11.5" # Utilities diff --git a/src/types/schema_type_mapper.rs b/src/types/schema_type_mapper.rs index 5c5de52..ab96abc 100644 --- a/src/types/schema_type_mapper.rs +++ b/src/types/schema_type_mapper.rs @@ -6,22 +6,27 @@ use sqlparser::ast::{Expr, ObjectName, ObjectNamePart, SelectItem, SetExpr, Stat use sqlparser::dialect::PostgreSqlDialect; use sqlparser::parser::Parser; +fn parse_select_projection(query: &str) -> Option> { + let parsed = Parser::parse_sql(&PostgreSqlDialect {}, query).ok()?; + let Statement::Query(query) = parsed.into_iter().next()? else { + return None; + }; + let SetExpr::Select(select) = *query.body else { + return None; + }; + Some(select.projection) +} + fn select_projection_has_unaliased_function(query: &str, function_name: &str) -> bool { if function_name.is_empty() { return false; } - let Ok(parsed) = Parser::parse_sql(&PostgreSqlDialect {}, query) else { - return false; - }; - let Some(Statement::Query(query)) = parsed.into_iter().next() else { - return false; - }; - let SetExpr::Select(select) = *query.body else { + let Some(projection) = parse_select_projection(query) else { return false; }; - select.projection.iter().any(|item| match item { + projection.iter().any(|item| match item { SelectItem::UnnamedExpr(Expr::Function(function)) => object_name_last_part_lower(&function.name) .is_some_and(|projected| projected.eq_ignore_ascii_case(function_name)), _ => false, @@ -399,13 +404,14 @@ impl SchemaTypeMapper { // return type from the SELECT projection AST only. let function_name_lower = function_name.to_ascii_lowercase(); let projection_matches = select_projection_has_unaliased_function(q, function_name) - || (function_name_lower == "current_timestamp" && select_projection_has_unaliased_function(q, "now")) - || (function_name_lower == "now" && select_projection_has_unaliased_function(q, "current_timestamp")); + || (parse_select_projection(q).is_some_and(|projection| projection.len() == 1) + && ((function_name_lower == "current_timestamp" && select_projection_has_unaliased_function(q, "now")) + || (function_name_lower == "now" && select_projection_has_unaliased_function(q, "current_timestamp")))); if projection_matches { let function_call = if matches!( function_name_lower.as_str(), - "now" | "current_timestamp" | "current_time" | "current_date" | "epoch" + "now" | "current_timestamp" | "current_time" | "epoch" ) { format!("{function_name}()") } else { @@ -646,6 +652,15 @@ mod tests { ), Some(PgType::Timestamptz.to_oid()), ); + assert_eq!( + SchemaTypeMapper::get_aggregate_return_type_with_query( + "current_timestamp", + None, + None, + Some("SELECT now(), 'x' AS current_timestamp"), + ), + None, + ); } #[test] diff --git a/tests/row_to_json_test.rs b/tests/row_to_json_test.rs index 687dffc..aa2f621 100644 --- a/tests/row_to_json_test.rs +++ b/tests/row_to_json_test.rs @@ -1,4 +1,5 @@ use tokio_postgres::{Client, NoTls}; +use tokio_postgres::types::Json; use tokio::net::TcpListener; use std::time::Duration; use tokio::time::timeout; @@ -62,16 +63,15 @@ async fn test_row_to_json_basic_subquery() { &[] ).await.expect("Failed to insert data"); - // Test row_to_json with subquery - let messages = client.simple_query( - "SELECT row_to_json(t) FROM (SELECT name, age FROM users WHERE id = 1) t" + // Test row_to_json with subquery over the extended protocol. + let rows = client.query( + "SELECT row_to_json(t) FROM (SELECT name, age FROM users WHERE id = 1) t", + &[] ).await.expect("Failed to execute query"); - let rows: Vec<_> = messages.into_iter() - .filter_map(|m| match m { tokio_postgres::SimpleQueryMessage::Row(row) => Some(row), _ => None }) - .collect(); assert_eq!(rows.len(), 1); - let json_result = rows[0].get(0).unwrap().to_string(); + let Json(json_value): Json = rows[0].get(0); + let json_result = json_value.to_string(); // The result should be a JSON object with name and age assert!(json_result.contains("\"name\":\"Alice\"") || json_result.contains("\"name\": \"Alice\"")); @@ -99,15 +99,14 @@ async fn test_row_to_json_multiple_columns() { ).await.expect("Failed to insert data"); // Test row_to_json with multiple column types - let messages = client.simple_query( - "SELECT row_to_json(p) FROM (SELECT id, name, price, in_stock FROM products WHERE id = 1) p" + let rows = client.query( + "SELECT row_to_json(p) FROM (SELECT id, name, price, in_stock FROM products WHERE id = 1) p", + &[] ).await.expect("Failed to execute query"); - let rows: Vec<_> = messages.into_iter() - .filter_map(|m| match m { tokio_postgres::SimpleQueryMessage::Row(row) => Some(row), _ => None }) - .collect(); assert_eq!(rows.len(), 1); - let json_result = rows[0].get(0).unwrap().to_string(); + let Json(json_value): Json = rows[0].get(0); + let json_result = json_value.to_string(); println!("Multi-column row to JSON result: {json_result}"); @@ -139,15 +138,14 @@ async fn test_row_to_json_with_aliases() { ).await.expect("Failed to insert data"); // Test row_to_json with column aliases - let messages = client.simple_query( - "SELECT row_to_json(e) FROM (SELECT first_name AS fname, last_name AS lname, salary FROM employees WHERE id = 1) e" + let rows = client.query( + "SELECT row_to_json(e) FROM (SELECT first_name AS fname, last_name AS lname, salary FROM employees WHERE id = 1) e", + &[] ).await.expect("Failed to execute query"); - let rows: Vec<_> = messages.into_iter() - .filter_map(|m| match m { tokio_postgres::SimpleQueryMessage::Row(row) => Some(row), _ => None }) - .collect(); assert_eq!(rows.len(), 1); - let json_result = rows[0].get(0).unwrap().to_string(); + let Json(json_value): Json = rows[0].get(0); + let json_result = json_value.to_string(); // Verify aliases are used in the JSON assert!(json_result.contains("\"fname\":\"John\"") || json_result.contains("\"fname\": \"John\"")); @@ -199,17 +197,17 @@ async fn test_row_to_json_multiple_rows() { ).await.expect("Failed to insert data"); // Test row_to_json returning multiple rows - let messages = client.simple_query( - "SELECT row_to_json(i) FROM (SELECT name, category FROM items ORDER BY id) i" + let rows = client.query( + "SELECT row_to_json(i) FROM (SELECT name, category FROM items ORDER BY id) i", + &[] ).await.expect("Failed to execute query"); - let rows: Vec<_> = messages.into_iter() - .filter_map(|m| match m { tokio_postgres::SimpleQueryMessage::Row(row) => Some(row), _ => None }) - .collect(); assert_eq!(rows.len(), 2); - let json_result1 = rows[0].get(0).unwrap().to_string(); - let json_result2 = rows[1].get(0).unwrap().to_string(); + let Json(json_value1): Json = rows[0].get(0); + let Json(json_value2): Json = rows[1].get(0); + let json_result1 = json_value1.to_string(); + let json_result2 = json_value2.to_string(); // Verify both rows are correctly converted assert!(json_result1.contains("\"name\":\"Apple\"") || json_result1.contains("\"name\": \"Apple\""));