diff --git a/pg_lake_copy/tests/pytests/test_csv_copy.py b/pg_lake_copy/tests/pytests/test_csv_copy.py index 35044d45..c4c41ee5 100644 --- a/pg_lake_copy/tests/pytests/test_csv_copy.py +++ b/pg_lake_copy/tests/pytests/test_csv_copy.py @@ -1074,3 +1074,49 @@ def test_auto_detect(pg_conn, azure): # assert result_before == result_after pg_conn.rollback() + + +def test_copy_to_multidim_array_csv(pg_conn, s3): + """ + Regression test for https://github.com/Snowflake-Labs/pg_lake/issues/407. + + Unlike Parquet/JSON, COPY TO in CSV format must still ACCEPT + multidimensional arrays. For CSV, ShouldUseDuckSerialization is false and + ChooseDuckDBEngineTypeForWrite treats every column as VARCHAR, so the value + is written using the PostgreSQL text representation ("{{1,2},{3,4}}") and + copied through verbatim rather than cast to a DuckDB LIST(T). The multidim + guard in CopyOneRowTo must therefore NOT fire for CSV, otherwise a value + that writes fine today would start erroring. + + (Local-file / STDOUT CSV is handled by PostgreSQL directly and never + reaches pg_lake, so we exercise the pg_lake CSV path via an object-store + URL.) + """ + csv_key = "test_copy_to_multidim_array_csv/data.csv" + csv_path = f"s3://{TEST_BUCKET}/{csv_key}" + + # Must not raise: the guard is scoped to DuckDB-serialised formats. + run_command( + f""" + CREATE TABLE test_multidim_csv (id bigint, v int[]); + INSERT INTO test_multidim_csv VALUES + (1, ARRAY[[1,2],[3,4]]), + (2, ARRAY[10,20,30]), + (3, NULL); + COPY test_multidim_csv TO '{csv_path}' WITH (format 'csv'); + """, + pg_conn, + ) + + # The multidimensional value is preserved verbatim as PostgreSQL array + # text (the field is quoted because it contains commas, but the literal is + # intact), and the 1-D array is written alongside it. + content = s3.get_object(Bucket=TEST_BUCKET, Key=csv_key)["Body"].read().decode() + assert ( + "{{1,2},{3,4}}" in content + ), f"multidimensional array text missing from CSV output: {content!r}" + assert ( + "{10,20,30}" in content + ), f"1-D array text missing from CSV output: {content!r}" + + pg_conn.rollback() diff --git a/pg_lake_copy/tests/pytests/test_parquet_copy.py b/pg_lake_copy/tests/pytests/test_parquet_copy.py index 48be82fe..6596bbaf 100644 --- a/pg_lake_copy/tests/pytests/test_parquet_copy.py +++ b/pg_lake_copy/tests/pytests/test_parquet_copy.py @@ -884,6 +884,108 @@ def test_md_array(pg_conn, duckdb_conn, tmp_path): assert result[0]["val"] == 2 +def test_copy_to_multidim_array_errors(pg_conn, tmp_path): + """ + Regression test for https://github.com/Snowflake-Labs/pg_lake/issues/407. + + COPY TO must raise a clear pg_lake error when a column + contains a multidimensional array value. Previously the value was + serialised into the intermediate temp CSV and handed to DuckDB, which + either returned a cryptic Conversion Error or crashed the shared + pgduck_server process (issue #408). + + The check lives in CopyOneRowTo (csv_writer.c) and fires before the CSV + is written, so the engine is never involved. + """ + parquet_path = tmp_path / "test_multidim_err.parquet" + + run_command( + """ + CREATE TABLE test_multidim_err (id bigint, v int[]); + INSERT INTO test_multidim_err VALUES (1, ARRAY[[1,2],[3,4]]); + """, + pg_conn, + ) + + error = run_command( + f"COPY test_multidim_err TO '{parquet_path}' WITH (format 'parquet')", + pg_conn, + raise_error=False, + ) + + assert error is not None, "Expected an error for multidimensional array in COPY TO" + assert ( + "multidimensional arrays are not supported" in error.lower() + ), f"Unexpected error message: {error}" + + pg_conn.rollback() + + +def test_copy_to_1d_array_succeeds(pg_conn, duckdb_conn, tmp_path): + """ + Regression guard: 1-D array columns must still round-trip correctly through + COPY TO after the multidim check is added. A 1-D value has ARR_NDIM == 1 + and must not be rejected. + """ + parquet_path = tmp_path / "test_1d_array.parquet" + + run_command( + f""" + CREATE TABLE test_1d_array (id bigint, tags text[]); + INSERT INTO test_1d_array VALUES + (1, ARRAY['a', 'b', 'c']), + (2, NULL), + (3, ARRAY['x']); + COPY test_1d_array TO '{parquet_path}' WITH (format 'parquet'); + """, + pg_conn, + ) + + duckdb_conn.execute( + "SELECT id, tags FROM read_parquet($1) ORDER BY id", [str(parquet_path)] + ) + rows = duckdb_conn.fetchall() + + assert len(rows) == 3 + assert rows[0] == (1, ["a", "b", "c"]) + assert rows[1] == (2, None) + assert rows[2] == (3, ["x"]) + + pg_conn.rollback() + + +def test_copy_to_multidim_array_json_errors(pg_conn, tmp_path): + """ + Companion to the CSV case: JSON serialises arrays through DuckDB as a + typed LIST(T) (ShouldUseDuckSerialization is true), so a multidimensional + value must still be rejected by the guard in CopyOneRowTo. + """ + json_path = tmp_path / "test_multidim.json" + + run_command( + """ + CREATE TABLE test_multidim_json (id bigint, v int[]); + INSERT INTO test_multidim_json VALUES (1, ARRAY[[1,2],[3,4]]); + """, + pg_conn, + ) + + error = run_command( + f"COPY test_multidim_json TO '{json_path}' WITH (format 'json')", + pg_conn, + raise_error=False, + ) + + assert ( + error is not None + ), "Expected an error for multidimensional array in COPY TO json" + assert ( + "multidimensional arrays are not supported" in error.lower() + ), f"Unexpected error message: {error}" + + pg_conn.rollback() + + def test_copy_virtual_column(pg_conn, tmp_path): # virtual columns were introduced in PostgreSQL 18 if get_pg_version_num(pg_conn) < 180000: diff --git a/pg_lake_engine/src/csv/csv_writer.c b/pg_lake_engine/src/csv/csv_writer.c index 3b91870f..766036a0 100644 --- a/pg_lake_engine/src/csv/csv_writer.c +++ b/pg_lake_engine/src/csv/csv_writer.c @@ -43,6 +43,7 @@ #include "nodes/execnodes.h" #include "nodes/makefuncs.h" #include "port/pg_bswap.h" +#include "utils/array.h" #include "utils/builtins.h" #include "utils/lsyscache.h" #include "utils/memutils.h" @@ -770,7 +771,55 @@ CopyOneRowTo(CopyToState cstate, TupleTableSlot *slot) */ Form_pg_attribute attr = TupleDescAttr(slot->tts_tupleDescriptor, attnum - 1); - if (ShouldUseDuckSerialization(cstate->targetFormat, MakePGType(attr->atttypid, attr->atttypmod))) + bool useDuckSerialization = + ShouldUseDuckSerialization(cstate->targetFormat, + MakePGType(attr->atttypid, attr->atttypmod)); + + /* + * Reject multidimensional arrays before DuckDB serialization. + * PostgreSQL cannot distinguish int[] from int[][] at the + * type level, so a value with ndim > 1 would be serialised as + * "[[1,2],[3,4]]" and DuckDB cannot cast that string back to + * a flat LIST(T). Check before serialisation so we do not + * pay the cost of PGDuckSerialize on a value we will reject. + * + * This only fires for formats that hand the value to DuckDB + * as a typed LIST(T) (Parquet/Iceberg/JSON), i.e. exactly the + * formats for which ShouldUseDuckSerialization returns true + * for an array. CSV is deliberately exempt: it preserves the + * PostgreSQL text representation ("{{1,2},{3,4}}") and reads + * every column back as VARCHAR, so a multidimensional value + * round-trips faithfully and must not be rejected (#407). + * + * For the Iceberg write path (INSERT into an Iceberg table) a + * multidimensional value is already rejected (error policy) + * or set to NULL (clamp policy) upstream by + * IcebergErrorOrClampDatum, so it never reaches here with + * ndim > 1. COPY TO in Iceberg format is rejected earlier in + * EnsureFormatSupported. In practice this only fires on + * plain COPY TO to a Parquet or JSON file. + */ + if (useDuckSerialization && + get_element_type(attr->atttypid) != InvalidOid) + { + ArrayType *arr = DatumGetArrayTypeP(value); + + if (ARR_NDIM(arr) > 1) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("multidimensional arrays are not supported" + " in COPY TO"), + errdetail("Column \"%s\" contains a" + " %d-dimensional array value.", + NameStr(attr->attname), + ARR_NDIM(arr)), + errhint("Flatten the array to one dimension before" + " exporting, write to CSV format, or write" + " to an Iceberg table with" + " out_of_range_values = 'clamp'."))); + } + + if (useDuckSerialization) { /* * Since we are at the top-level when emitting an diff --git a/pg_lake_engine/src/pgduck/write_data.c b/pg_lake_engine/src/pgduck/write_data.c index ff333a9b..3cb49211 100644 --- a/pg_lake_engine/src/pgduck/write_data.c +++ b/pg_lake_engine/src/pgduck/write_data.c @@ -93,8 +93,15 @@ ConvertCSVFileTo(char *csvFilePath, TupleDesc csvTupleDesc, int maxLineSize, bool queryHasRowIds = false; /* - * CSV data is already clamped by WriteInsertRecord and converted to - * struct for Iceberg + * When reached from the Iceberg FDW INSERT path, the CSV data has already + * been clamped by WriteInsertRecord (via ClampAndCheckConstraints → + * IcebergErrorOrClampSlotInPlace). When reached from the plain COPY TO + * path (ProcessPgLakeCopyTo) with a DuckDB-serialised destination + * (Parquet/JSON), multidimensional array values are rejected earlier in + * CopyOneRowTo, so they never appear in the CSV. For a CSV destination + * such values may appear, but every column is read back as VARCHAR (see + * ChooseDuckDBEngineTypeForWrite), so the PostgreSQL array text is copied + * through verbatim rather than cast to a LIST(T). */ return WriteQueryResultTo(command.data, destinationPath,