Implement PostgreSQL-conformant result columns#79
Merged
Conversation
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.
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.
…mn?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.
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).
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).
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pgsqlite.legacy_result_columnsescape hatch.Verification
cargo checkcargo clippy --quietcargo buildcargo testPATH="/tmp/pgsqlite-test-tools:$PATH" META_COMMAND_FILE=/tmp/nonexistent ./tests/runner/run_ssl_tests.sh -s tests/sql/features/column_conformance.sql