Skip to content

Fix DuckDB identifier and struct value quoting for special characters - #297

Merged
sfc-gh-dachristensen merged 17 commits into
mainfrom
pgguru/duckdb-quote-identifier
May 12, 2026
Merged

Fix DuckDB identifier and struct value quoting for special characters#297
sfc-gh-dachristensen merged 17 commits into
mainfrom
pgguru/duckdb-quote-identifier

Conversation

@sfc-gh-dachristensen

@sfc-gh-dachristensen sfc-gh-dachristensen commented Apr 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Harden the pg_lake → pgduck_server SQL emission paths against identifier quoting and struct field-name escaping mismatches between PostgreSQL and DuckDB.

Identifier quoting

  • Add duckdb_quote_identifier() that quotes identifiers reserved in DuckDB but not in PostgreSQL — both RESERVED_KEYWORD (pivot, qualify, lambda, summarize,
    from DuckDB's vendored kwlist.hpp via tools/generate_duckdb_kwlist.py. CI now runs make check-duckdb-kwlist to catch a stale table after a DuckDB bump; the script
    falls back to fetching the file from raw.githubusercontent.com at the pinned submodule SHA so CI doesn't need the submodule initialised.
  • Route every relation / schema / column / function / operator-namespace name through duckdb_quote_identifier() in both the ruleutils deparse path
    (deparse_ruleutils.c) and the postgres_fdw-style deparser (deparse.c). Fixes pivot.t style schema-qualified names appearing unquoted in EXPLAIN (VERBOSE) output.
  • deparse_ruleutils.c uses set_config_option("quote_all_identifiers", "on", …, GUC_ACTION_SAVE, …) so AtEOXact_GUC unwinds the GUC stack even if pg_get_querydef
    throws.

STRUCT field-name handling

  • QuoteDuckDBFieldName emits SQL-standard "" doubling (what DuckDB and SQL both expect) instead of C-style \" backslash escaping.
  • ParseDuckDBFieldName rewritten to consume SQL-standard "" doubling; the paren-depth scanner in ParseDuckDBFieldType skips over quoted regions so embedded ( /
    ) in field names don't break struct parsing.
  • StructOutForPGDuck uses DuckDB-compatible backslash escaping for struct literal values travelling through CSV, because that context needs to round-trip through
    the outer CSV layer.
  • Two helpers exposed via struct_conversion.h:
    • QuoteDuckDBStructKey (backslash escaping) for struct literals emitted as CSV values — the struct_conversion.c:StructOutForPGDuck path.
    • QuoteDuckDBStructKeySQL (SQL-standard '' doubling) for struct literals concatenated directly into SQL query text — the read_data.c:TupleDescToStructProjection
      path, including the INTERVAL / struct-of-INTERVAL / INTERVAL[] emit branches. Previously used bare '%s': which truncated on any field name containing '.

Composite-type catalog lookup

  • GetOrCreatePGStructType previously psprintf'd composite field names directly into a '{...}'::name[] array literal, producing "malformed array literal" for field
    names containing ,, :, quotes, spaces, parens, or backslashes during auto-detect. Switched to SPI_execute_with_args with the names and oids passed as proper
    name[] / oid[] Datums constructed via construct_array_builtin.

CI / infrastructure

  • Artifact name for postgres-logs upload now includes worker_id (when the matrix supplies one) and from_pg_version (for the upgrade matrix), so parallel matrix
    jobs stop colliding on the same artifact name.
  • Lint workflow runs make check-duckdb-kwlist.

Test plan

Reserved-keyword column names

  • test_{read,read_csv,read_json,writable_parquet,iceberg_table}_with_reserved_keyword_columns
  • test_each_reserved_keyword_{parquet,csv,json}[kw] — parameterized across every DuckDB-only reserved keyword; covers SELECT + WHERE pushdown + EXPLAIN-quoted
    verification
  • test_iceberg_table_with_reserved_keyword_columns, test_iceberg_table_insert_select_all_reserved_keywords,
    test_readable_iceberg_foreign_table_with_reserved_keyword_columns
  • test_copy_roundtrip_{parquet,csv,json}[kw] (in pg_lake_copy)

Typed struct fields with reserved-keyword names

  • test_reserved_keyword_struct_field_typed[interval|timestamp|interval[]] — INTERVAL / TIMESTAMP / INTERVAL[] emit branches in read_data.c
  • test_reserved_keyword_geometry_column — iceberg FDW with a pivot geometry column; SELECT + filter pushdown
  • test_reserved_keyword_geometry_roundtrip[parquet|csv|json] — parameterized round-trip exercising ST_AsWKB / ST_AsGeoJSON / ST_GeomFromWKB / ST_GeomFromText /
    ST_GeomFromGeoJSON with reserved column names
  • test_iceberg_map_of_interval_with_reserved_column — MAP-of-INTERVAL column named pivot
  • test_iceberg_timetz_to_time_cast_on_reserved_columnwrite_data.c TIMETZ→TIME CAST branch
  • test_iceberg_overflow_conversion_on_reserved_column — type-widening CAST projection path
  • test_s3log_strptime_with_reserved_column_name — LOG-format foreign table hitting the strptime() wrapper branch

STRUCT field-name edge cases

  • test_iceberg_composite_field_with_special_characters — spaces, ", ', \ in field names
  • test_iceberg_composite_field_with_embedded_double_quoteU&"has\0022quote"
  • test_struct_of_interval_field_name_with_single_quote — struct-of-INTERVAL with a '-bearing field name
  • test_autodetect_struct_field_names_with_hostile_charactersCREATE FOREIGN TABLE () SERVER pg_lake OPTIONS (path …) against a parquet file whose struct fields
    have commas, colons, quotes, spaces, parens, backslashes (covers the SPI parameter-binding fix)
  • test_copy_roundtrip_composite_with_embedded_quote, test_copy_roundtrip_csv_composite_with_embedded_quote — parquet/CSV round-trips

Other pg_lake_engine paths

  • test_iceberg_analyze_with_reserved_columns — ANALYZE on iceberg FDW with reserved-keyword columns
  • test_reserved_keyword_schema_with_custom_function_and_operator — schema-qualified function/operator deparse with a reserved schema name

@sfc-gh-dachristensen
sfc-gh-dachristensen force-pushed the pgguru/duckdb-quote-identifier branch 2 times, most recently from 281231c to 1f60253 Compare April 2, 2026 23:21
@sfc-gh-dachristensen
sfc-gh-dachristensen marked this pull request as ready for review April 3, 2026 00:51
@sfc-gh-dachristensen
sfc-gh-dachristensen force-pushed the pgguru/duckdb-quote-identifier branch 7 times, most recently from 24cde4c to 3c29903 Compare May 5, 2026 20:14
Replace the fragile RequoteDuckDBReservedInSQL() string scanner with
PostgreSQL's built-in quote_all_identifiers mechanism.  Setting this
flag before calling pg_get_querydef() causes all identifiers to be
double-quoted, which covers DuckDB-reserved words without needing a
separate post-processing pass.

Update all test assertions that check deparsed SQL fragments to expect
the now-quoted identifiers (function names, column names, field access).

Retain duckdb_quote_identifier() for direct SQL building paths (e.g.,
read_data.c, write_data.c) that don't go through pg_get_querydef.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
@sfc-gh-dachristensen
sfc-gh-dachristensen force-pushed the pgguru/duckdb-quote-identifier branch from 3c29903 to ca2eebb Compare May 5, 2026 20:42
QuoteDuckDBFieldName() used C-style backslash escaping (e.g. \") for
special characters in composite type field names. DuckDB's SQL parser
expects standard SQL identifier escaping (""). This caused parse errors
when composite types had field names containing double-quote characters,
crashing the CDC worker in a restart loop.

Replace the custom escaping logic with a delegation to
duckdb_quote_identifier(), which correctly uses standard SQL "" doubling
and also handles DuckDB reserved keywords.

Add tests for composite type field names with embedded double-quotes
and other special characters in both Iceberg tables and COPY paths.

Ref: https://github.com/snowflake-eng/sfpg-extension-pg_lake_replication/issues/361

Signed-off-by: David Christensen <david.christensen@snowflake.com>
@sfc-gh-dachristensen
sfc-gh-dachristensen force-pushed the pgguru/duckdb-quote-identifier branch from 5ed7273 to b2608f9 Compare May 6, 2026 20:03
StructOutForPGDuck() used PostgreSQL's quote_literal_cstr() which doubles
single-quotes ('has''single'). DuckDB's struct literal parser expects
backslash escaping ('has\'single'). This caused read_csv() to fail when
parsing struct values with single-quote characters in field names.

Replace quote_literal_cstr() with QuoteDuckDBStructKey() which uses
DuckDB-compatible backslash escaping for single-quotes and backslashes.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
Extend composite type tests to include fields with backslash characters
in their names, verifying that QuoteDuckDBStructKey() correctly escapes
both single-quotes and backslashes in struct literal serialization.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
@sfc-gh-dachristensen sfc-gh-dachristensen changed the title Add duckdb_quote_identifier() to fix quoting for DuckDB pushdown Fix DuckDB identifier and struct value quoting for special characters May 7, 2026
Comment thread Makefile
Comment thread pg_lake_table/src/fdw/deparse_ruleutils.c Outdated
Comment thread pg_lake_engine/src/pgduck/parse_struct.c
Comment thread pg_lake_engine/src/pgduck/keywords.c
Comment thread pg_lake_engine/src/pgduck/parse_struct.c
Comment thread pg_lake_table/tests/pytests/test_duckdb_reserved_keywords.py
Comment thread pg_lake_engine/include/pg_lake/pgduck/keywords.h
Comment thread pg_lake_table/src/fdw/deparse_ruleutils.c
- keywords.c: duckdb_quote_identifier now quotes any non-UNRESERVED
  DuckDB keyword (COL_NAME, TYPE_FUNC_NAME, RESERVED), matching the
  predicate used by the removed IsDuckDBReservedWord.  Fixes asof,
  anti, glob and other COL_NAME_KEYWORD identifiers that previously
  slipped through.
- keywords.{c,h}: remove unused IsDuckDBReservedWord.
- parse_struct.c: ParseDuckDBFieldName now handles SQL-standard
  doubled-quote ("") escaping to match what DuckDB DESCRIBE and our
  ForceQuoteIdentifier emit, replacing the old C-style backslash
  handling.  ParseDuckDBFieldType skips over quoted field names so
  embedded parentheses don't break struct depth tracking.
- deparse_ruleutils.c: set quote_all_identifiers via set_config_option
  with GUC_ACTION_SAVE so AtEOXact_GUC restores the prior value even
  if pg_get_querydef throws.
- deparse.c: route all relation, schema, column, function, and
  operator-namespace name emission through duckdb_quote_identifier so
  DuckDB-only reserved words (pivot, qualify, lambda, ...) in this
  deparse path are quoted consistent with the ruleutils path.
- test_duckdb_reserved_keywords.py: add read-only iceberg foreign
  table coverage for reserved keyword column names.
- lint.yml: run make check-duckdb-kwlist in CI to catch stale keyword
  tables after a DuckDB version bump.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
- Rewrap two comments in keywords.c and parse_struct.c to satisfy
  check-indent after recent edits.
- Append worker_id to the postgres-logs artifact name when the matrix
  supplies one, so the five split workers don't collide on a single
  artifact name and overwrite each other's logs.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
In CI we don't initialise the duckdb submodule, which made
check-duckdb-kwlist fail with "source file not found" on the lint
runner.  Rather than cloning all of DuckDB just to read one header,
resolve the submodule SHA with git ls-tree and fetch the file from
raw.githubusercontent.com.  The local submodule still wins when it is
present, so developer workflow is unchanged.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
The upgrade matrix runs multiple from-version pairs (16->18, 17->18)
that share a target pg_version, so the postgres-logs artifact name
still collided after the worker_id fix.  Append -from<N> when the
run-test action has a from_pg_version input.

Signed-off-by: David Christensen <david.christensen@snowflake.com>

@sfc-gh-okalaci sfc-gh-okalaci left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are very close to merge this, few edge cases remaining.

Tests cover the headline path (top-level reserved-keyword columns through COPY parquet/CSV/JSON, plus composite types with embedded quotes). I verified the following uncovered branches manually and they all work but I suggest we somehow add them to the tests, see below for suggestions

  • INTERVAL, STRUCT-of-INTERVAL, JSON read with reserved-keyword cols
  • TIMETZ→TIME cast on iceberg INSERT
  • ANALYZE on iceberg with reserved cols
  • Custom OPERATOR / function in a reserved-keyword schema
  • STRUCT field names with embedded (, ), , (and combined with ""-doubled ")
  • Iceberg overflow/native-conversion wrappers on reserved-name cols

And:

  • The only branches with neither a PR test nor my manual coverage are: geometry read/write to/from parquet/CSV/JSON/iceberg with reserved-named cols (ST_AsWKB, ST_AsGeoJSON, ST_GeomFromWKB/Text/GeoJSON), MAP-of-INTERVAL with a reserved-name col, and strptime with a custom timestampformat. All are 1-line quote_identifier → duckdb_quote_identifier swaps using the same pattern as the verified branches, so correctness follows by construction; would be nice to add at least one parameterized geometry round-trip test in pg_lake_spatial/tests/pytests/ to lock in regression protection.

The following has been my testing suite, only one failure

-- =============================================================================
-- PR #297 (pgguru/duckdb-quote-identifier) — identifier-quoting stress matrix
--
-- Ran ~155 SQL shapes against iceberg + Parquet/CSV foreign tables, mixing:
--   * PG-reserved          : where, select, union, order
--   * DuckDB-only-reserved : pivot, qualify, lambda, unpivot, summarize, describe
--   * DuckDB TYPE_FUNC_NAME: asof, anti, glob
--   * mixed-case           : MyCol
--   * embedded special     : 'weird name', 'col$with_dollar', 'col-with-hyphen',
--                            'col\nwith_newline', "123"
-- as schema names, table names, column names, struct field names, CTE/subquery
-- aliases, column-list aliases, function names, and type names.
--
-- After the 861c091 fix (set_config_option(quote_all_identifiers, on,
-- GUC_ACTION_SAVE) + the duckdb_quote_identifier upgrade to force-quote any
-- non-UNRESERVED_KEYWORD), the only remaining real defect is the silent " loss
-- in struct text VALUES — see Section 1.
-- =============================================================================


-- =============================================================================
-- Setup
-- =============================================================================

SET client_min_messages TO WARNING;
DROP SCHEMA IF EXISTS pivot   CASCADE;
DROP SCHEMA IF EXISTS qualify CASCADE;
DROP SCHEMA IF EXISTS "select" CASCADE;
DROP TABLE  IF EXISTS heap_kw CASCADE;

CREATE SCHEMA pivot;
CREATE SCHEMA qualify;
CREATE SCHEMA "select";

CREATE TYPE qualify.lambda_t AS (k int, t text);

CREATE TABLE pivot.t (
    id        int,
    "where"   int,
    unpivot   text,
    summarize int,
    s         qualify.lambda_t)
USING iceberg;

CREATE TABLE qualify.u (
    id            int,
    describe      text,
    "asof"        int,
    "MyCol"       text,
    "weird name"  text)
USING iceberg;

CREATE TABLE "select"."union" (
    id          int,
    pivot       int,
    lambda      int,
    "anti"      int,
    "glob"      int)
USING iceberg;

INSERT INTO pivot.t VALUES
  (1, 100, 'one',   11, ROW(1,'a')::qualify.lambda_t),
  (2, 200, 'two',   22, ROW(2,'b')::qualify.lambda_t),
  (3, 300, 'three', 33, ROW(3,'c')::qualify.lambda_t),
  (4, 400, 'four',  44, ROW(4,'d')::qualify.lambda_t);

INSERT INTO qualify.u VALUES
  (1, 'd1', 11, 'M1', 'n 1'),
  (2, 'd2', 22, 'M2', 'n 2'),
  (3, 'd3', 33, 'M3', 'n 3'),
  (5, 'd5', 55, 'M5', 'n 5');

INSERT INTO "select"."union" VALUES
  (1, 10, 100,  1, -1),
  (2, 20, 200,  2, -2),
  (3, 30, 300,  3, -3),
  (4, 40, 400,  4, -4);

CREATE TABLE heap_kw (id int, asof int, lambda int, "where" int, "anti" int, "glob" int);
INSERT INTO heap_kw VALUES (1,11,111,100,1,-1), (2,22,222,200,2,-2);


-- =============================================================================
-- SECTION 1 — STILL FAILING
-- =============================================================================

-- -----------------------------------------------------------------------------
-- F1.  Silent data loss: every `"` in a struct text VALUE is dropped on insert.
--
-- Same family of bug as the field-name escaping commits (b2608f9, ba5304e,
-- 861c091), just on the value-emitter side instead of the field-name side.
-- Lives in pg_lake_engine/src/pgduck/struct_conversion.c:158-172 — the
-- composite-value emitter uses PG composite-literal escape rules (`""`
-- doubling), but the output is then nested inside a DuckDB STRUCT literal
-- whose parser treats `""` as empty-string concatenation, dropping every `"`.
--
-- Single quotes, backslashes, commas, braces, colons, newlines all round-trip.
-- Only `"` is lost. Plain (non-struct) text columns are unaffected.
-- This is reachable from a normal user storing JSON / HTML / dialogue / code
-- inside a struct field, with no exotic identifier needed.
-- -----------------------------------------------------------------------------
DROP TABLE IF EXISTS val_t;
CREATE TABLE val_t (id int, s qualify.lambda_t) USING iceberg;

INSERT INTO val_t VALUES
  (1,  ROW(1,  'plain')                 ::qualify.lambda_t),  -- control: no special chars
  (2,  ROW(2,  E'has '' single')        ::qualify.lambda_t),  -- control: single quote OK
  (3,  ROW(3,  E'has " dquote')         ::qualify.lambda_t),  -- BAD: " dropped
  (4,  ROW(4,  E'has \\ slash')         ::qualify.lambda_t),  -- control: backslash OK
  (10, ROW(10, '{"k":1,"t":"x"}')       ::qualify.lambda_t),  -- BAD: all 6 " dropped
  (12, ROW(12, E'has " and '' both')    ::qualify.lambda_t),  -- BAD: " dropped, ' kept
  (302,ROW(302,'"""""""""""')           ::qualify.lambda_t);  -- BAD: all 11 " → ""

SELECT id, octet_length((s).t) AS got_len, (s).t
  FROM val_t
 WHERE id IN (1,2,3,4,10,12,302)
 ORDER BY id;

-- Observed output (silent corruption — no error, just shorter strings):
--
--   id  | got_len |       t
--  -----+---------+--------------------
--    1  |   5     | plain                ← OK
--    2  |  12     | has ' single         ← OK
--    3  |  11     | has  dquote          ← lost " (was 12 bytes)
--    4  |  11     | has \ slash          ← OK
--   10  |   9     | {k:1,t:x}            ← lost 6 " (was 15 bytes)
--   12  |  15     | has  and ' both      ← lost " (was 16 bytes)
--  302  |   0     |                      ← lost all 11 " (now empty string)

-- Same string in a plain text column round-trips fine — confirms struct-value
-- emitter is the culprit, not the underlying CSV pipeline:
DROP TABLE IF EXISTS rt_plain;
CREATE TABLE rt_plain (id int, got text) USING iceberg;
INSERT INTO rt_plain VALUES
  (1, E'has " dquote'),
  (2, '{"k":1,"t":"x"}');
SELECT id, octet_length(got) AS len, got FROM rt_plain ORDER BY id;
-- 1 | 12 | has " dquote
-- 2 | 15 | {"k":1,"t":"x"}


-- =============================================================================
-- SECTION 2 — PASSING
-- All shapes below worked end-to-end with reserved-keyword identifiers
-- everywhere they appear.
-- =============================================================================


-- ----- CTEs -----
WITH pivot AS (SELECT id, "where" FROM pivot.t WHERE "where" >= 200) SELECT count(*) FROM pivot;                                                              -- CTE name = reserved keyword
WITH p(id, asof) AS (SELECT id, "where" FROM pivot.t) SELECT id, asof FROM p ORDER BY id;                                                                     -- CTE column-list alias = reserved keyword
WITH a AS (SELECT id, "where" w FROM pivot.t), b AS (SELECT id, lambda FROM "select"."union") SELECT a.w, b.lambda FROM a JOIN b USING (id) ORDER BY a.w;     -- multi-CTE
WITH x AS (SELECT id, "where" AS pivot, lambda FROM pivot.t JOIN "select"."union" USING (id)) SELECT pivot, lambda FROM x ORDER BY pivot;                     -- CTE column aliased to reserved
WITH RECURSIVE r(level, id, val) AS (
  SELECT 0, id, "where" FROM pivot.t WHERE id = 1
  UNION ALL
  SELECT level+1, t.id, t."where" FROM pivot.t t JOIN r ON t.id = r.id+1 WHERE level < 3
) SELECT level, id, val FROM r ORDER BY level;                                                                                                                -- RECURSIVE CTE on reserved cols
WITH p AS (SELECT id, "where" w, summarize FROM pivot.t) SELECT a.id, a.w, b.summarize FROM p a JOIN p b ON a.id = b.id WHERE a.w > 100 ORDER BY a.id;        -- CTE referenced multiple times
WITH "select" AS (SELECT id, lambda FROM "select"."union") SELECT id, lambda FROM "select" ORDER BY id;                                                       -- CTE name shadows a real schema name
WITH t AS (SELECT id, "where" FROM pivot.t WHERE "where" > 100) SELECT * FROM t ORDER BY id;                                                                  -- CTE name shadows the source table name

-- ----- Window functions -----
SELECT id, "where", row_number() OVER (PARTITION BY unpivot ORDER BY "where") rn FROM pivot.t ORDER BY id;                                                    -- PARTITION BY / ORDER BY reserved cols
SELECT id, sum("where") OVER w FROM pivot.t WINDOW w AS (ORDER BY summarize) ORDER BY id;                                                                     -- named WINDOW with reserved column
SELECT id, sum("where") OVER (PARTITION BY summarize ORDER BY id ROWS BETWEEN 1 PRECEDING AND 1 FOLLOWING) FROM pivot.t ORDER BY id;                          -- explicit ROWS BETWEEN frame
SELECT id, "where", rank() OVER w FROM pivot.t WINDOW w AS (ORDER BY "where" DESC) ORDER BY id;                                                               -- rank() OVER named window

-- ----- Subqueries -----
SELECT id, "where", (SELECT max(lambda) FROM "select"."union") AS m FROM pivot.t ORDER BY id;                                                                 -- scalar subquery
SELECT id, (SELECT lambda FROM "select"."union" u WHERE u.id = t.id) AS l FROM pivot.t t ORDER BY id;                                                         -- correlated subquery
SELECT * FROM (SELECT id, "where" FROM pivot.t) qualify(asof, anti) ORDER BY asof;                                                                            -- subquery with reserved-keyword alias and column-list
SELECT id FROM pivot.t WHERE "where" IN (SELECT lambda FROM "select"."union") ORDER BY id;                                                                    -- WHERE x IN (SELECT ...)
SELECT id FROM pivot.t WHERE EXISTS (SELECT 1 FROM "select"."union" u WHERE u.id = pivot.t.id AND u."anti" > 0) ORDER BY id;                                  -- EXISTS
SELECT id FROM qualify.u WHERE NOT EXISTS (SELECT 1 FROM pivot.t t WHERE t."where" = qualify.u."asof");                                                       -- NOT EXISTS
SELECT id, (SELECT max("asof")::text FROM qualify.u WHERE id <= pivot.t.id) m FROM pivot.t ORDER BY id;                                                       -- scalar subquery + cast
SELECT id FROM (SELECT id FROM (SELECT id FROM (SELECT id FROM (SELECT id, "where" FROM pivot.t WHERE "where" > 100) qualify) pivot) lambda WHERE id > 1) glob ORDER BY id; -- 5-deep subqueries with reserved aliases at every level

-- ----- Set operations -----
SELECT id, "where" v FROM pivot.t UNION ALL SELECT id, lambda FROM "select"."union" ORDER BY id, v;                                                           -- UNION ALL
SELECT id FROM pivot.t UNION SELECT id FROM "select"."union" ORDER BY id;                                                                                     -- UNION (distinct)
SELECT id FROM pivot.t INTERSECT SELECT id FROM "select"."union" ORDER BY id;                                                                                 -- INTERSECT
SELECT id FROM pivot.t EXCEPT SELECT id FROM qualify.u ORDER BY id;                                                                                           -- EXCEPT
SELECT id, "where"::bigint v FROM pivot.t UNION ALL SELECT id, lambda::bigint FROM "select"."union" ORDER BY id, v LIMIT 5;                                   -- UNION ALL with cast columns

-- ----- Joins -----
SELECT t.id, t."where", x.lambda FROM pivot.t t, LATERAL (SELECT lambda FROM "select"."union" u WHERE u.id = t.id) x ORDER BY t.id;                            -- LATERAL
SELECT t.id, t."where", x.lambda FROM pivot.t t, LATERAL (SELECT lambda FROM "select"."union" u WHERE u.id = t.id AND u.lambda > t."where") x ORDER BY t.id;   -- LATERAL with arg passing
SELECT t.id, u."asof" FROM pivot.t t CROSS JOIN qualify.u u WHERE t.id = 1 AND u.id = 2;                                                                       -- CROSS JOIN
SELECT t.id ti, u.id ui, t."where", u."asof" FROM pivot.t t FULL OUTER JOIN qualify.u u ON t.id = u.id ORDER BY ti NULLS LAST, ui NULLS LAST;                  -- FULL OUTER JOIN
SELECT id FROM pivot.t JOIN "select"."union" USING (id) ORDER BY id;                                                                                           -- USING (reserved col)
SELECT t.id FROM pivot.t t JOIN "select"."union" u ON (t.s).k = u.id ORDER BY t.id;                                                                            -- JOIN ON struct-field-access
WITH x AS (SELECT id, "where" FROM pivot.t) SELECT a.id, a."where", b."where" b FROM x a JOIN x b ON a.id = b.id WHERE a."where" >= 200 ORDER BY a.id;         -- self-join via CTE

-- ----- Grouping -----
SELECT unpivot, summarize, count(*) FROM pivot.t GROUP BY GROUPING SETS ((unpivot),(summarize),()) ORDER BY 1 NULLS FIRST, 2 NULLS FIRST;                      -- GROUPING SETS
SELECT unpivot, summarize, count(*) FROM pivot.t GROUP BY CUBE (unpivot, summarize) ORDER BY 1 NULLS FIRST, 2 NULLS FIRST;                                     -- CUBE
SELECT unpivot, summarize, count(*) FROM pivot.t GROUP BY ROLLUP (unpivot, summarize) ORDER BY 1 NULLS FIRST, 2 NULLS FIRST;                                   -- ROLLUP
SELECT GROUPING(unpivot, summarize) g, count(*) FROM pivot.t GROUP BY ROLLUP (unpivot, summarize) ORDER BY 1;                                                  -- GROUPING(...)
SELECT count(*) FILTER (WHERE "where" > 150) FROM pivot.t;                                                                                                     -- FILTER WHERE on reserved col
SELECT unpivot, sum("where") s FROM pivot.t GROUP BY unpivot HAVING sum("where") > 150 ORDER BY unpivot;                                                       -- HAVING on reserved-col aggregate

-- ----- DISTINCT / ORDER BY -----
SELECT DISTINCT unpivot FROM pivot.t ORDER BY unpivot;                                                                                                         -- DISTINCT
SELECT DISTINCT ON (summarize) id, summarize, "where" FROM pivot.t ORDER BY summarize, "where" DESC;                                                           -- DISTINCT ON (reserved)
SELECT id FROM pivot.t ORDER BY abs("where") DESC;                                                                                                             -- ORDER BY function-call on reserved
SELECT id, "where" FROM pivot.t ORDER BY 2 DESC NULLS LAST, id;                                                                                                -- ORDER BY position + expression

-- ----- Casts / CASE / arrays -----
SELECT id, "where"::bigint FROM pivot.t WHERE "where"::text LIKE '2%' ORDER BY id;                                                                             -- cast on reserved col in projection and WHERE
SELECT id, CASE WHEN "where" > 200 THEN 'hi' WHEN "where" > 100 THEN 'mid' ELSE 'lo' END FROM pivot.t ORDER BY id;                                             -- CASE
SELECT array_agg("where" ORDER BY id) FROM pivot.t;                                                                                                            -- array_agg(reserved ORDER BY)
SELECT id, ARRAY["where", summarize] FROM pivot.t ORDER BY id;                                                                                                 -- ARRAY[] of reserved cols
SELECT id FROM pivot.t WHERE "where" = ANY (ARRAY[100,200,500]) ORDER BY id;                                                                                   -- ANY (ARRAY[...]) on reserved col
SELECT u FROM unnest((SELECT array_agg("where" ORDER BY id) FROM pivot.t)) u ORDER BY u;                                                                       -- array_agg then unnest

-- ----- Composite / struct types -----
SELECT id FROM pivot.t WHERE (s).k > 1 ORDER BY id;                                                                                                            -- struct field access in WHERE
SELECT (s).t, count(*) FROM pivot.t GROUP BY (s).t ORDER BY (s).t;                                                                                             -- struct field access in GROUP BY
SELECT id FROM pivot.t ORDER BY (s).k DESC;                                                                                                                    -- struct field access in ORDER BY
SELECT id, ROW(id, unpivot)::qualify.lambda_t AS r FROM pivot.t ORDER BY id;                                                                                   -- ROW(...)::reserved-schema-type
SELECT (s).k AS k, (s).t AS t FROM pivot.t WHERE (s).k > 1 ORDER BY k;                                                                                         -- deconstruct struct
SELECT (sub.s).k FROM (SELECT s FROM pivot.t WHERE id < 3) sub ORDER BY (sub.s).k;                                                                             -- struct from subquery

-- ----- Functions / operators -----
SELECT id, upper(unpivot), length(unpivot) FROM pivot.t ORDER BY id;                                                                                           -- pushable functions on reserved col
SELECT id, regexp_replace(unpivot, 'o', 'O') FROM pivot.t ORDER BY id;                                                                                         -- function-with-function-arg
SELECT coalesce(unpivot, 'NA') FROM pivot.t ORDER BY id;                                                                                                       -- coalesce
SELECT id, greatest("where", summarize*10) g FROM pivot.t ORDER BY id;                                                                                         -- greatest
SELECT id FROM pivot.t WHERE unpivot LIKE 't%' ORDER BY id;                                                                                                    -- LIKE
SELECT id FROM pivot.t WHERE unpivot ~ '^t' ORDER BY id;                                                                                                       -- regex
SELECT id, jsonb_build_object('pivot', "where", 'asof', summarize) FROM pivot.t ORDER BY id;                                                                   -- jsonb_build_object with reserved keys
SELECT id, row_to_json(t) FROM pivot.t t ORDER BY id;                                                                                                          -- row_to_json(reserved-col table)

-- ----- DML with FROM/USING + RETURNING -----
UPDATE pivot.t SET unpivot = u.describe FROM qualify.u u WHERE pivot.t.id = u.id;                                                                              -- UPDATE ... FROM
DELETE FROM "select"."union" u USING qualify.u q WHERE u.id = q.id AND q."asof" > 30;                                                                          -- DELETE ... USING
INSERT INTO "select"."union" VALUES (4,40,400,4,-4) ON CONFLICT DO NOTHING;                                                                                    -- restore row deleted above
INSERT INTO "select"."union" VALUES (10, 100, 1000, 10, -10) RETURNING id, pivot, lambda, "anti";                                                              -- INSERT RETURNING reserved cols
DELETE FROM "select"."union" WHERE id = 10;
UPDATE pivot.t SET "where" = "where" + 1 WHERE id = 1 RETURNING id, "where";                                                                                   -- UPDATE RETURNING reserved
UPDATE pivot.t SET "where" = "where" - 1 WHERE id = 1;
DELETE FROM "select"."union" WHERE id = 4 RETURNING id, pivot, lambda;                                                                                         -- DELETE RETURNING reserved
INSERT INTO "select"."union" VALUES (4,40,400,4,-4);
UPDATE pivot.t SET unpivot = (SELECT describe FROM qualify.u WHERE qualify.u.id = pivot.t.id) WHERE id IN (1,2,3);                                             -- UPDATE with subquery in SET
DELETE FROM "select"."union" WHERE id IN (SELECT id FROM qualify.u WHERE "asof" >= 50);                                                                        -- DELETE with IN-subquery
UPDATE pivot.t SET s = ROW((s).k+10, (s).t || '!')::qualify.lambda_t WHERE id = 1;                                                                             -- UPDATE struct field
INSERT INTO "select"."union" VALUES (1,99,99,99,99) ON CONFLICT DO NOTHING;                                                                                    -- INSERT ON CONFLICT (no constraint, so still inserts)

-- ----- VALUES & SRF in FROM -----
SELECT v.id, t."where" FROM (VALUES (1,'a'),(2,'b')) v(id,nm) JOIN pivot.t t ON v.id = t.id;                                                                   -- VALUES in FROM
WITH wanted(id) AS (VALUES (1),(2)) SELECT t.id, t."where" FROM pivot.t t JOIN wanted USING (id) ORDER BY t.id;                                                -- VALUES in CTE
SELECT u.elem, u.ord FROM pivot.t, unnest(ARRAY["where",summarize]) WITH ORDINALITY AS u(elem,ord) WHERE pivot.t.id = 1;                                       -- unnest WITH ORDINALITY of reserved cols

-- ----- DDL / schema-evolution -----
DROP VIEW IF EXISTS v_pivot CASCADE;
CREATE VIEW v_pivot AS SELECT id, "where", summarize FROM pivot.t WHERE "where" >= 200;                                                                        -- CREATE VIEW over iceberg with reserved cols
SELECT * FROM v_pivot ORDER BY id;
DROP TABLE IF EXISTS trunc_t;
CREATE TABLE trunc_t (id int, "asof" int) USING iceberg;
INSERT INTO trunc_t VALUES (1,1),(2,2);
TRUNCATE trunc_t;                                                                                                                                              -- TRUNCATE iceberg with reserved col
SELECT count(*) FROM trunc_t;
DROP TABLE IF EXISTS alter_t;
CREATE TABLE alter_t (id int) USING iceberg;
ALTER TABLE alter_t ADD COLUMN pivot int;                                                                                                                      -- ALTER TABLE ADD reserved column
INSERT INTO alter_t VALUES (1, 100);
ALTER TABLE alter_t RENAME COLUMN pivot TO "asof";                                                                                                             -- ALTER TABLE RENAME to reserved
INSERT INTO alter_t VALUES (2, 200);
SELECT * FROM alter_t ORDER BY id;
DROP TABLE IF EXISTS gen_t;
CREATE TABLE gen_t (id int, asof int, computed int GENERATED ALWAYS AS (asof * 2) STORED) USING iceberg;                                                       -- generated column referring to reserved col
INSERT INTO gen_t (id, asof) VALUES (1, 100);
SELECT * FROM gen_t;
DROP TABLE IF EXISTS def_t;
CREATE TABLE def_t (id int, asof timestamptz DEFAULT now()) USING iceberg;                                                                                     -- DEFAULT expression on reserved col
INSERT INTO def_t (id) VALUES (1), (2);
SELECT id, asof IS NOT NULL FROM def_t ORDER BY id;
DROP TABLE IF EXISTS chk_t;
CREATE TABLE chk_t (id int, "where" int CHECK ("where" > 0)) USING iceberg;                                                                                    -- CHECK constraint on reserved col
INSERT INTO chk_t VALUES (1, 100);
SELECT * FROM chk_t;

-- ----- Identifier corners -----
DROP TABLE IF EXISTS long_t;
CREATE TABLE long_t (id int, lambda_qualify_pivot_summarize_unpivot_describe_show_anti_glob_x int) USING iceberg;                                              -- 60+-char identifier
INSERT INTO long_t VALUES (1, 999);
SELECT * FROM long_t;
DROP TABLE IF EXISTS dollar_t;
CREATE TABLE dollar_t (id int, "col$with_dollar" int) USING iceberg;                                                                                           -- $ in identifier
INSERT INTO dollar_t VALUES (1,10);
SELECT "col$with_dollar" FROM dollar_t;
DROP TABLE IF EXISTS hyphen_t;
CREATE TABLE hyphen_t (id int, "col-with-hyphen" int) USING iceberg;                                                                                           -- hyphen in identifier
INSERT INTO hyphen_t VALUES (1,10);
SELECT "col-with-hyphen" FROM hyphen_t;
DROP TABLE IF EXISTS num_t;
CREATE TABLE num_t (id int, "123" int) USING iceberg;                                                                                                          -- all-digits quoted identifier
INSERT INTO num_t VALUES (1,10);
SELECT "123" FROM num_t;

-- ----- Custom domain / type / function named after reserved word -----
DROP DOMAIN IF EXISTS "asof_d" CASCADE;
CREATE DOMAIN "asof_d" AS int CHECK (VALUE > 0);                                                                                                               -- domain named after reserved keyword
DROP TABLE IF EXISTS dom_t;
CREATE TABLE dom_t (id int, "asof" "asof_d") USING iceberg;
INSERT INTO dom_t VALUES (1, 100);
SELECT id FROM dom_t WHERE "asof" = 100::"asof_d";                                                                                                             -- cast-to-reserved-domain in WHERE

DROP TYPE IF EXISTS public.pivot CASCADE;
CREATE TYPE public.pivot AS (k int, v text);                                                                                                                   -- composite TYPE named after reserved keyword
DROP TABLE IF EXISTS tn_t;
CREATE TABLE tn_t (id int, p public.pivot) USING iceberg;
INSERT INTO tn_t VALUES (1, ROW(7,'x')::public.pivot);
SELECT id, (p).k, (p).v FROM tn_t;
SELECT (id, unpivot)::public.pivot AS p FROM pivot.t WHERE id = 2;                                                                                             -- cast row to reserved-name composite type

DROP FUNCTION IF EXISTS public."pivot"(int);
CREATE FUNCTION public."pivot"(int) RETURNS int LANGUAGE sql AS 'SELECT $1 * 2';                                                                               -- function name = reserved keyword
SELECT id, public."pivot"("where") FROM pivot.t ORDER BY id;

-- ----- Catalog / introspection -----
SELECT column_name FROM information_schema.columns WHERE table_schema='pivot' AND table_name='t' ORDER BY ordinal_position;                                    -- information_schema enumerates reserved cols
SELECT attname FROM pg_attribute WHERE attrelid='pivot.t'::regclass AND attnum > 0 ORDER BY attnum;                                                            -- pg_attribute lookup

-- ----- Cross-format reads (parquet + CSV foreign tables) -----
DROP FOREIGN TABLE IF EXISTS pq_kw  CASCADE;
DROP FOREIGN TABLE IF EXISTS csv_kw CASCADE;
COPY (SELECT id, asof, lambda, "where" FROM heap_kw) TO 's3://testbucketpglake/review_pr297_share/pq_kw/data.parquet' WITH (FORMAT 'parquet');
COPY (SELECT id, asof, lambda, "where" FROM heap_kw) TO 's3://testbucketpglake/review_pr297_share/csv_kw/data.csv'    WITH (FORMAT 'csv', HEADER 'true');
CREATE FOREIGN TABLE pq_kw  () SERVER pg_lake OPTIONS (path 's3://testbucketpglake/review_pr297_share/pq_kw/data.parquet');
CREATE FOREIGN TABLE csv_kw () SERVER pg_lake OPTIONS (path 's3://testbucketpglake/review_pr297_share/csv_kw/data.csv', header 'true');
SELECT id, asof, lambda, "where" FROM pq_kw  ORDER BY id;                                                                                                      -- parquet foreign table with reserved cols
SELECT id FROM pq_kw WHERE asof > 15 AND "where" < 250 ORDER BY id;                                                                                            -- WHERE on reserved cols, parquet
SELECT id, asof, lambda, "where" FROM csv_kw ORDER BY id;                                                                                                      -- CSV foreign table with reserved cols
SELECT count(*), max(asof) FROM csv_kw WHERE lambda > 100;                                                                                                     -- aggregate on reserved cols, CSV
SELECT p.id, p.asof, t."where" FROM pq_kw p JOIN pivot.t t USING (id) ORDER BY p.id;                                                                           -- parquet ⨝ iceberg join

-- ----- Pipeline shapes -----
DROP TABLE IF EXISTS sink_t;
CREATE TABLE sink_t (id int, w int, l int) USING iceberg;
INSERT INTO sink_t SELECT t.id, t."where", u.lambda FROM pivot.t t JOIN "select"."union" u USING (id);                                                         -- INSERT … SELECT cross-iceberg with reserved cols
SELECT count(*) FROM sink_t;
DROP TABLE IF EXISTS ctas_t;
CREATE TABLE ctas_t USING iceberg AS SELECT id, "where" pivot, summarize asof FROM pivot.t WHERE "where" >= 200;                                               -- CTAS with reserved-aliased projection
SELECT pivot, asof FROM ctas_t ORDER BY pivot;

DROP TABLE IF EXISTS cp_t;
CREATE TABLE cp_t (id int, "asof" int, lambda text) USING iceberg;
COPY cp_t FROM STDIN WITH (FORMAT csv);                                                                                                                        -- COPY iceberg FROM STDIN with reserved cols
1,11,a
2,22,b
3,33,c
\.
SELECT * FROM cp_t ORDER BY id;
COPY (SELECT id, "asof", lambda FROM cp_t ORDER BY id) TO STDOUT WITH (FORMAT csv, HEADER true);                                                               -- COPY (SELECT … reserved …) TO STDOUT

-- ----- Mega-shape (CTE + JOIN + window + aggregate + reserved cols + grouping) -----
WITH base AS (
  SELECT t.id, t."where" w, t.summarize, u.lambda l
  FROM pivot.t t JOIN "select"."union" u USING (id)
)
SELECT id, w,
       sum(l) OVER (PARTITION BY summarize ORDER BY id) cum_l,
       count(*) OVER ()                                  total
  FROM base
 ORDER BY id;                                                                                                                                                  -- everything together

WITH a AS (SELECT id, "where" w, lambda FROM pivot.t JOIN "select"."union" USING (id))
SELECT a.id, a.w, sum(a.lambda) OVER (PARTITION BY a.id) s, GROUPING(a.id) g
  FROM a
 GROUP BY ROLLUP (a.id, a.w, a.lambda)
 ORDER BY a.id NULLS FIRST, a.w NULLS FIRST;                                                                                                                    -- ROLLUP + window + reserved cols

-- ----- EXPLAIN ANALYZE on every DML -----
EXPLAIN (ANALYZE, BUFFERS) SELECT id, "where" FROM pivot.t WHERE summarize > 20;                                                                               -- EXPLAIN ANALYZE SELECT
EXPLAIN ANALYZE INSERT INTO "select"."union" VALUES (99,1,2,3,4); DELETE FROM "select"."union" WHERE id = 99;                                                  -- EXPLAIN ANALYZE INSERT
EXPLAIN ANALYZE UPDATE pivot.t SET "where" = "where" + 0 WHERE id = 1;                                                                                         -- EXPLAIN ANALYZE UPDATE
BEGIN; EXPLAIN ANALYZE DELETE FROM "select"."union" WHERE id = 1; ROLLBACK;                                                                                    -- EXPLAIN ANALYZE DELETE


-- =============================================================================
-- SUMMARY
--
-- ~150 deparser-driven shapes pass after 861c091. The
-- set_config_option(quote_all_identifiers, on, GUC_ACTION_SAVE) +
-- AtEOXact_GUC approach is comprehensive on the deparser path, and the
-- duckdb_quote_identifier upgrade correctly catches DuckDB-only-reserved
-- and TYPE_FUNC_NAME identifiers (asof, anti, glob).
--
-- Only F1 (silent " loss in struct text VALUES) remains, and it is the
-- same family of escaping bug as the field-name commits in this PR.
-- =============================================================================

#include "pg_lake/parsetree/options.h"
#include "pg_lake/parquet/field.h"
#include "pg_lake/pgduck/gdal.h"
#include "pg_lake/pgduck/keywords.h"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes touch INTERVAL, struct-of-INTERVAL, MAP-of-INTERVAL, geometry, and strptime branches that aren't directly covered by the new copy tests. I verified the INTERVAL and struct-of-INTERVAL paths work manually; could the existing parameterized parquet/csv test be extended to also include lambda interval, pivot timestamp, and one geometry column to lock in regression protection?

Comment thread pg_lake_engine/src/pgduck/parse_struct.c
Comment thread pg_lake_engine/src/pgduck/read_data.c
Comment thread pg_lake_engine/src/pgduck/read_data.c
GetOrCreatePGStructType was psprintf-ing composite field names directly
into a '{...}'::name[] literal.  Hostile shapes (commas, colons, quotes,
spaces, backslashes, parens in field names) produced malformed array
literals and a confusing SPI error on auto-detect.

Switch to SPI_execute_with_args with the names and oids passed as
properly constructed name[]/oid[] arrays.  Adds a regression test
covering an auto-detect path through CREATE FOREIGN TABLE () against a
parquet file whose struct fields contain each hostile character class.

Addresses review feedback on PR #297.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
TupleDescToStructProjection (read_data.c) was emitting struct field
keys with bare "'%s':" formatting, which truncated on the first
embedded "'" and broke INTERVAL / struct-of-INTERVAL projections for
composite types whose field names contain a single quote.

- Expose QuoteDuckDBStructKey via the existing struct_conversion.h
  (was static to struct_conversion.c where the sibling emit site
  already used it) and route the read_data.c call site through it.
- Add a regression test for a struct-of-INTERVAL with a "'"-bearing
  field name, plus a small parameterized test covering the INTERVAL /
  TIMESTAMP / INTERVAL[] branches in the struct projection builder
  with reserved-keyword field names, closing the coverage gap the
  reviewer flagged on read_data.c.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
Signed-off-by: David Christensen <david.christensen@snowflake.com>
PR #297's existing reserved-keyword tests only cover int columns, which
doesn't exercise the geometry branches in pg_lake_engine's read/write
paths.  Add two tests in pg_lake_spatial/tests/pytests/test_spatial_basics.py
that lock those in:

- test_reserved_keyword_geometry_column: writable iceberg table with a
  "pivot" geometry column, covering both projection and filter-pushdown
  (deparse.c) with duckdb_quote_identifier applied to geometry columns.
- test_reserved_keyword_struct_of_geometry: composite column with a
  "lambda" geometry sub-field, exercising the struct projection builder
  in read_data.c for geometry fields alongside the INTERVAL /
  struct-of-INTERVAL / INTERVAL[] cases already covered in
  pg_lake_table's test_reserved_keyword_struct_field_typed.

Addresses review feedback on PR #297 (read_data.c:31).

Signed-off-by: David Christensen <david.christensen@snowflake.com>
The new call site in read_data.c:987 is a struct literal concatenated
directly into the SQL text sent to DuckDB, not a CSV value.  DuckDB's
parser expects SQL-standard '' doubling inside single-quoted string
literals in that context — the existing QuoteDuckDBStructKey helper
uses backslash escaping (\\' / \\\\) which is correct for the CSV value
context it was designed for but produces a parser error when the
resulting string appears directly in a SELECT.

Add a second helper QuoteDuckDBStructKeySQL that uses '' doubling and
route the read_data.c emit through it.  Keep the original helper for
the struct_conversion.c CSV-emit path which still needs backslash
escaping to round-trip through the outer CSV layer.

Also drop the spatial struct-of-geometry test: composite columns
containing geometry aren't supported by pg_lake_iceberg, so the test
can't land on an iceberg foreign table.  The sibling
test_reserved_keyword_geometry_column still covers the geometry read
path on iceberg, and the non-geometry struct branches
(INTERVAL/TIMESTAMP/INTERVAL[]) are covered by
test_reserved_keyword_struct_field_typed.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
Extend the reserved-keyword regression suite to cover the remaining
emission branches identified in PR #297 review 4270327414:

pg_lake_table/tests/pytests/test_duckdb_reserved_keywords.py:
- test_iceberg_timetz_to_time_cast_on_reserved_column —
  write_data.c:TupleDescToProjectionListForWrite CAST(... AS TIME)
  branch with reserved-keyword column name.
- test_iceberg_analyze_with_reserved_columns — ANALYZE on iceberg FDW.
- test_reserved_keyword_schema_with_custom_function_and_operator —
  schema-qualified deparse with reserved-keyword schema name.
- test_iceberg_overflow_conversion_on_reserved_column — type-widening
  CAST projection path with reserved-name column.

pg_lake_spatial/tests/pytests/test_spatial_basics.py:
- test_reserved_keyword_geometry_roundtrip[parquet|csv|json] —
  parameterized round-trip exercising the
  ST_AsWKB / ST_AsGeoJSON / ST_GeomFromWKB/Text/GeoJSON wrapper branches
  in write_data.c / read_data.c with duckdb_quote_identifier applied to
  the column name.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
Per review feedback, the remaining two "by construction" branches are
cheap to cover in tests:

- test_iceberg_map_of_interval_with_reserved_column: MAP-of-INTERVAL
  column named "pivot" on an iceberg table, exercising the map+interval
  encoding path with duckdb_quote_identifier.
- test_s3log_strptime_with_reserved_column_name: format='log' /
  log_format='s3' foreign table, driving the strptime() wrapper branch
  in read_data.c for timestamp columns and confirming its emission via
  assert_remote_query_contains_expression.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
- test_iceberg_map_of_interval_with_reserved_column: add
  with_default_location fixture so the USING iceberg table has a
  location (the table doesn't specify one inline).
- test_s3log_strptime_with_reserved_column_name: rollback at start
  so a prior test's failed txn doesn't cascade.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
The strptime() call emitted from read_data.c's LOG-format branch doesn't
survive to the top-level EXPLAIN remote SQL string as an identifiable
substring.  A successful round-trip query through the log table is
sufficient to show the path executed with the column name correctly
quoted.

Signed-off-by: David Christensen <david.christensen@snowflake.com>
@sfc-gh-dachristensen
sfc-gh-dachristensen merged commit 663413b into main May 12, 2026
64 checks passed
@sfc-gh-dachristensen
sfc-gh-dachristensen deleted the pgguru/duckdb-quote-identifier branch May 12, 2026 21:14
sfc-gh-dachristensen added a commit that referenced this pull request May 12, 2026
…#297) (#348)

## Summary

Harden the pg_lake → pgduck_server SQL emission paths against identifier quoting and struct field-name escaping mismatches between PostgreSQL and DuckDB.

### Identifier quoting

  - Add `duckdb_quote_identifier()` that quotes identifiers reserved in DuckDB but not in PostgreSQL — both RESERVED_KEYWORD (`pivot`, `qualify`, `lambda`, `summarize`,
  from DuckDB's vendored `kwlist.hpp` via `tools/generate_duckdb_kwlist.py`. CI now runs `make check-duckdb-kwlist` to catch a stale table after a DuckDB bump; the script
  falls back to fetching the file from `raw.githubusercontent.com` at the pinned submodule SHA so CI doesn't need the submodule initialised.
  - Route every relation / schema / column / function / operator-namespace name through `duckdb_quote_identifier()` in both the ruleutils deparse path
    (`deparse_ruleutils.c`) and the postgres_fdw-style deparser (`deparse.c`). Fixes `pivot.t` style schema-qualified names appearing unquoted in `EXPLAIN (VERBOSE)` output.
  - `deparse_ruleutils.c` uses `set_config_option("quote_all_identifiers", "on", …, GUC_ACTION_SAVE, …)` so `AtEOXact_GUC` unwinds the GUC stack even if `pg_get_querydef`
  throws.

  ### STRUCT field-name handling

  - `QuoteDuckDBFieldName` emits SQL-standard `""` doubling (what DuckDB and SQL both expect) instead of C-style `\"` backslash escaping.
  - `ParseDuckDBFieldName` rewritten to consume SQL-standard `""` doubling; the paren-depth scanner in `ParseDuckDBFieldType` skips over quoted regions so embedded `(` /
  `)` in field names don't break struct parsing.
  - `StructOutForPGDuck` uses DuckDB-compatible backslash escaping for struct literal **values** travelling through CSV, because that context needs to round-trip through
  the outer CSV layer.
  - Two helpers exposed via `struct_conversion.h`:
    - `QuoteDuckDBStructKey` (backslash escaping) for struct literals emitted as CSV values — the `struct_conversion.c:StructOutForPGDuck` path.
    - `QuoteDuckDBStructKeySQL` (SQL-standard `''` doubling) for struct literals concatenated directly into SQL query text — the `read_data.c:TupleDescToStructProjection`
  path, including the INTERVAL / struct-of-INTERVAL / INTERVAL[] emit branches. Previously used bare `'%s':` which truncated on any field name containing `'`.

  ### Composite-type catalog lookup

  - `GetOrCreatePGStructType` previously `psprintf`'d composite field names directly into a `'{...}'::name[]` array literal, producing "malformed array literal" for field
  names containing `,`, `:`, quotes, spaces, parens, or backslashes during auto-detect. Switched to `SPI_execute_with_args` with the names and oids passed as proper
  `name[]` / `oid[]` Datums constructed via `construct_array_builtin`.

  ### CI / infrastructure

  - Artifact name for `postgres-logs` upload now includes `worker_id` (when the matrix supplies one) and `from_pg_version` (for the upgrade matrix), so parallel matrix
  jobs stop colliding on the same artifact name.
  - Lint workflow runs `make check-duckdb-kwlist`.

  ## Test plan

  ### Reserved-keyword column names
  - `test_{read,read_csv,read_json,writable_parquet,iceberg_table}_with_reserved_keyword_columns`
  - `test_each_reserved_keyword_{parquet,csv,json}[kw]` — parameterized across every DuckDB-only reserved keyword; covers SELECT + WHERE pushdown + EXPLAIN-quoted
  verification
  - `test_iceberg_table_with_reserved_keyword_columns`, `test_iceberg_table_insert_select_all_reserved_keywords`,
  `test_readable_iceberg_foreign_table_with_reserved_keyword_columns`
  - `test_copy_roundtrip_{parquet,csv,json}[kw]` (in `pg_lake_copy`)

  ### Typed struct fields with reserved-keyword names
  - `test_reserved_keyword_struct_field_typed[interval|timestamp|interval[]]` — INTERVAL / TIMESTAMP / INTERVAL[] emit branches in `read_data.c`
  - `test_reserved_keyword_geometry_column` — iceberg FDW with a `pivot geometry` column; SELECT + filter pushdown
  - `test_reserved_keyword_geometry_roundtrip[parquet|csv|json]` — parameterized round-trip exercising `ST_AsWKB` / `ST_AsGeoJSON` / `ST_GeomFromWKB` / `ST_GeomFromText` /
   `ST_GeomFromGeoJSON` with reserved column names
  - `test_iceberg_map_of_interval_with_reserved_column` — MAP-of-INTERVAL column named `pivot`
  - `test_iceberg_timetz_to_time_cast_on_reserved_column` — `write_data.c` TIMETZ→TIME CAST branch
  - `test_iceberg_overflow_conversion_on_reserved_column` — type-widening CAST projection path
  - `test_s3log_strptime_with_reserved_column_name` — LOG-format foreign table hitting the `strptime()` wrapper branch

  ### STRUCT field-name edge cases
  - `test_iceberg_composite_field_with_special_characters` — spaces, `"`, `'`, `\` in field names
  - `test_iceberg_composite_field_with_embedded_double_quote` — `U&"has\0022quote"`
  - `test_struct_of_interval_field_name_with_single_quote` — struct-of-INTERVAL with a `'`-bearing field name
  - `test_autodetect_struct_field_names_with_hostile_characters` — `CREATE FOREIGN TABLE () SERVER pg_lake OPTIONS (path …)` against a parquet file whose struct fields
  have commas, colons, quotes, spaces, parens, backslashes (covers the SPI parameter-binding fix)
  - `test_copy_roundtrip_composite_with_embedded_quote`, `test_copy_roundtrip_csv_composite_with_embedded_quote` — parquet/CSV round-trips

  ### Other pg_lake_engine paths
  - `test_iceberg_analyze_with_reserved_columns` — ANALYZE on iceberg FDW with reserved-keyword columns
  - `test_reserved_keyword_schema_with_custom_function_and_operator` — schema-qualified function/operator deparse with a reserved schema name

(cherry picked from commit 663413b)

Signed-off-by: David Christensen <david.christensen@snowflake.com>
@sfc-gh-mslot sfc-gh-mslot mentioned this pull request Jun 7, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants