From 2621e1bb474e3dffd6e22848ef2c93cde2e82288 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 22 Oct 2026 18:40:38 +0000 Subject: [PATCH 1/4] fix: a Parquet timestamp read into the other timestamp type follows PostgreSQL's casts (#1706) Item 2 of #1706, decided there as option (a). The reader skipped the TimestampType's isAdjustedToUTC flag and returned the raw microseconds for either target type. In a New York session a naive (wall-time) column read into timestamptz came back as 08:00-04 instead of 12:00-04, and an adjusted column read into timestamp kept its UTC wall time instead of 08:00. The reader now keeps the flag (the legacy TIMESTAMP_* converted types are defined as adjusted) and, in those two mixed cases only, converts as timestamp_timestamptz and timestamptz_timestamp do, in the session zone. It is written from timestamp2tm, DetermineTimeZoneOffset and tm2timestamp, so it is the same on every major: PG19 renamed the exported helper, and PG15-18 export none for timestamptz -> timestamp. Same-kind reads, and any read in a UTC session, are unchanged. A wall time is not monotonic in its instant across a spring-forward gap: in New York on 2024-03-10, 02:00, 02:30 and 03:00 become 07:00Z, 07:30Z and 07:00Z. Converted row-group statistics need not bound the converted rows, so the foreign table no longer skips row groups of a column read this way. Measured: with skipping left on, `t > '07:15Z'` over such a group returned 0 rows instead of 1. The Iceberg foreign table's year(), month(), day() and hour() pruning compared a constant with partition cells the writer computed in the source field's own kind, while the reader now converts a mixed-kind column's rows. PgColumnarIcebergTemporalMap now leaves out a field whose Iceberg source type is the other timestamp kind than its column, so nothing prunes on it. Measured on PG17 before the guard, in New York: byhour declared timestamptz with `ts >= '2021-03-01 06:00+00'` returned no rows where iceberg_scan returned 2,3, and byhour_tz declared timestamp with `tt <= '2021-02-28 20:00'` returned none where it returned 1,2. #1707 (merged) is a prerequisite: with the reader honouring the flag and the old exporter marking timestamp adjusted, a timestamp exported and re-imported in a New York session came back 08:00 (measured). An arm pins that round trip. New suite test/parquet_timestamp_zone.sh and its twin, 16 checks, including the naive file read in a second zone (Tokyo), the repeated hour of the 2024-11-03 fall-back, and test/fixtures/parquet/legacy_timestamp_micros.parquet, written by main's exporter before #1707 (0c5e924d) with only the TIMESTAMP_MICROS converted type. Two arms in test/iceberg_fdw.sh and its twin. docs/user-guide.md states the rule; CHANGELOG under Changed. Measured on PG17: - Red, the head before the Iceberg guard: both new iceberg_fdw checks, in shell and twin ("got [] want [2,3]", "got [] want [1,2]"). - Mutants, the same names in shell and twin: the flag ignored (6), no instant-to-wall shift (2), skipping on converted stats (1), main's exporter before #1707 (the round trip, 1), the conversion pinned to New York (the Tokyo pair, 2), legacy converted types read as naive (the legacy pair, 2), the zone offset taken at the wall time read as UTC (the fall-back hour and the two gap checks, 3), no Iceberg kind guard (2). - Green: parquet_timestamp_zone 16/16 and iceberg_fdw 76/76, shell; 16/16 and 80/80, twin; on each of PG15 to PG19. - Without pyarrow the twin declines by the shell's skip name. - docs_style 53/53, harness_selftest 1248/1248, shellcheck clean. Ledger: 6951 rows, 2811 dated by the mutations and 4140 never. suites_not_covered 75 -> 75 (a new suite, registered and covered together; iceberg_fdw is not yet seeded, so its two new checks add no rows), cluster_tests 3222 -> 3240, TESTS.md section 296. The ledger gate, guards (804 passed), name grading and the self-test pass over 9ab6a4cf. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 + docs/user-guide.md | 18 ++ src/columnar_iceberg.c | 54 +++- src/columnar_parquet_reader.c | 75 ++++- test/check_ledger.tsv | 16 ++ test/check_ledger_budget.txt | 20 +- .../parquet/legacy_timestamp_micros.parquet | Bin 0 -> 146 bytes test/iceberg_fdw.sh | 25 ++ test/parquet_timestamp_zone.sh | 139 ++++++++++ test/pytest/TESTS.md | 27 ++ test/pytest/expected_tests.txt | 4 +- test/pytest/test_compare_to_bash.py | 1 + test/pytest/test_iceberg_fdw.py | 48 ++++ test/pytest/test_parquet_timestamp_zone.py | 261 ++++++++++++++++++ test/run_all_versions.sh | 1 + 15 files changed, 697 insertions(+), 5 deletions(-) create mode 100644 test/fixtures/parquet/legacy_timestamp_micros.parquet create mode 100755 test/parquet_timestamp_zone.sh create mode 100644 test/pytest/test_parquet_timestamp_zone.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d1baa30c..1db70a33d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -236,6 +236,19 @@ true until the next version shipped. over a failed attempt's file is allowed. ### Changed +- **A Parquet timestamp read into the other PostgreSQL timestamp type is + converted in the session's time zone.** The reader ignored the file's + `isAdjustedToUTC` flag. A wall-time column read into `timestamptz` was taken as + UTC (12:00 read as 08:00-04 in a New York session), and a UTC-adjusted column + read into `timestamp` kept its UTC wall time. Both now follow PostgreSQL's casts. + Same-type reads are unchanged, and so is any read in a UTC session (#1706). + A legacy `TIMESTAMP_MILLIS` or `TIMESTAMP_MICROS` column with no logical type, + as pgcolumnar's own exporter wrote before #1707, is read as UTC-adjusted, as + the Parquet format defines it. The Iceberg foreign table no longer prunes on a + `year()`, `month()`, `day()` or `hour()` partition whose source column is the + other timestamp type: its partition values are in the source's type, so they + no longer bound the converted rows. + `test/parquet_timestamp_zone.sh`, `test/iceberg_fdw.sh` and their twins. - `GROUP BY` is vectorized by default (#1292). `pgcolumnar.enable_group_vectorization` is now `on`. The planner takes the diff --git a/docs/user-guide.md b/docs/user-guide.md index cf8252056..e78b40614 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -245,6 +245,24 @@ EXPLAIN (ANALYZE, COSTS OFF) SELECT id FROM events_parquet WHERE ts >= '2026-01- -- Files: 4 ``` +**Timestamps and time zones.** A Parquet timestamp column holds either instants +(UTC-adjusted) or wall times with no zone. Read into the matching type +(`timestamptz` for instants, `timestamp` for wall times), a value comes back +unchanged. Read into the other type, it is converted as PostgreSQL's own cast +converts it, in the session's `TimeZone`: + +- a wall time read into `timestamptz` is taken as local time in that zone, like + `'2024-06-01 12:00'::timestamp::timestamptz`; +- an instant read into `timestamp` becomes the local wall time, like + `'2024-06-01 12:00+00'::timestamptz::timestamp`. + +A foreign-table scan does not skip row groups of a column read this way, because +the conversion does not keep order across a daylight-saving change. For the same +reason the Iceberg foreign table does not prune files on a `year()`, `month()`, +`day()` or `hour()` partition of such a column. A column marked only with the +older `TIMESTAMP_MILLIS` or `TIMESTAMP_MICROS` type, with no `isAdjustedToUTC` +flag, holds instants: the Parquet format defines it so. + The `path` may also be an object-storage URL. Every read and export function, and both foreign-data wrappers, accept an `s3://`, `http://`, or `https://` URL where they accept a local path. diff --git a/src/columnar_iceberg.c b/src/columnar_iceberg.c index c7deae891..dd4b2732b 100644 --- a/src/columnar_iceberg.c +++ b/src/columnar_iceberg.c @@ -1116,6 +1116,50 @@ ice_field_id_by_name(JsonbContainer *fieldsc, uint32 nf, const char *nm) return -1; } +/* + * Whether the current-schema field named `nm` is a timestamp of the OTHER kind + * than the PostgreSQL column `typid` (timestamp vs timestamptz, any unit). The + * reader converts such a column's rows as PostgreSQL's casts do, in the session + * zone (#1706), but the writer computed its temporal partition cells in the + * source's own kind, so those cells do not bound the converted rows. + */ +static bool +ice_field_timestamp_kind_differs(JsonbContainer *fieldsc, uint32 nf, + const char *nm, Oid typid) +{ + uint32 j; + + if (typid != TIMESTAMPOID && typid != TIMESTAMPTZOID) + return false; + for (j = 0; j < nf; j++) + { + JsonbValue *sf = getIthJsonbValueFromContainer(fieldsc, j); + JsonbValue *v; + bool src_tz; + + if (sf == NULL || sf->type != jbvBinary) + continue; + v = ice_field(sf->val.binary.data, "name"); + if (v == NULL || v->type != jbvString || + (int) strlen(nm) != v->val.string.len || + strncmp(nm, v->val.string.val, v->val.string.len) != 0) + continue; + v = ice_field(sf->val.binary.data, "type"); + if (v == NULL || v->type != jbvString) + return false; + if (v->val.string.len >= 11 && + strncmp(v->val.string.val, "timestamptz", 11) == 0) + src_tz = true; + else if (v->val.string.len >= 9 && + strncmp(v->val.string.val, "timestamp", 9) == 0) + src_tz = false; + else + return false; + return src_tz != (typid == TIMESTAMPTZOID); + } + return false; +} + /* * Map the current partition spec's IDENTITY fields to foreign-table columns, for * FDW pruning. On return *out_attno[k] is the 1-based attno of the table column @@ -1581,7 +1625,10 @@ PgColumnarIcebergDayMap(const char *path, const PgColumnarObjStoreConfig *cfg, * default spec's partition-cell positions and 1-based source attnos for the * fields carrying that transform. The FDW routes each mapped field by its source * column type: the coarse temporal compiler handles the timestamp cases, the - * exact day() compiler handles a day() on a date. + * exact day() compiler handles a day() on a date. A field whose source is a + * timestamp of the other kind than its column (timestamp vs timestamptz) is left + * out, so nothing prunes on it: its cells are in the source's kind, its rows are + * converted in the session zone (#1706). */ void PgColumnarIcebergTemporalMap(const char *path, const PgColumnarObjStoreConfig *cfg, @@ -1676,6 +1723,11 @@ PgColumnarIcebergTemporalMap(const char *path, const PgColumnarObjStoreConfig *c continue; if (ice_field_id_by_name(fieldsc, nf, NameStr(att->attname)) == srcid) { + /* the cells are in the source's kind, the rows in the column's */ + if (ice_field_timestamp_kind_differs(fieldsc, nf, + NameStr(att->attname), + att->atttypid)) + break; pos[cnt] = (int) k; attno[cnt] = a + 1; cnt++; diff --git a/src/columnar_parquet_reader.c b/src/columnar_parquet_reader.c index 42d8f15fc..16d0a5991 100644 --- a/src/columnar_parquet_reader.c +++ b/src/columnar_parquet_reader.c @@ -76,6 +76,8 @@ #include "utils/rel.h" #include "utils/snapmgr.h" #include "utils/timestamp.h" +#include "utils/datetime.h" +#include "pgtime.h" #include "utils/varlena.h" #include "utils/tuplestore.h" #include "utils/typcache.h" @@ -137,6 +139,7 @@ typedef struct PqSchemaCol int converted_type; /* -1 if none */ int time_unit; /* PQ_TU_* for a TIME/TIMESTAMP column */ bool is_timestamp; /* the unit belongs to TIMESTAMP, not TIME */ + bool adjusted_utc; /* TIMESTAMP isAdjustedToUTC: an instant, not a wall time */ int type_length; /* FIXED_LEN_BYTE_ARRAY length */ int scale; /* DECIMAL scale (0 if none) */ int precision; /* DECIMAL precision (0 if none) */ @@ -416,8 +419,12 @@ parse_logical_type(TCReader *r, PqSchemaCol *sc) PgColumnarThriftField(r, &uft, &ufid, &uLast); } } + else if (ifid == 1 && isTs && + (ift == TC_BOOL_TRUE || ift == TC_BOOL_FALSE)) + /* isAdjustedToUTC: compact protocol carries a bool in its type */ + sc->adjusted_utc = (ift == TC_BOOL_TRUE); else - PgColumnarThriftSkip(r, ift); /* isAdjustedToUTC and anything later */ + PgColumnarThriftSkip(r, ift); /* anything later */ } } else @@ -436,6 +443,8 @@ parse_schema_element(TCReader *r, PqSchemaCol *sc) sc->converted_type = -1; sc->time_unit = PQ_TU_NONE; sc->is_timestamp = false; + /* the legacy TIMESTAMP_* converted types are defined as UTC-adjusted */ + sc->adjusted_utc = true; sc->type_length = 0; sc->scale = 0; sc->precision = 0; @@ -875,8 +884,21 @@ typedef struct PqColPlan int type_length; /* FIXED_LEN_BYTE_ARRAY width, for uuid/decimal */ int dec_scale; /* DECIMAL scale, when the target is numeric */ bool is_decimal; /* the bytes are a DECIMAL unscaled integer */ + int ts_shift; /* PQ_TS_*: a cast between wall time and instant */ } PqColPlan; +/* + * A Parquet timestamp read into the OTHER PostgreSQL timestamp type is converted + * as PostgreSQL's own cast would, in the session zone (#1706): a naive (not + * UTC-adjusted) wall time into timestamptz, and an adjusted instant into + * timestamp. The same-kind reads are left as they are. + */ +#define PQ_TS_NONE 0 +#define PQ_TS_WALL_TO_INSTANT 1 +#define PQ_TS_INSTANT_TO_WALL 2 + +static bool pq_ts_shift(int shift, Timestamp in, Timestamp *out); + /* * Convert a stored TIME/TIMESTAMP value to the microseconds PostgreSQL stores, * per the column's declared unit. Returns false if the conversion overflows. @@ -1176,7 +1198,9 @@ plain_value_to_datum(const PqColPlan *plan, const uint8 **p, const uint8 *end, if (!pq_scale_to_usecs(plan->time_unit, v, &us) || pg_sub_s64_overflow(us, PG_TO_UNIX_USECS, &t) || - !IS_VALID_TIMESTAMP((Timestamp) t)) + !IS_VALID_TIMESTAMP((Timestamp) t) || + (plan->ts_shift != PQ_TS_NONE && + !pq_ts_shift(plan->ts_shift, (Timestamp) t, (Timestamp *) &t))) return pq_value_out_of_range(strict, ok, ERRCODE_DATETIME_FIELD_OVERFLOW, tn, v); @@ -2276,6 +2300,47 @@ pq_plan_bind_schema(PqColPlan *plan, const PqSchemaCol *sc) plan->dec_scale = sc->scale; plan->is_decimal = (plan->typid == NUMERICOID && sc->converted_type == PQ_CT_DECIMAL); + plan->ts_shift = PQ_TS_NONE; + if (sc->is_timestamp && !sc->adjusted_utc && plan->typid == TIMESTAMPTZOID) + plan->ts_shift = PQ_TS_WALL_TO_INSTANT; + else if (sc->is_timestamp && sc->adjusted_utc && plan->typid == TIMESTAMPOID) + plan->ts_shift = PQ_TS_INSTANT_TO_WALL; +} + +/* + * The two casts, from the primitives PostgreSQL's timestamp_timestamptz and + * timestamptz_timestamp use, so they behave the same on every major (PG19 renamed + * the exported helper, and PG15-18 export no timestamptz -> timestamp one). + * false when the result is out of range. + */ +static bool +pq_ts_shift(int shift, Timestamp in, Timestamp *out) +{ + struct pg_tm tm; + fsec_t fsec; + int tz; + + if (TIMESTAMP_NOT_FINITE(in)) + { + *out = in; + return true; + } + if (shift == PQ_TS_WALL_TO_INSTANT) + { + if (timestamp2tm(in, NULL, &tm, &fsec, NULL, NULL) != 0) + return false; + tz = DetermineTimeZoneOffset(&tm, session_timezone); + if (tm2timestamp(&tm, fsec, &tz, out) != 0) + return false; + } + else + { + if (timestamp2tm(in, &tz, &tm, &fsec, NULL, NULL) != 0) + return false; + if (tm2timestamp(&tm, fsec, NULL, out) != 0) + return false; + } + return IS_VALID_TIMESTAMP(*out); } /* @@ -4486,6 +4551,12 @@ pqfdw_clause_excludes_group(ImpLeaf *leaf, PqChunk *ch, int strategy, return false; if (con->consttype != leaf->plan.typid || con->constisnull) return false; + /* + * A wall time is not monotonic in its instant across a spring-forward gap, so + * converted statistics need not bound the converted rows (#1706): never skip. + */ + if (leaf->plan.ts_shift != PQ_TS_NONE) + return false; tce = lookup_type_cache(leaf->plan.typid, TYPECACHE_CMP_PROC_FINFO); if (!OidIsValid(tce->cmp_proc_finfo.fn_oid)) diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index e5e80d92d..ace68abc1 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -5804,6 +5804,22 @@ parquet_nested_import parquet_nested_import nested import round-trips (arrays + parquet_nested_import parquet_nested_import pyarrow import rows 15;16;17;18;19 never - parquet_nested_import parquet_nested_import pyarrow nested file matches oracle 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: an empty Parquet list is read back as a NULL array parquet_nested_import parquet_nested_import round-trips after delete 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: an empty Parquet list is read back as a NULL array +parquet_timestamp_zone parquet_timestamp_zone a legacy TIMESTAMP_MICROS file read into timestamp is the session-zone wall time of its instant 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a column with only a legacy TIMESTAMP_* converted type reads as not UTC-adjusted +parquet_timestamp_zone parquet_timestamp_zone a legacy TIMESTAMP_MICROS file read into timestamptz keeps its instant 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a column with only a legacy TIMESTAMP_* converted type reads as not UTC-adjusted +parquet_timestamp_zone parquet_timestamp_zone a naive file read into timestamptz in Tokyo is its wall time there, as the cast gives 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a naive column read into timestamptz is converted in America/New_York, not the session zone +parquet_timestamp_zone parquet_timestamp_zone a naive file read into timestamptz is its wall time in the session zone 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: isAdjustedToUTC is ignored, so every timestamp column reads as UTC-adjusted +parquet_timestamp_zone parquet_timestamp_zone a naive wall time in the fall-back hour read into timestamptz is what the cast gives 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a naive wall time takes the zone offset in force at that wall time read as UTC, not at the wall time itself +parquet_timestamp_zone parquet_timestamp_zone a row group spanning a spring-forward gap is not skipped on its converted min and max 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a converted timestamp column's row groups are skipped on their converted min and max;columnar_parquet_reader.c: a naive wall time takes the zone offset in force at that wall time read as UTC, not at the wall time itself +parquet_timestamp_zone parquet_timestamp_zone an adjusted file read into timestamp is the session-zone wall time of its instant 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: an adjusted column read into timestamp is not converted to the session-zone wall time +parquet_timestamp_zone parquet_timestamp_zone an export and import round trip keeps timestamp and timestamptz in a non-UTC session 15;16;17;18;19 2026-10-22 columnar_parquet.c: main's exporter before #1707, which marks a timestamp column UTC-adjusted +parquet_timestamp_zone parquet_timestamp_zone and that is what PostgreSQL's cast from timestamp gives 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: isAdjustedToUTC is ignored, so every timestamp column reads as UTC-adjusted +parquet_timestamp_zone parquet_timestamp_zone and that is what PostgreSQL's cast from timestamptz gives 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: an adjusted column read into timestamp is not converted to the session-zone wall time +parquet_timestamp_zone parquet_timestamp_zone and the Tokyo reading is a different instant from the New York one 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a naive column read into timestamptz is converted in America/New_York, not the session zone +parquet_timestamp_zone parquet_timestamp_zone control: a naive file read into timestamp keeps its wall time 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: isAdjustedToUTC is ignored, so every timestamp column reads as UTC-adjusted +parquet_timestamp_zone parquet_timestamp_zone control: an adjusted file read into timestamptz keeps its instant 15;16;17;18;19 never - +parquet_timestamp_zone parquet_timestamp_zone premise: pyarrow wrote the naive file without isAdjustedToUTC and the other with it 15;16;17;18;19 never - +parquet_timestamp_zone parquet_timestamp_zone premise: the legacy fixture carries the TIMESTAMP_MICROS converted type and no logical type 15;16;17;18;19 never - +parquet_timestamp_zone parquet_timestamp_zone the gap file's wall times 02:00, 02:30 and 03:00 become, sorted, the instants 07:00Z, 07:00Z and 07:30Z 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a naive wall time takes the zone offset in force at that wall time read as UTC, not at the wall time itself;columnar_parquet_reader.c: isAdjustedToUTC is ignored, so every timestamp column reads as UTC-adjusted partition_lifecycle partition_lifecycle a NULL cutoff is refused, in both overloads 15;16;17;18;19 2026-10-13 partition_drop_plan without its NULL-cutoff check, in both overloads partition_lifecycle partition_lifecycle a call that would create more than 1000 partitions is refused before any DDL 15;16;17;18;19 2026-10-12 partition_create_ahead without the 1000-partition bound partition_lifecycle partition_lifecycle a cutoff before every partition plans nothing 15;16;17;18;19 2026-10-13 the cutoff filter removed: every partition planned diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 8b62c6794..b304ad7de 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -7056,4 +7056,22 @@ suites_not_covered 27 # never 4520 + 15 = 4535 # dated 3443 + 2 = 3445 # 4535 + 3445 == 7980 -checks_never_observed_red 4535 +# +# #1706 item 2, parquet_timestamp_zone seeded, with its twin (jdatcmd): 4535 -> 4538. A new +# suite, so suites_not_covered does not move: it is registered and covered in the same change. +# It enters with 16 checks from five-major runs (all green on each of 15-19, shell and twin +# alike). 13 are dated by seven mutations, each reddening the same names in both twins: the +# reader ignoring isAdjustedToUTC (6), not converting an adjusted column read into +# timestamp (2), skipping a converted column's row groups on their converted statistics +# (the spring-forward group, 1), main's exporter before #1707 (the round trip, 1), the +# conversion pinned to New York instead of the session zone (the Tokyo pair, 2), a legacy +# TIMESTAMP_MICROS column read as naive (the legacy pair, 2), and the zone offset taken at +# the wall time read as UTC (the fall-back hour, with the two gap checks, 1 new). The two +# premises and the adjusted-into-timestamptz control arrive never. iceberg_fdw's two new +# mixed-kind hour() checks add no rows: that suite is not yet seeded. +# +# ledger rows 7980 + 16 = 7996 +# never 4535 + 3 = 4538 +# dated 3445 + 13 = 3458 +# 4538 + 3458 == 7996 +checks_never_observed_red 4538 diff --git a/test/fixtures/parquet/legacy_timestamp_micros.parquet b/test/fixtures/parquet/legacy_timestamp_micros.parquet new file mode 100644 index 0000000000000000000000000000000000000000..8c842e2ffa07b1068c5996762d3050a50fc3c620 GIT binary patch literal 146 zcmWG=3^EjD5S0;?(Gg_=GT21f7#Nrs7#Nrt87{oNc1e= TIMESTAMPTZ '2021-03-01 01:00:00+00'")" fi +# ---- a temporal transform read into the OTHER timestamp type (#1706) --------- +# The reader converts a naive (timestamp) source read into timestamptz, and a +# timestamptz source read into timestamp, as PostgreSQL's casts do in the session +# zone. The writer computed each partition cell in the source's own kind, so a +# cell does not bound the converted rows, and the FDW must not prune on it. +# In New York, byhour's naive 2021-03-01 00:00, 01:00, 05:00 are 05:00Z, 06:00Z +# and 10:00Z; byhour_tz's 00:00Z, 01:00Z, 05:00Z are the wall times 2021-02-28 +# 19:00, 20:00 and 2021-03-01 00:00. Each FDW read is compared with iceberg_scan +# under the same declaration and zone, and check_text refuses two empty readings. +qny() { PGOPTIONS="-c TimeZone=America/New_York" q "$1"; } +if [ -n "${MD_th:-}" ] && ftmpx hn byhour_tz "tt timestamp"; then + q "CREATE FOREIGN TABLE hw (id bigint, ts timestamptz) + SERVER ice OPTIONS (metadata_path '$MD_th')" >/dev/null + check_text "hour() on a naive source read as timestamptz in New York matches iceberg_scan" \ + "$(qny "SELECT string_agg(id::text, ',' ORDER BY id) FROM hw WHERE ts >= TIMESTAMPTZ '2021-03-01 06:00:00+00'")" \ + "$(qny "SELECT string_agg(id::text, ',' ORDER BY id) + FROM pgcolumnar.iceberg_scan('$MD_th') AS t(id bigint, ts timestamptz) + WHERE ts >= TIMESTAMPTZ '2021-03-01 06:00:00+00'")" + check_text "hour() on a timestamptz source read as timestamp in New York matches iceberg_scan" \ + "$(qny "SELECT string_agg(id::text, ',' ORDER BY id) FROM hn WHERE tt <= TIMESTAMP '2021-02-28 20:00:00'")" \ + "$(qny "SELECT string_agg(id::text, ',' ORDER BY id) + FROM pgcolumnar.iceberg_scan('$MD_hn') AS t(id bigint, tt timestamp) + WHERE tt <= TIMESTAMP '2021-02-28 20:00:00'")" +fi + # ---- an invalid option is rejected by the validator ------------------------- check "an unknown table option is refused (FDW_INVALID_OPTION_NAME)" \ "$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -qtA 2>&1 </dev/null; then + pgc_skip pyarrow "pyarrow not available; Parquet timestamp zone suite needs it" +fi + +W="$PGC_WORKDIR" +# Every session of this suite runs in New York, so each reading below is in that zone. +export PGOPTIONS="-c TimeZone=America/New_York" +psql_run "CREATE SERVER pq FOREIGN DATA WRAPPER pgcolumnar_parquet;" + +python3 - "$W" <<'PY' +import sys, datetime as dt, pyarrow as pa, pyarrow.parquet as pq +w = sys.argv[1] +noon = dt.datetime(2024, 6, 1, 12, 0) +# naive: timestamp[us], isAdjustedToUTC=false; adjusted: tz=UTC, isAdjustedToUTC=true +pq.write_table(pa.table({"t": pa.array([noon], pa.timestamp("us"))}), f"{w}/naive.parquet") +pq.write_table(pa.table({"t": pa.array([noon], pa.timestamp("us", tz="UTC"))}), f"{w}/adjusted.parquet") +# one row group spanning New York's 2024-03-10 spring-forward gap +gap = [dt.datetime(2024, 3, 10, 2, 0), dt.datetime(2024, 3, 10, 2, 30), dt.datetime(2024, 3, 10, 3, 0)] +pq.write_table(pa.table({"t": pa.array(gap, pa.timestamp("us"))}), f"{w}/gap.parquet", row_group_size=3) +# the repeated hour of New York's 2024-11-03 fall-back +fb = [dt.datetime(2024, 11, 3, 1, 30)] +pq.write_table(pa.table({"t": pa.array(fb, pa.timestamp("us"))}), f"{w}/fallback.parquet") +PY +cp "$(dirname "${BASH_SOURCE[0]}")/fixtures/parquet/legacy_timestamp_micros.parquet" "$W/legacy.parquet" +for f in naive adjusted gap fallback legacy; do chmod 644 "$W/$f.parquet"; done + +check "premise: pyarrow wrote the naive file without isAdjustedToUTC and the other with it" \ + "$(python3 -c " +import sys, pyarrow.parquet as pq +a = lambda f: pq.ParquetFile(f).schema_arrow.field('t').type.tz +print(a('$W/naive.parquet'), a('$W/adjusted.parquet'))")" "None UTC" +# pyarrow derives a logical type from a converted type and says so in this flag +check "premise: the legacy fixture carries the TIMESTAMP_MICROS converted type and no logical type" \ + "$(python3 -c " +import pyarrow.parquet as pq +c = pq.ParquetFile('$W/legacy.parquet').metadata.schema.column(0) +print(c.converted_type, 'is_from_converted_type=true' in str(c.logical_type))")" "TIMESTAMP_MICROS True" + +psql_run "CREATE TABLE n_ts (t timestamp) USING pgcolumnar; CREATE TABLE n_tz (t timestamptz) USING pgcolumnar; + CREATE TABLE a_ts (t timestamp) USING pgcolumnar; CREATE TABLE a_tz (t timestamptz) USING pgcolumnar;" +for t in n_ts n_tz; do psql_run "SELECT pgcolumnar.import_parquet('$t', '$W/naive.parquet');"; done +for t in a_ts a_tz; do psql_run "SELECT pgcolumnar.import_parquet('$t', '$W/adjusted.parquet');"; done + +check_text "a naive file read into timestamptz is its wall time in the session zone" \ + "$(q "SELECT t::text FROM n_tz;")" "2024-06-01 12:00:00-04" +check_text "and that is what PostgreSQL's cast from timestamp gives" \ + "$(q "SELECT (SELECT t FROM n_tz) = '2024-06-01 12:00'::timestamp::timestamptz;")" "t" +check_text "an adjusted file read into timestamp is the session-zone wall time of its instant" \ + "$(q "SELECT t::text FROM a_ts;")" "2024-06-01 08:00:00" +check_text "and that is what PostgreSQL's cast from timestamptz gives" \ + "$(q "SELECT (SELECT t FROM a_ts) = '2024-06-01 12:00+00'::timestamptz::timestamp;")" "t" +check_text "control: a naive file read into timestamp keeps its wall time" \ + "$(q "SELECT t::text FROM n_ts;")" "2024-06-01 12:00:00" +check_text "control: an adjusted file read into timestamptz keeps its instant" \ + "$(q "SELECT t::text FROM a_tz;")" "2024-06-01 08:00:00-04" + +# The round trip through pgcolumnar's own export, in the same zone. This holds only +# because the exporter writes timestamp as naive (#1707): with the reader honouring +# the flag and the old exporter marking timestamp adjusted, the timestamp came back +# 08:00, four hours early. +psql_run "CREATE TABLE rt_src (ts timestamp, tz timestamptz) USING pgcolumnar; + INSERT INTO rt_src VALUES ('2024-06-01 12:00', '2024-06-01 12:00'); + CREATE TABLE rt_dst (ts timestamp, tz timestamptz) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.export_parquet('rt_src', '$W/rt.parquet');" +psql_run "SELECT pgcolumnar.import_parquet('rt_dst', '$W/rt.parquet');" +check_text "an export and import round trip keeps timestamp and timestamptz in a non-UTC session" \ + "$(q "SELECT ts::text || ' | ' || tz::text FROM rt_dst;")" "2024-06-01 12:00:00 | 2024-06-01 12:00:00-04" + +psql_run "CREATE FOREIGN TABLE f_gap (t timestamptz) SERVER pq OPTIONS (path '$W/gap.parquet');" +check_text "the gap file's wall times 02:00, 02:30 and 03:00 become, sorted, the instants 07:00Z, 07:00Z and 07:30Z" \ + "$(q "SELECT string_agg(to_char(t AT TIME ZONE 'UTC', 'HH24:MI'), ',' ORDER BY t) FROM f_gap;")" \ + "07:00,07:00,07:30" +check_text "a row group spanning a spring-forward gap is not skipped on its converted min and max" \ + "$(q "SELECT count(*) FROM f_gap WHERE t > '2024-03-10 07:15+00';")" "1" + +# A second zone: the same naive file read through a foreign table in Tokyo. +psql_run "CREATE FOREIGN TABLE f_naive (t timestamptz) SERVER pq OPTIONS (path '$W/naive.parquet');" +qtk() { PGOPTIONS="-c TimeZone=Asia/Tokyo" q "$1"; } +check_text "a naive file read into timestamptz in Tokyo is its wall time there, as the cast gives" \ + "$(qtk "SELECT t::text || ' ' || (t = '2024-06-01 12:00'::timestamp::timestamptz)::text FROM f_naive;")" \ + "2024-06-01 12:00:00+09 true" +check_text "and the Tokyo reading is a different instant from the New York one" \ + "$(qtk "SELECT ((SELECT t FROM f_naive) <> (SELECT t FROM n_tz))::text;")" "true" + +# The repeated hour: 01:30 happens twice in New York on 2024-11-03. +psql_run "CREATE FOREIGN TABLE f_fb (t timestamptz) SERVER pq OPTIONS (path '$W/fallback.parquet');" +check_text "a naive wall time in the fall-back hour read into timestamptz is what the cast gives" \ + "$(q "SELECT t::text || ' ' || (t = '2024-11-03 01:30'::timestamp::timestamptz)::text FROM f_fb;")" \ + "2024-11-03 01:30:00-05 true" + +# The legacy converted type, read as the instant the spec defines it to be. +psql_run "CREATE FOREIGN TABLE f_leg_ts (t timestamp) SERVER pq OPTIONS (path '$W/legacy.parquet'); + CREATE FOREIGN TABLE f_leg_tz (t timestamptz) SERVER pq OPTIONS (path '$W/legacy.parquet');" +check_text "a legacy TIMESTAMP_MICROS file read into timestamp is the session-zone wall time of its instant" \ + "$(q "SELECT t::text || ' ' || (t = '2024-06-01 12:00+00'::timestamptz::timestamp)::text FROM f_leg_ts;")" \ + "2024-06-01 08:00:00 true" +check_text "a legacy TIMESTAMP_MICROS file read into timestamptz keeps its instant" \ + "$(q "SELECT t::text FROM f_leg_tz;")" "2024-06-01 08:00:00-04" + +pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 6048fbf14..d2a4db9c0 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -388,6 +388,7 @@ behaviour, the source of that number is named. - [340. test_decode_skip_interrupts.py: the Thrift and Avro skip loops of a hostile file yield to a cancel, per skipped value](#340-test_decode_skip_interruptspy-the-thrift-and-avro-skip-loops-of-a-hostile-file-yield-to-a-cancel-per-skipped-value) - [341. test_native_cancel.py: a scan's row-group load yields to statement_timeout inside the decode loop](#341-test_native_cancelpy-a-scans-row-group-load-yields-to-statement_timeout-inside-the-decode-loop) - [342. test_objstore_export_double_cancel.py: a second cancel during a remote export's cleanup keeps the export's own error](#342-test_objstore_export_double_cancelpy-a-second-cancel-during-a-remote-exports-cleanup-keeps-the-exports-own-error) +- [343. test_parquet_timestamp_zone.py: a Parquet timestamp read into the other timestamp type follows PostgreSQL's casts](#343-test_parquet_timestamp_zonepy-a-parquet-timestamp-read-into-the-other-timestamp-type-follows-postgresqls-casts) ## 1. How to read a test in here @@ -4445,6 +4446,8 @@ carry are `pgc_skip`'s refusal names, which are structural and declared in | `test_the_year_table_reads_whole_and_keeps_the_boundary_file` | the boundary case stated alone | | `test_a_year_equality_prunes_to_one_file` | an equality narrows to the constant's own bucket | | `test_a_year_on_date_equality_prunes_to_one_file` | the same on a date column | +| `test_an_hour_partition_over_a_naive_source_read_as_timestamptz_keeps_its_rows` | #1706: `byhour` (a naive source) declared `timestamptz` in New York returns `iceberg_scan`'s rows; its hour cells are wall times, so the FDW must not prune on them | +| `test_an_hour_partition_over_a_timestamptz_source_read_as_timestamp_keeps_its_rows` | #1706: the reverse, `byhour_tz` declared `timestamp` | | `test_an_unknown_table_option_is_refused` | the validator, by SQLSTATE `HV00D` rather than by message text | | `test_a_plan_with_no_pruning_marker_is_not_read_as_zero` | a plan that never mentions `Files Pruned` is not read as 0; needs no server | @@ -12845,3 +12848,27 @@ Every test reads one value from a module fixture that runs the whole scenario on | `test_no_completed_object_remains_at_the_prefix` | double cancel: no completed object remains at the prefix | | `test_the_server_survived` | double cancel: the server survived | +## 343. test_parquet_timestamp_zone.py: a Parquet timestamp read into the other timestamp type follows PostgreSQL's casts + +The pytest twin of `test/parquet_timestamp_zone.sh` (#1706 item 2), every check by the shell's name. In a New York session: a naive Parquet timestamp read into `timestamptz` is its wall time in the session zone, and a UTC-adjusted one read into `timestamp` is the session-zone wall time of its instant, each equal to PostgreSQL's own cast; the same-kind reads are controls; an export and import round trip keeps both types (with #1707's exporter); and a row group spanning the 2024-03-10 spring-forward gap is not skipped on its converted min and max, which need not bound its converted rows. The naive file is read again in Tokyo, so a reader that hard-codes a zone fails; the repeated hour of the 2024-11-03 fall-back resolves as the cast resolves it; and `test/fixtures/parquet/legacy_timestamp_micros.parquet`, written by pgcolumnar's own exporter before #1707 with only the `TIMESTAMP_MICROS` converted type, reads as the instant the Parquet spec defines it to be. The files are written by `python3` with pyarrow, so no test imports pyarrow: each declines by the shell's skip name, then asks for the fixture. On a private database. + +### Every test + +| test | what it holds | +| --- | --- | +| `test_ptz_pyarrow_wrote_one_naive_and_one_adjusted_file` | premise: pyarrow wrote the naive file without isAdjustedToUTC and the other with it | +| `test_ptz_a_naive_file_into_timestamptz_is_its_session_wall_time` | a naive file read into timestamptz is its wall time in the session zone | +| `test_ptz_the_naive_read_equals_the_cast_from_timestamp` | and that is what PostgreSQL's cast from timestamp gives | +| `test_ptz_an_adjusted_file_into_timestamp_is_the_session_wall_time` | an adjusted file read into timestamp is the session-zone wall time of its instant | +| `test_ptz_the_adjusted_read_equals_the_cast_from_timestamptz` | and that is what PostgreSQL's cast from timestamptz gives | +| `test_ptz_a_naive_file_into_timestamp_keeps_its_wall_time` | control: a naive file read into timestamp keeps its wall time | +| `test_ptz_an_adjusted_file_into_timestamptz_keeps_its_instant` | control: an adjusted file read into timestamptz keeps its instant | +| `test_ptz_an_export_import_round_trip_keeps_both_types` | an export and import round trip keeps timestamp and timestamptz in a non-UTC session | +| `test_ptz_the_gap_wall_times_become_their_instants` | the gap file's wall times become, sorted, the instants 07:00Z, 07:00Z and 07:30Z | +| `test_ptz_a_gap_spanning_group_is_not_skipped` | a row group spanning a spring-forward gap is not skipped on its converted min and max | +| `test_ptz_the_legacy_fixture_carries_only_the_converted_type` | premise: the legacy fixture carries the TIMESTAMP_MICROS converted type and no logical type | +| `test_ptz_a_naive_file_in_tokyo_is_its_wall_time_there` | a naive file read into timestamptz in Tokyo is its wall time there, as the cast gives | +| `test_ptz_the_tokyo_reading_differs_from_new_york` | and the Tokyo reading is a different instant from the New York one | +| `test_ptz_a_fall_back_wall_time_is_what_the_cast_gives` | a naive wall time in the fall-back hour read into timestamptz is what the cast gives | +| `test_ptz_a_legacy_file_into_timestamp_is_the_session_wall_time` | a legacy TIMESTAMP_MICROS file read into timestamp is the session-zone wall time of its instant | +| `test_ptz_a_legacy_file_into_timestamptz_keeps_its_instant` | a legacy TIMESTAMP_MICROS file read into timestamptz keeps its instant | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index c2ef5cf38..5e30d3814 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -1210,4 +1210,6 @@ guard_tests 839 # 54 tests, one per check, by collection over the cluster files. # cluster_tests 4225 -> 4226 for #1753's arm 5 in test_parallel_flush_optin.py (folded into #1746): # one structural test, by collection over the cluster files. -cluster_tests 4231 +# cluster_tests 4231 -> 4249 for #1706: test_parquet_timestamp_zone.py, the twin of parquet_timestamp_zone.sh +# (16 tests, one per check), and two mixed-kind hour() arms in test_iceberg_fdw.py, by collection. +cluster_tests 4249 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 216ab6438..834787edf 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -207,6 +207,7 @@ "cancel_decode", "bench_guards", "catalog_delete_index", + "parquet_timestamp_zone", "catalog_filter_reads", "catalog_plan_index", "catalog_scan_index", diff --git a/test/pytest/test_iceberg_fdw.py b/test/pytest/test_iceberg_fdw.py index f87043d7e..ad7dd90ea 100644 --- a/test/pytest/test_iceberg_fdw.py +++ b/test/pytest/test_iceberg_fdw.py @@ -58,6 +58,9 @@ "mz": ("warehouse_temporal_xt", "bymonth_tz", "id bigint, tt timestamptz"), "dz": ("warehouse_temporal_xt", "byday_tz", "id bigint, tt timestamptz"), "hz": ("warehouse_temporal_xt", "byhour_tz", "id bigint, tt timestamptz"), + # the same two hour() tables declared as the OTHER timestamp type (#1706) + "hw": ("warehouse_temporal", "byhour", "id bigint, ts timestamptz"), + "hn": ("warehouse_temporal_xt", "byhour_tz", "id bigint, tt timestamp"), } @@ -647,6 +650,51 @@ def test_a_year_on_date_equality_prunes_to_one_file(ice, expect): "year()-on-date equality returns id 2") +# ------------------------------- a temporal transform read into the OTHER timestamp type +# +# The reader converts a naive (timestamp) source read into timestamptz, and a +# timestamptz source read into timestamp, as PostgreSQL's casts do in the session zone +# (#1706). The writer computed each partition cell in the source's own kind, so a cell +# does not bound the converted rows and the FDW must not prune on it. In New York, +# byhour's naive 2021-03-01 00:00, 01:00, 05:00 are 05:00Z, 06:00Z and 10:00Z; byhour_tz's +# 00:00Z, 01:00Z, 05:00Z are the wall times 2021-02-28 19:00, 20:00 and 2021-03-01 00:00. +# Measured before the guard: the FDW returned no rows where iceberg_scan returned two. + +def _in_new_york(conn, read): + """-> read(), with the shared connection's zone set to New York for that read only.""" + with conn.cursor() as cur: + cur.execute("SET TimeZone = 'America/New_York'") + try: + return read() + finally: + with conn.cursor() as cur: + cur.execute("RESET TimeZone") + + +def test_an_hour_partition_over_a_naive_source_read_as_timestamptz_keeps_its_rows(ice, expect): + conn, meta = ice + if not _need(expect, meta, "hw"): + return + where = "ts >= TIMESTAMPTZ '2021-03-01 06:00:00+00'" + got, want = _in_new_york(conn, lambda: ( + _ids(conn, "hw", where), + _scan_ids(conn, meta["hw"], "id bigint, ts timestamptz", where))) + expect.text(got, want, + "hour() on a naive source read as timestamptz in New York matches iceberg_scan") + + +def test_an_hour_partition_over_a_timestamptz_source_read_as_timestamp_keeps_its_rows(ice, expect): + conn, meta = ice + if not _need(expect, meta, "hn"): + return + where = "tt <= TIMESTAMP '2021-02-28 20:00:00'" + got, want = _in_new_york(conn, lambda: ( + _ids(conn, "hn", where), + _scan_ids(conn, meta["hn"], "id bigint, tt timestamp", where))) + expect.text(got, want, + "hour() on a timestamptz source read as timestamp in New York matches iceberg_scan") + + # ------------------------------------------------------------------- option validation def test_an_unknown_table_option_is_refused(ice, expect): diff --git a/test/pytest/test_parquet_timestamp_zone.py b/test/pytest/test_parquet_timestamp_zone.py new file mode 100644 index 000000000..6d4035185 --- /dev/null +++ b/test/pytest/test_parquet_timestamp_zone.py @@ -0,0 +1,261 @@ +"""A Parquet timestamp read into the other PostgreSQL timestamp type follows PostgreSQL's +cast semantics (#1706 item 2). The pytest twin of `test/parquet_timestamp_zone.sh`, every +check by the shell's name. + +Parquet marks a timestamp column isAdjustedToUTC: true for an instant, false for a wall +time. In a New York session, a naive file read into timestamptz must be its wall time in +the session zone, and an adjusted file read into timestamp the session-zone wall time of +its instant, each equal to PostgreSQL's own cast; the same-kind reads are the controls. An +export and import round trip keeps both types (it needs #1707's exporter). A row group that +spans the 2024-03-10 spring-forward gap must not be skipped on its converted min and max, +which need not bound its converted rows. The naive file read again in Tokyo must be the cast +in that zone, a different instant from New York's; the repeated hour of New York's 2024-11-03 +fall-back must resolve as the cast resolves it; and a file carrying only the legacy +TIMESTAMP_MICROS converted type (pgcolumnar's own export before #1707, checked in as +`test/fixtures/parquet/legacy_timestamp_micros.parquet`) is read as the instant the +Parquet spec defines it to be. + +The files are written by `python3` with pyarrow, as the shell writes them, so no test here +imports pyarrow: each declines by the shell's skip name first, and only then asks for the +module fixture. ON A PRIVATE DATABASE, every session at TimeZone America/New_York. +""" +import pathlib +import shutil +import subprocess +import sys + +import psycopg +import pytest + +from pgc_vacuity import query_error + +DB = "pgc_parquet_timestamp_zone" +ZONE = "-c TimeZone=America/New_York" +TOKYO = "-c TimeZone=Asia/Tokyo" +LEGACY = (pathlib.Path(__file__).resolve().parents[1] / "fixtures" / "parquet" + / "legacy_timestamp_micros.parquet") + +WRITER = r''' +import sys, datetime as dt, pyarrow as pa, pyarrow.parquet as pq +w = sys.argv[1] +noon = dt.datetime(2024, 6, 1, 12, 0) +pq.write_table(pa.table({"t": pa.array([noon], pa.timestamp("us"))}), f"{w}/naive.parquet") +pq.write_table(pa.table({"t": pa.array([noon], pa.timestamp("us", tz="UTC"))}), f"{w}/adjusted.parquet") +gap = [dt.datetime(2024, 3, 10, 2, 0), dt.datetime(2024, 3, 10, 2, 30), dt.datetime(2024, 3, 10, 3, 0)] +pq.write_table(pa.table({"t": pa.array(gap, pa.timestamp("us"))}), f"{w}/gap.parquet", row_group_size=3) +fb = [dt.datetime(2024, 11, 3, 1, 30)] +pq.write_table(pa.table({"t": pa.array(fb, pa.timestamp("us"))}), f"{w}/fallback.parquet") +a = lambda f: pq.ParquetFile(f).schema_arrow.field("t").type.tz +print(a(f"{w}/naive.parquet"), a(f"{w}/adjusted.parquet")) +''' + +# pyarrow derives a logical type from a converted type and says so in this flag +LEGACY_PREMISE = r''' +import sys, pyarrow.parquet as pq +c = pq.ParquetFile(sys.argv[1]).metadata.schema.column(0) +print(c.converted_type, "is_from_converted_type=true" in str(c.logical_type)) +''' + + +def _failed(exc): + return query_error(str(exc).splitlines()[0] if str(exc) else type(exc).__name__) + + +def _value(dsn, sql, zone=ZONE): + """-> the first column of the first row as text, in a New York session unless + `zone` names another, or query_error() carrying the failure.""" + try: + with psycopg.connect(dsn, autocommit=True, options=zone) as conn: + row = conn.execute(sql).fetchone() + except psycopg.Error as exc: + return _failed(exc) + return "" if row is None or row[0] is None else str(row[0]) + + +def _run(dsn, sql): + try: + with psycopg.connect(dsn, autocommit=True, options=ZONE) as conn: + conn.execute(sql) + except psycopg.Error as exc: + return _failed(exc) + return None + + +@pytest.fixture(scope="module") +def ptz(pgc_cluster, server_dir): + with psycopg.connect(pgc_cluster.dsn(), autocommit=True) as a: + a.execute(f"DROP DATABASE IF EXISTS {DB} WITH (FORCE)") + a.execute(f"CREATE DATABASE {DB}") + try: + dsn = pgc_cluster.dsn(dbname=DB) + w = server_dir + wrote = subprocess.run([sys.executable, "-c", WRITER, str(w)], capture_output=True, text=True) + shutil.copyfile(LEGACY, w / "legacy.parquet") + for f in ("naive", "adjusted", "gap", "fallback", "legacy"): + if (w / f"{f}.parquet").exists(): + (w / f"{f}.parquet").chmod(0o644) + r = {"tz of the files": wrote.stdout.strip() if wrote.returncode == 0 + else query_error(f"the writer failed: {wrote.stderr.strip().splitlines()[-1:]}")} + legacy = subprocess.run([sys.executable, "-c", LEGACY_PREMISE, str(w / "legacy.parquet")], + capture_output=True, text=True) + r["legacy premise"] = (legacy.stdout.strip() if legacy.returncode == 0 else + query_error(f"the reader failed: {legacy.stderr.strip().splitlines()[-1:]}")) + _run(dsn, "CREATE EXTENSION IF NOT EXISTS pgcolumnar") + _run(dsn, "CREATE SERVER pq FOREIGN DATA WRAPPER pgcolumnar_parquet") + for t, typ in (("n_ts", "timestamp"), ("n_tz", "timestamptz"), + ("a_ts", "timestamp"), ("a_tz", "timestamptz")): + _run(dsn, f"CREATE TABLE {t} (t {typ}) USING pgcolumnar") + src = "naive" if t.startswith("n_") else "adjusted" + _run(dsn, f"SELECT pgcolumnar.import_parquet('{t}', '{w}/{src}.parquet')") + for t in ("n_ts", "n_tz", "a_ts", "a_tz"): + r[t] = _value(dsn, f"SELECT t::text FROM {t}") + r["n_tz cast"] = _value(dsn, "SELECT ((SELECT t FROM n_tz) = " + "'2024-06-01 12:00'::timestamp::timestamptz)::text") + r["a_ts cast"] = _value(dsn, "SELECT ((SELECT t FROM a_ts) = " + "'2024-06-01 12:00+00'::timestamptz::timestamp)::text") + _run(dsn, "CREATE TABLE rt_src (ts timestamp, tz timestamptz) USING pgcolumnar") + _run(dsn, "INSERT INTO rt_src VALUES ('2024-06-01 12:00', '2024-06-01 12:00')") + _run(dsn, "CREATE TABLE rt_dst (ts timestamp, tz timestamptz) USING pgcolumnar") + _run(dsn, f"SELECT pgcolumnar.export_parquet('rt_src', '{w}/rt.parquet')") + _run(dsn, f"SELECT pgcolumnar.import_parquet('rt_dst', '{w}/rt.parquet')") + r["round trip"] = _value(dsn, "SELECT ts::text || ' | ' || tz::text FROM rt_dst") + _run(dsn, f"CREATE FOREIGN TABLE f_gap (t timestamptz) SERVER pq OPTIONS (path '{w}/gap.parquet')") + r["gap instants"] = _value(dsn, "SELECT string_agg(to_char(t AT TIME ZONE 'UTC', 'HH24:MI'), ',' " + "ORDER BY t) FROM f_gap") + r["gap count"] = _value(dsn, "SELECT count(*)::text FROM f_gap WHERE t > '2024-03-10 07:15+00'") + _run(dsn, f"CREATE FOREIGN TABLE f_naive (t timestamptz) SERVER pq OPTIONS (path '{w}/naive.parquet')") + r["tokyo"] = _value(dsn, "SELECT t::text || ' ' || (t = '2024-06-01 12:00'::timestamp::timestamptz)::text " + "FROM f_naive", zone=TOKYO) + r["tokyo differs"] = _value(dsn, "SELECT ((SELECT t FROM f_naive) <> (SELECT t FROM n_tz))::text", + zone=TOKYO) + _run(dsn, f"CREATE FOREIGN TABLE f_fb (t timestamptz) SERVER pq OPTIONS (path '{w}/fallback.parquet')") + r["fall-back"] = _value(dsn, "SELECT t::text || ' ' || (t = '2024-11-03 01:30'::timestamp::timestamptz)::text " + "FROM f_fb") + _run(dsn, f"CREATE FOREIGN TABLE f_leg_ts (t timestamp) SERVER pq OPTIONS (path '{w}/legacy.parquet')") + _run(dsn, f"CREATE FOREIGN TABLE f_leg_tz (t timestamptz) SERVER pq OPTIONS (path '{w}/legacy.parquet')") + r["legacy ts"] = _value(dsn, "SELECT t::text || ' ' || (t = '2024-06-01 12:00+00'::timestamptz::timestamp)::text " + "FROM f_leg_ts") + r["legacy tz"] = _value(dsn, "SELECT t::text FROM f_leg_tz") + yield r + finally: + with psycopg.connect(pgc_cluster.dsn(), autocommit=True) as a: + a.execute(f"DROP DATABASE IF EXISTS {DB} WITH (FORCE)") + + +def _readings(request, expect): + """Decline by the shell's skip name first; only then build the fixture.""" + if expect.require_module("pyarrow.parquet", + name="pyarrow not available; Parquet timestamp zone suite needs it") is None: + return None + return request.getfixturevalue("ptz") + + +def test_ptz_pyarrow_wrote_one_naive_and_one_adjusted_file(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["tz of the files"], "None UTC", + "premise: pyarrow wrote the naive file without isAdjustedToUTC and the other with it") + + +def test_ptz_a_naive_file_into_timestamptz_is_its_session_wall_time(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["n_tz"], "2024-06-01 12:00:00-04", + "a naive file read into timestamptz is its wall time in the session zone") + + +def test_ptz_the_naive_read_equals_the_cast_from_timestamp(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["n_tz cast"], "true", "and that is what PostgreSQL's cast from timestamp gives") + + +def test_ptz_an_adjusted_file_into_timestamp_is_the_session_wall_time(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["a_ts"], "2024-06-01 08:00:00", + "an adjusted file read into timestamp is the session-zone wall time of its instant") + + +def test_ptz_the_adjusted_read_equals_the_cast_from_timestamptz(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["a_ts cast"], "true", "and that is what PostgreSQL's cast from timestamptz gives") + + +def test_ptz_a_naive_file_into_timestamp_keeps_its_wall_time(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["n_ts"], "2024-06-01 12:00:00", + "control: a naive file read into timestamp keeps its wall time") + + +def test_ptz_an_adjusted_file_into_timestamptz_keeps_its_instant(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["a_tz"], "2024-06-01 08:00:00-04", + "control: an adjusted file read into timestamptz keeps its instant") + + +def test_ptz_an_export_import_round_trip_keeps_both_types(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["round trip"], "2024-06-01 12:00:00 | 2024-06-01 12:00:00-04", + "an export and import round trip keeps timestamp and timestamptz in a non-UTC session") + + +def test_ptz_the_gap_wall_times_become_their_instants(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["gap instants"], "07:00,07:00,07:30", + "the gap file's wall times 02:00, 02:30 and 03:00 become, sorted, the instants " + "07:00Z, 07:00Z and 07:30Z") + + +def test_ptz_a_gap_spanning_group_is_not_skipped(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["gap count"], "1", + "a row group spanning a spring-forward gap is not skipped on its converted min and max") + + +def test_ptz_the_legacy_fixture_carries_only_the_converted_type(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["legacy premise"], "TIMESTAMP_MICROS True", + "premise: the legacy fixture carries the TIMESTAMP_MICROS converted type and no logical type") + + +def test_ptz_a_naive_file_in_tokyo_is_its_wall_time_there(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["tokyo"], "2024-06-01 12:00:00+09 true", + "a naive file read into timestamptz in Tokyo is its wall time there, as the cast gives") + + +def test_ptz_the_tokyo_reading_differs_from_new_york(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["tokyo differs"], "true", + "and the Tokyo reading is a different instant from the New York one") + + +def test_ptz_a_fall_back_wall_time_is_what_the_cast_gives(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["fall-back"], "2024-11-03 01:30:00-05 true", + "a naive wall time in the fall-back hour read into timestamptz is what the cast gives") + + +def test_ptz_a_legacy_file_into_timestamp_is_the_session_wall_time(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["legacy ts"], "2024-06-01 08:00:00 true", + "a legacy TIMESTAMP_MICROS file read into timestamp is the session-zone wall time of its instant") + + +def test_ptz_a_legacy_file_into_timestamptz_keeps_its_instant(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["legacy tz"], "2024-06-01 08:00:00-04", + "a legacy TIMESTAMP_MICROS file read into timestamptz keeps its instant") diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index c3db03fd1..5d7c24419 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -261,6 +261,7 @@ SUITES=( parquet_level_width parquet_nested parquet_nested_import + parquet_timestamp_zone partition_lifecycle partition_rows partman_columnar From a4ea17561566950403c9b80847b7332a14f2bab6 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 22 Oct 2026 14:43:45 +0000 Subject: [PATCH 2/4] fix: Parquet row-group skipping uses a constant of another type in the operator family (#1712) The foreign table's group-exclusion check refused any constant whose type was not the column's. A bare integer literal is int4, so the most ordinary filter on a bigint column, `WHERE id > 150000`, read every row group; only `150000::int8` skipped. Answers were correct; reads were not saved. The check now compares the statistics with the constant through the operator family's comparison function for (column type, constant type), as the native zone-map path already did (OffgridwithJD's measurement on #1712). A family with no such function means no skip. Two details the old code would have got wrong for a cross-type constant: the NaN check read the constant as the column's float type (it now reads it by its own type), and equality called the comparison with the constant first (the column's value is now always the first argument). New suite test/parquet_skip_cross_type.sh and its twin, 9 checks, over a 200,000-row file in 4 row groups: int4 constants on an int8 column (>, =, and on the left), a negative float4 constant on a float8 column, the answers against the same-type query, and a float4 NaN that must still find the NaN row its group's statistics leave out. parquet_export_stats measured the old refusal in six arms ("a cross-type constant skips nothing", and five like it). Those assertions are now wrong by design: each is renamed for what it asserts, the skip its same-type twin gets, keeps its ledger row, and is re-dated by the pre-#1712 reader, which fails exactly those six in both harnesses. Three soundness arms are added for the changed predicates that had none ([i8 < 5::int], [i8 < 5], [f4 < 0.4]); every oracle arm passes. docs/limitations.md states the new rule. Measured on PG17: - Red: the pre-#1712 reader fails the four cross-type skip arms of the new suite, in shell and twin. - Green: 9/9 shell and twin on each of PG15 to PG19; parquet_export_stats 165/165 shell and twin on each of PG15 to PG19. - Mutants, the same names in shell and twin: the comparison called with the constant first (4, the same-type control among them); the column type's own function used for a constant of another type (the negative float4 arm); the NaN constant read as the column's type (the NaN arm). - 47 parquet, iceberg and arrow suites on PG17: only parquet_export_stats's six refusal arms failed, before they were rewritten; 1044 checks. - docs_style passes. Guards 476 passed. cluster_tests 2763 -> 2775 (collected 2775). Ledger 6180 + 12 = 6192 rows, 3886 -> 3892 never, 2294 -> 2300 dated; the gate passes, ceiling 102. - Rebased onto #1736 at a93f14bf, the registry re-derived there: cluster_tests 3240 -> 3252 (collected 3252). Ledger 6951 + 12 = 6963 rows, 4140 -> 4146 never, 2811 -> 2817 dated. The gate is run on the commit after this one. Stacked on #1736 (and #1707), which touch the same function. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 + docs/limitations.md | 8 +- src/columnar_parquet_reader.c | 58 +++++-- test/check_ledger.tsv | 24 ++- test/check_ledger_budget.txt | 27 ++- test/parquet_export_stats.sh | 48 +++--- test/parquet_skip_cross_type.sh | 77 +++++++++ test/pytest/TESTS.md | 33 +++- test/pytest/expected_tests.txt | 6 +- test/pytest/test_compare_to_bash.py | 1 + test/pytest/test_parquet_export_stats.py | 40 +++-- test/pytest/test_parquet_skip_cross_type.py | 175 ++++++++++++++++++++ test/run_all_versions.sh | 1 + 13 files changed, 437 insertions(+), 68 deletions(-) create mode 100755 test/parquet_skip_cross_type.sh create mode 100644 test/pytest/test_parquet_skip_cross_type.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 1db70a33d..20b087f0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -444,6 +444,13 @@ true until the next version shipped. ended the cleanup early: the segments stayed pinned until the server restarted, and the second cancel was reported instead of the first. The cleanup now finishes whatever arrives, in the time a single cancel already took (#1745). +- **Parquet row-group skipping now uses a constant of another integer or float + type.** A bare integer literal is `int4`, so `WHERE id > 150000` on a `bigint` + column of a Parquet foreign table read every row group; only `150000::int8` + skipped. The check now compares through the operator family's cross-type + comparison, as the native columnar scan already did. Answers were always correct; + this only reads less (#1712). `test/parquet_skip_cross_type.sh` and + `test_parquet_skip_cross_type.py`. - **`export_parquet` no longer marks a `timestamp` column as a UTC instant.** Both `timestamp` and `timestamptz` were written as the converted type `TIMESTAMP_MICROS`, which the Parquet spec defines as adjusted to UTC. Every other reader (pyarrow, diff --git a/docs/limitations.md b/docs/limitations.md index d64ba6fe0..2b657adc0 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -1091,9 +1091,11 @@ correct rows; these are the conditions under which it can skip at all: bytea, uuid, and boolean columns are filtered but never skipped, whatever their statistics. A `numeric` column follows its storage: one written as an INT32 or INT64 DECIMAL does skip, one written as a byte-array DECIMAL does not. -- The constant's type must match the column's type exactly. A cross-type - comparison such as `ts >= DATE '2026-01-01'` against a `timestamp` column, or - `bigint_col > 5::int`, does not skip. +- The constant may be of another type in the operator's family. So + `bigint_col > 5` skips, though `5` is an `int4`. So do `real_col < 0.4` and + `ts >= DATE '2026-01-01'` on a `timestamp` column. A comparison that casts the + column instead does not skip. An example is `bigint_col < 5.0`, where `5.0` is a + `numeric`. - The row group's statistics must carry both a minimum and a maximum, and the interval must not be inverted. Two cases decode to an interval that the reader does not trust for skipping. The first is an unsigned Parquet column that diff --git a/src/columnar_parquet_reader.c b/src/columnar_parquet_reader.c index 16d0a5991..6a6f1c363 100644 --- a/src/columnar_parquet_reader.c +++ b/src/columnar_parquet_reader.c @@ -34,6 +34,7 @@ #include "fmgr.h" #include "funcapi.h" +#include "access/nbtree.h" #include "access/htup_details.h" #include "common/int.h" #include "access/relation.h" @@ -4534,9 +4535,11 @@ pqfdw_top_for_attno(ImpTop *tops, int ntops, int attno0) */ static bool pqfdw_clause_excludes_group(ImpLeaf *leaf, PqChunk *ch, int strategy, - bool varLeft, Const *con) + bool varLeft, Const *con, Oid opfamily) { TypeCacheEntry *tce; + FmgrInfo cross; + FmgrInfo *vsConst; const uint8 *p; bool ok; Datum minD; @@ -4549,7 +4552,7 @@ pqfdw_clause_excludes_group(ImpLeaf *leaf, PqChunk *ch, int strategy, if (phys != PQ_INT32 && phys != PQ_INT64 && phys != PQ_FLOAT && phys != PQ_DOUBLE) return false; - if (con->consttype != leaf->plan.typid || con->constisnull) + if (con->constisnull) return false; /* * A wall time is not monotonic in its instant across a spring-forward gap, so @@ -4562,6 +4565,27 @@ pqfdw_clause_excludes_group(ImpLeaf *leaf, PqChunk *ch, int strategy, if (!OidIsValid(tce->cmp_proc_finfo.fn_oid)) return false; + /* + * The statistics are compared with the constant through the operator family's + * comparison function for (column type, constant type), as the native zone-map + * path does (#1712). A bare integer literal is int4, so requiring the column's + * own type left `WHERE id > 150000` on a bigint column skipping nothing. A + * family with no such function, or no family, means no skip. + */ + if (con->consttype == leaf->plan.typid) + vsConst = &tce->cmp_proc_finfo; + else + { + Oid proc = OidIsValid(opfamily) + ? get_opfamily_proc(opfamily, leaf->plan.typid, con->consttype, BTORDER_PROC) + : InvalidOid; + + if (!OidIsValid(proc)) + return false; + fmgr_info(proc, &cross); + vsConst = ✗ + } + p = ch->stat_min; minD = plain_value_to_datum(&leaf->plan, &p, p + ch->stat_min_len, false, &ok); if (!ok) @@ -4628,9 +4652,12 @@ pqfdw_clause_excludes_group(ImpLeaf *leaf, PqChunk *ch, int strategy, ? (float8) DatumGetFloat4(minD) : DatumGetFloat8(minD); float8 hi = (phys == PQ_FLOAT) ? (float8) DatumGetFloat4(maxD) : DatumGetFloat8(maxD); - float8 c = (phys == PQ_FLOAT) + /* the constant is read by ITS type, which may not be the column's */ + float8 c = (con->consttype == FLOAT4OID) ? (float8) DatumGetFloat4(con->constvalue) - : DatumGetFloat8(con->constvalue); + : (con->consttype == FLOAT8OID) + ? DatumGetFloat8(con->constvalue) + : 0.0; if (isnan(lo) || isnan(hi)) return false; @@ -4641,22 +4668,29 @@ pqfdw_clause_excludes_group(ImpLeaf *leaf, PqChunk *ch, int strategy, return false; } + /* + * The column's value is always the FIRST argument: a cross-type comparison + * function is defined for (column type, constant type) only. + */ +#define PQ_VS(a) \ + DatumGetInt32(FunctionCall2Coll(vsConst, InvalidOid, (a), con->constvalue)) + switch (strategy) { case BTLessStrategyNumber: /* col < const: skip if min >= const */ - return PQ_CMP(minD, con->constvalue) >= 0; + return PQ_VS(minD) >= 0; case BTLessEqualStrategyNumber: /* col <= const: skip if min > const */ - return PQ_CMP(minD, con->constvalue) > 0; + return PQ_VS(minD) > 0; case BTEqualStrategyNumber: /* col = const: skip if const outside [min,max] */ - return PQ_CMP(con->constvalue, minD) < 0 || - PQ_CMP(con->constvalue, maxD) > 0; + return PQ_VS(minD) > 0 || PQ_VS(maxD) < 0; case BTGreaterEqualStrategyNumber: /* col >= const: skip if max < const */ - return PQ_CMP(maxD, con->constvalue) < 0; + return PQ_VS(maxD) < 0; case BTGreaterStrategyNumber: /* col > const: skip if max <= const */ - return PQ_CMP(maxD, con->constvalue) <= 0; + return PQ_VS(maxD) <= 0; default: return false; } +#undef PQ_VS #undef PQ_CMP } @@ -4701,6 +4735,7 @@ pqfdw_compute_skip(ForeignScanState *node, PqFile *pf, List *interp; ListCell *ic; int strategy = 0; + Oid opfamily = InvalidOid; if (!IsA(op, OpExpr) || list_length(op->args) != 2) continue; @@ -4741,6 +4776,7 @@ pqfdw_compute_skip(ForeignScanState *node, PqFile *pf, if (s >= BTLessStrategyNumber && s <= BTGreaterStrategyNumber) { strategy = s; + opfamily = o->opfamily_id; break; } } @@ -4749,7 +4785,7 @@ pqfdw_compute_skip(ForeignScanState *node, PqFile *pf, if (pqfdw_clause_excludes_group(&leaves[top->firstLeaf], &pf->rgs[rg].chunks[top->firstLeaf], - strategy, varLeft, con)) + strategy, varLeft, con, opfamily)) { skipGroup[rg] = true; break; diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index ace68abc1..b1e1daace 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -5623,7 +5623,7 @@ parquet_count_bounds parquet_count_bounds the num_rows rejection names num_rows parquet_count_bounds parquet_count_bounds the num_values rejection names num_values in its detail 15;16;17;18;19 never - parquet_count_bounds parquet_count_bounds the server is still up after the honest import 15;16;17;18;19 never - parquet_export_stats parquet_export_stats a bigint predicate skips 15;16;17;18;19 2026-10-21 columnar_parquet.c: pqstats_reset leaves hasBounds set, so each later group's bounds start from the reset 0;columnar_parquet.c: write_statistics writes no min_value or max_value for any type;columnar_parquet_reader.c: col < const skips a group on max >= const instead of min >= const -parquet_export_stats parquet_export_stats a cross-type constant skips nothing, on the identical predicate 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: row-group skipping no longer refuses a constant of another type +parquet_export_stats parquet_export_stats a cross-type constant skips as the matching one does, on the identical predicate 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the exact-type refusal before #1712 restored, so a constant of another type skips nothing parquet_export_stats parquet_export_stats a date predicate skips 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds;columnar_parquet.c: pqstats_reset leaves hasBounds set, so each later group's bounds start from the reset 0;columnar_parquet.c: write_statistics writes no min_value or max_value for any type;columnar_parquet_reader.c: col < const skips a group on max >= const instead of min >= const parquet_export_stats parquet_export_stats a decimal-point literal casts the column, not the constant 15;16;17;18;19 never - parquet_export_stats parquet_export_stats a float >= predicate above every bound still skips nothing 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: row-group skipping no longer refuses > and >= on a float column @@ -5649,8 +5649,8 @@ parquet_export_stats parquet_export_stats control: the parser reports the deprec parquet_export_stats parquet_export_stats control: the quoted form casts the constant to the column's type 15;16;17;18;19 never - parquet_export_stats parquet_export_stats control: the same column skips when the constant matches its type 15;16;17;18;19 2026-10-21 columnar_parquet.c: pqstats_reset leaves hasBounds set, so each later group's bounds start from the reset 0;columnar_parquet.c: write_statistics writes no min_value or max_value for any type parquet_export_stats parquet_export_stats control: the unadorned form resolves to the column's type instead 15;16;17;18;19 never - +parquet_export_stats parquet_export_stats digits too wide for an integer are a bigint, and an int4 column skips as well 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the exact-type refusal before #1712 restored, so a constant of another type skips nothing parquet_export_stats parquet_export_stats digits too wide for an integer are a bigint, and skip 15;16;17;18;19 2026-10-21 columnar_parquet.c: pqstats_reset leaves hasBounds set, so each later group's bounds start from the reset 0;columnar_parquet.c: write_statistics writes no min_value or max_value for any type;columnar_parquet_reader.c: col < const skips a group on max >= const instead of min >= const -parquet_export_stats parquet_export_stats digits too wide for an integer are a bigint, so an int4 column refuses 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: row-group skipping no longer refuses a constant of another type parquet_export_stats parquet_export_stats each bucket holds its own contiguous id range 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds parquet_export_stats parquet_export_stats every ColumnOrder is TYPE_ORDER 15;16;17;18;19 2026-10-21 columnar_parquet.c: FileMetaData.column_orders not written parquet_export_stats parquet_export_stats every bound is its physical width 15;16;17;18;19 2026-10-21 columnar_parquet.c: write_statistics writes no min_value or max_value for any type @@ -5759,6 +5759,7 @@ parquet_export_stats parquet_export_stats the FDW result equals the oracle for [ parquet_export_stats parquet_export_stats the FDW result equals the oracle for [d < TIMESTAMP '2000-01-05 00:00:00'] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds parquet_export_stats parquet_export_stats the FDW result equals the oracle for [d > DATE '2060-01-01'] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds parquet_export_stats parquet_export_stats the FDW result equals the oracle for [f4 < '0.4'] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds +parquet_export_stats parquet_export_stats the FDW result equals the oracle for [f4 < 0.4] 15;16;17;18;19 never - parquet_export_stats parquet_export_stats the FDW result equals the oracle for [f4 < 1.0::real] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds;columnar_parquet_reader.c: col < const skips a group on max >= const instead of min >= const parquet_export_stats parquet_export_stats the FDW result equals the oracle for [f4 >= 1.0::real] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds parquet_export_stats parquet_export_stats the FDW result equals the oracle for [i2 < '500'] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds;columnar_parquet_reader.c: col < const skips a group on max >= const instead of min >= const @@ -5767,6 +5768,8 @@ parquet_export_stats parquet_export_stats the FDW result equals the oracle for [ parquet_export_stats parquet_export_stats the FDW result equals the oracle for [i8 < '5'] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds parquet_export_stats parquet_export_stats the FDW result equals the oracle for [i8 < 1000000000::bigint] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds;columnar_parquet_reader.c: col < const skips a group on max >= const instead of min >= const parquet_export_stats parquet_export_stats the FDW result equals the oracle for [i8 < 5000000000] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds;columnar_parquet_reader.c: col < const skips a group on max >= const instead of min >= const +parquet_export_stats parquet_export_stats the FDW result equals the oracle for [i8 < 5::int] 15;16;17;18;19 never - +parquet_export_stats parquet_export_stats the FDW result equals the oracle for [i8 < 5] 15;16;17;18;19 never - parquet_export_stats parquet_export_stats the FDW result equals the oracle for [id < -1] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds parquet_export_stats parquet_export_stats the FDW result equals the oracle for [id < -5000000000] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds parquet_export_stats parquet_export_stats the FDW result equals the oracle for [id < 5000] 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds;columnar_parquet_reader.c: col < const skips a group on max >= const instead of min >= const @@ -5779,11 +5782,11 @@ parquet_export_stats parquet_export_stats the footer carries one ColumnOrder per parquet_export_stats parquet_export_stats the four groups report four distinct id minima 15;16;17;18;19 2026-10-21 columnar_parquet.c: pqstats_reset leaves hasBounds set, so each later group's bounds start from the reset 0;columnar_parquet.c: write_statistics writes no min_value or max_value for any type parquet_export_stats parquet_export_stats the oracle holds every exported row 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds parquet_export_stats parquet_export_stats the oracle's buckets are the file's row groups 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds -parquet_export_stats parquet_export_stats the same date adorned as a timestamp does not skip 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds +parquet_export_stats parquet_export_stats the same date adorned as a timestamp skips as well 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the exact-type refusal before #1712 restored, so a constant of another type skips nothing parquet_export_stats parquet_export_stats the same date unadorned is unknown, takes the column's type and skips 15;16;17;18;19 2026-10-21 columnar_parquet.c: leaf_value_representable admits date and timestamp infinities, so they reach the data and the bounds;columnar_parquet.c: pqstats_reset leaves hasBounds set, so each later group's bounds start from the reset 0;columnar_parquet.c: write_statistics writes no min_value or max_value for any type;columnar_parquet_reader.c: col < const skips a group on max >= const instead of min >= const -parquet_export_stats parquet_export_stats the same digits unquoted are an integer, and skip nothing 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: row-group skipping no longer refuses a constant of another type -parquet_export_stats parquet_export_stats the same digits unquoted are an integer, and skip nothing (int2) 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: row-group skipping no longer refuses a constant of another type -parquet_export_stats parquet_export_stats the same literal unquoted is double precision, and skips nothing 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: row-group skipping no longer refuses a constant of another type +parquet_export_stats parquet_export_stats the same digits unquoted are an integer, and skip as well 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the exact-type refusal before #1712 restored, so a constant of another type skips nothing +parquet_export_stats parquet_export_stats the same digits unquoted are an integer, and skip as well (int2) 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the exact-type refusal before #1712 restored, so a constant of another type skips nothing +parquet_export_stats parquet_export_stats the same literal unquoted is double precision, and skips as well 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the exact-type refusal before #1712 restored, so a constant of another type skips nothing parquet_level_width parquet_level_width a non-positive maximum returns before shifting at all 15;16;17;18;19 2026-10-21 columnar_parquet_format.h: pq_bits_for's non-positive guard is commented out parquet_level_width parquet_level_width and both callers reach the shared one 15;16;17;18;19 never - parquet_level_width parquet_level_width and no signed 1 << remains in it 15;16;17;18;19 2026-10-21 columnar_parquet_format.h: pq_bits_for shifts a signed 1 @@ -5804,6 +5807,15 @@ parquet_nested_import parquet_nested_import nested import round-trips (arrays + parquet_nested_import parquet_nested_import pyarrow import rows 15;16;17;18;19 never - parquet_nested_import parquet_nested_import pyarrow nested file matches oracle 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: an empty Parquet list is read back as a NULL array parquet_nested_import parquet_nested_import round-trips after delete 15;16;17;18;19 2026-10-21 columnar_parquet_reader.c: an empty Parquet list is read back as a NULL array +parquet_skip_cross_type parquet_skip_cross_type a constant on the left of the operator is compared the same way 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the comparison function is called with the constant first, not the column's value +parquet_skip_cross_type parquet_skip_cross_type a float4 NaN constant still finds the float8 column's NaN row 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the float NaN check reads the constant as the column's float type +parquet_skip_cross_type parquet_skip_cross_type a float4 constant below every row skips all 4 row groups of the float8 column 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the column type's own comparison function is used for a constant of another type +parquet_skip_cross_type parquet_skip_cross_type an int4 constant in an equality on the int8 column skips 3 of the 4 row groups 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the comparison function is called with the constant first, not the column's value +parquet_skip_cross_type parquet_skip_cross_type an int4 constant on the int8 column skips 3 of the 4 row groups 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the comparison function is called with the constant first, not the column's value +parquet_skip_cross_type parquet_skip_cross_type and returns the rows the float8 constant returns 15;16;17;18;19 never - +parquet_skip_cross_type parquet_skip_cross_type and returns the rows the int8 constant returns 15;16;17;18;19 never - +parquet_skip_cross_type parquet_skip_cross_type control: an int8 constant on the int8 column skips 3 of the 4 row groups 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the comparison function is called with the constant first, not the column's value +parquet_skip_cross_type parquet_skip_cross_type premise: the file holds 4 row groups (pyarrow's own count) 15;16;17;18;19 never - parquet_timestamp_zone parquet_timestamp_zone a legacy TIMESTAMP_MICROS file read into timestamp is the session-zone wall time of its instant 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a column with only a legacy TIMESTAMP_* converted type reads as not UTC-adjusted parquet_timestamp_zone parquet_timestamp_zone a legacy TIMESTAMP_MICROS file read into timestamptz keeps its instant 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a column with only a legacy TIMESTAMP_* converted type reads as not UTC-adjusted parquet_timestamp_zone parquet_timestamp_zone a naive file read into timestamptz in Tokyo is its wall time there, as the cast gives 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a naive column read into timestamptz is converted in America/New_York, not the session zone diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index b304ad7de..51327c27c 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -7074,4 +7074,29 @@ suites_not_covered 27 # never 4535 + 3 = 4538 # dated 3445 + 13 = 3458 # 4538 + 3458 == 7996 -checks_never_observed_red 4538 +# +# #1712, parquet_skip_cross_type seeded, with its twin (jdatcmd): 4538 -> 4541. A new suite, +# so suites_not_covered does not move: it is registered and covered in the same change. It +# enters with 9 checks from five-major runs (all green on each of 15-19, shell and twin +# alike). 6 are dated by three mutations, each reddening the same names in both twins: the +# comparison function called with the constant first (4), the column type's own function +# used for a constant of another type (the negative float4 arm, 1), and the NaN check +# reading the constant as the column's type (1). The premise and the two answer controls +# arrive never. +# +# ledger rows 7996 + 9 = 8005 +# never 4538 + 3 = 4541 +# dated 3458 + 6 = 3464 +# 4541 + 3464 == 8005 +# +# #1712, parquet_export_stats follows the new rule (jdatcmd): 4541 -> 4544. Six arms measured +# the refusal of a cross-type constant (it skipped nothing); each is renamed for what it now +# asserts, the skip its same-type twin gets, and keeps its row's history. Three soundness +# arms are added for the predicates that had none, [i8 < 5::int], [i8 < 5] and [f4 < 0.4], +# and arrive never, like the other oracle arms. suites_not_covered does not move. +# +# ledger rows 8005 + 3 = 8008 +# never 4541 + 3 = 4544 +# dated 3464 + 0 = 3464 +# 4544 + 3464 == 8008 +checks_never_observed_red 4544 diff --git a/test/parquet_export_stats.sh b/test/parquet_export_stats.sh index e2e00ea68..0fa69141c 100755 --- a/test/parquet_export_stats.sh +++ b/test/parquet_export_stats.sh @@ -612,7 +612,11 @@ check_num "a date predicate skips" \ "$(skipped_for "WHERE d < DATE '2000-01-05'")" "3" check_num "a timestamp predicate skips" \ "$(skipped_for "WHERE ts < TIMESTAMP '2000-01-01 00:30:00'")" "3" -# Documented refusals, reachable from our own exported files for the first time. +# CROSS-TYPE CONSTANTS. These arms measured a documented refusal: a constant whose +# type was not the column's skipped nothing. Since #1712 the reader compares through +# the operator family's cross-type function, so each now skips exactly as its +# same-type twin beside it does, and the soundness arms below cover every one. +# The comments that follow describe how each pair was built. # # Each runs beside a predicate that differs in ONE respect and does skip. Without # that pair the checks are worthless: every group's max(i8) exceeds 5 and every @@ -621,8 +625,8 @@ check_num "a timestamp predicate skips" \ # not evidence of a refusal. check_num "control: the same column skips when the constant matches its type" \ "$(skipped_for "WHERE i8 < 5::bigint")" "$NGROUPS" -check_num "a cross-type constant skips nothing, on the identical predicate" \ - "$(skipped_for "WHERE i8 < 5::int")" "0" +check_num "a cross-type constant skips as the matching one does, on the identical predicate" \ + "$(skipped_for "WHERE i8 < 5::int")" "$NGROUPS" check_num "control: a float < predicate below every bound skips" \ "$(skipped_for "WHERE f4 < 0.4::real")" "$NGROUPS" # The reader refuses > and >= on a float column outright, because Parquet @@ -644,8 +648,8 @@ check_num "a float >= predicate above every bound still skips nothing" \ # constant cannot produce, which is what the pair is for. check_num "a quoted literal takes a bigint column's type and skips" \ "$(skipped_for "WHERE i8 < '5'")" "$NGROUPS" -check_num "the same digits unquoted are an integer, and skip nothing" \ - "$(skipped_for "WHERE i8 < 5")" "0" +check_num "the same digits unquoted are an integer, and skip as well" \ + "$(skipped_for "WHERE i8 < 5")" "$NGROUPS" check_num "digits too wide for an integer are a bigint, and skip" \ "$(skipped_for "WHERE i8 < 5000000000")" "3" # A decimal-point literal is a `numeric`, and PostgreSQL resolves @@ -665,12 +669,12 @@ check_num "control: the quoted form casts the constant to the column's type" \ "$(filter_has "WHERE i8 < '5'" '::bigint')" "1" check_num "a quoted literal takes a real column's type and skips" \ "$(skipped_for "WHERE f4 < '0.4'")" "$NGROUPS" -check_num "the same literal unquoted is double precision, and skips nothing" \ - "$(skipped_for "WHERE f4 < 0.4")" "0" +check_num "the same literal unquoted is double precision, and skips as well" \ + "$(skipped_for "WHERE f4 < 0.4")" "$NGROUPS" check_num "a quoted literal takes a smallint column's type and skips" \ "$(skipped_for "WHERE i2 < '500'")" "1" -check_num "the same digits unquoted are an integer, and skip nothing (int2)" \ - "$(skipped_for "WHERE i2 < 500")" "0" +check_num "the same digits unquoted are an integer, and skip as well (int2)" \ + "$(skipped_for "WHERE i2 < 500")" "1" # ADORNED is not the same as quoted. A literal that names its own type is that # type, not `unknown`, so it does not take the column's and the exact-type @@ -682,16 +686,12 @@ check_num "an adorned literal of the column's own type skips" \ "$(skipped_for "WHERE d < DATE '2000-01-05'")" "3" check_num "the same date unadorned is unknown, takes the column's type and skips" \ "$(skipped_for "WHERE d < '2000-01-05'")" "3" -check_num "the same date adorned as a timestamp does not skip" \ - "$(skipped_for "WHERE d < TIMESTAMP '2000-01-05 00:00:00'")" "0" -# That last arm is a documented-behaviour guard rather than evidence, and the -# distinction is worth stating rather than leaving for a reader to discover. -# No mutation in this suite's ledger reddens it: with the reader's -# exact-constant-type refusal deleted, the timestamp datum read as a date lands -# outside this fixture's range in the direction that prunes nothing, so the arm -# still reports 0 for a second reason. What IS falsifiable is the typing the -# sentence rests on, and the planner will say it: an adorned literal keeps its -# own type beside a bare Var, while the unadorned form resolves to the column's. +check_num "the same date adorned as a timestamp skips as well" \ + "$(skipped_for "WHERE d < TIMESTAMP '2000-01-05 00:00:00'")" "3" +# The typing the pair rests on is falsifiable, and the planner will say it: an +# adorned literal keeps its own type beside a bare Var, while the unadorned form +# resolves to the column's. Since #1712 both forms skip, through different +# comparison functions. check_num "an adorned literal keeps its own type, not the column's" \ "$(filter_has "WHERE d < TIMESTAMP '2000-01-05 00:00:00'" '::timestamp')" "1" check_num "control: the unadorned form resolves to the column's type instead" \ @@ -704,8 +704,8 @@ check_num "control: the unadorned form resolves to the column's type instead" \ # and the other prunes none. check_num "control: a digit string that fits an integer prunes every group" \ "$(skipped_for "WHERE id < -1")" "$NGROUPS" -check_num "digits too wide for an integer are a bigint, so an int4 column refuses" \ - "$(skipped_for "WHERE id < -5000000000")" "0" +check_num "digits too wide for an integer are a bigint, and an int4 column skips as well" \ + "$(skipped_for "WHERE id < -5000000000")" "$NGROUPS" # Soundness. A skipped group emits nothing, and the executor's recheck cannot # recover a row that never arrived, so every predicate above must return exactly @@ -755,6 +755,12 @@ fdw_vs_oracle "the FDW result equals the oracle for [id < -5000000000]" \ "id < -5000000000" fdw_vs_oracle "the FDW result equals the oracle for [alln IS NULL]" \ "alln IS NULL" +fdw_vs_oracle "the FDW result equals the oracle for [i8 < 5::int]" \ + "i8 < 5::int" +fdw_vs_oracle "the FDW result equals the oracle for [i8 < 5]" \ + "i8 < 5" +fdw_vs_oracle "the FDW result equals the oracle for [f4 < 0.4]" \ + "f4 < 0.4" # ---- the other direction: the instrument can report a bound that IS there --- # diff --git a/test/parquet_skip_cross_type.sh b/test/parquet_skip_cross_type.sh new file mode 100755 index 000000000..3f61c1ada --- /dev/null +++ b/test/parquet_skip_cross_type.sh @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +# +# Parquet row-group skipping uses a constant of another type in the column's +# operator family (#1712). +# +# The foreign table's group-exclusion check refused any constant whose type was +# not the column's. A bare integer literal is int4, so the most ordinary filter +# on a bigint column, `WHERE id > 150000`, read every row group; only +# `150000::int8` skipped. The same held for a float4 constant on a float8 column. +# The native columnar path already compares through the operator family's +# cross-type comparison function, and this suite asks the same of Parquet. +# +# A file of 200,000 ascending rows in 4 row groups of 50,000. Each arm reads the +# skip count from EXPLAIN (ANALYZE) and, beside it, the answer, compared with the +# same query written with a constant of the column's own type. +# +# Usage: test/parquet_skip_cross_type.sh [PG_CONFIG] + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +if ! python3 -c 'import pyarrow.parquet' 2>/dev/null; then + pgc_skip pyarrow "pyarrow not available; Parquet cross-type skip suite needs it" +fi + +W="$PGC_WORKDIR" +NGROUPS="$(python3 - "$W" <<'PY' +import sys, pyarrow as pa, pyarrow.parquet as pq +w = sys.argv[1] +ids = list(range(200000)) +pq.write_table(pa.table({"id": pa.array(ids, pa.int64()), + "f": pa.array([float(i) for i in ids], pa.float64())}), + f"{w}/xt.parquet", row_group_size=50000) +# NaN sits in the second group; Parquet statistics leave it out of min and max +pq.write_table(pa.table({"f": pa.array([1.0, 2.0, 3.0, float("nan")], pa.float64())}), + f"{w}/xn.parquet", row_group_size=2) +print(pq.ParquetFile(f"{w}/xt.parquet").metadata.num_row_groups) +PY +)" +chmod 644 "$W/xt.parquet" "$W/xn.parquet" +psql_run "CREATE SERVER pq FOREIGN DATA WRAPPER pgcolumnar_parquet; + CREATE FOREIGN TABLE xt (id int8, f float8) SERVER pq OPTIONS (path '$W/xt.parquet'); + CREATE FOREIGN TABLE xn (f float8) SERVER pq OPTIONS (path '$W/xn.parquet');" + +# skipped QUERY -> the "Row Groups Skipped" count EXPLAIN (ANALYZE) reports, or the error +skipped() { + local out + out="$(q "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) $1" 2>&1)" + grep -oE 'Row Groups Skipped: [0-9]+' <<<"$out" | grep -oE '[0-9]+$' || echo "no reading: $(head -1 <<<"$out")" +} + +check_num "premise: the file holds 4 row groups (pyarrow's own count)" "$NGROUPS" "4" +check_num "control: an int8 constant on the int8 column skips 3 of the 4 row groups" \ + "$(skipped "SELECT count(*) FROM xt WHERE id > 150000::int8")" "3" +check_num "an int4 constant on the int8 column skips 3 of the 4 row groups" \ + "$(skipped "SELECT count(*) FROM xt WHERE id > 150000")" "3" +check_num "and returns the rows the int8 constant returns" \ + "$(q "SELECT count(*) FROM xt WHERE id > 150000;")" "$(q "SELECT count(*) FROM xt WHERE id > 150000::int8;")" +check_num "an int4 constant in an equality on the int8 column skips 3 of the 4 row groups" \ + "$(skipped "SELECT count(*) FROM xt WHERE id = 150001")" "3" +check_num "a constant on the left of the operator is compared the same way" \ + "$(skipped "SELECT count(*) FROM xt WHERE 150000 < id")" "3" +# `<` because the float rule never skips on `>` or `>=` (a NaN row satisfies them +# and sits outside min/max), and a NEGATIVE constant because a float4 misread as +# float8 is a tiny positive number: then group 0 (min 0.0) would not be skipped. +check_num "a float4 constant below every row skips all 4 row groups of the float8 column" \ + "$(skipped "SELECT count(*) FROM xt WHERE f < -1::float4")" "4" +check_num "and returns the rows the float8 constant returns" \ + "$(q "SELECT count(*) FROM xt WHERE f < 50000::float4;")" "$(q "SELECT count(*) FROM xt WHERE f < 50000::float8;")" + +# A NaN constant is read by its own type. Read as the column's float8, a float4 NaN +# is some other number, and the group holding the NaN row would be skipped. +check_num "a float4 NaN constant still finds the float8 column's NaN row" \ + "$(q "SELECT count(*) FROM xn WHERE f = 'NaN'::float4;")" "1" + +pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index d2a4db9c0..d17facb4a 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -389,6 +389,7 @@ behaviour, the source of that number is named. - [341. test_native_cancel.py: a scan's row-group load yields to statement_timeout inside the decode loop](#341-test_native_cancelpy-a-scans-row-group-load-yields-to-statement_timeout-inside-the-decode-loop) - [342. test_objstore_export_double_cancel.py: a second cancel during a remote export's cleanup keeps the export's own error](#342-test_objstore_export_double_cancelpy-a-second-cancel-during-a-remote-exports-cleanup-keeps-the-exports-own-error) - [343. test_parquet_timestamp_zone.py: a Parquet timestamp read into the other timestamp type follows PostgreSQL's casts](#343-test_parquet_timestamp_zonepy-a-parquet-timestamp-read-into-the-other-timestamp-type-follows-postgresqls-casts) +- [344. test_parquet_skip_cross_type.py: Parquet row-group skipping uses a constant of another type in the operator family](#344-test_parquet_skip_cross_typepy-parquet-row-group-skipping-uses-a-constant-of-another-type-in-the-operator-family) ## 1. How to read a test in here @@ -10372,26 +10373,26 @@ The bound and null-count oracles read the heap built from the generator, not the | `test_export_stats_date_predicate_skips` | a date predicate skips | | `test_export_stats_timestamp_predicate_skips` | a timestamp predicate skips | | `test_export_stats_matching_constant_type_skips` | control: the same column skips when the constant matches its type | -| `test_export_stats_cross_type_constant_skips_nothing` | a cross-type constant skips nothing, on the identical predicate | +| `test_export_stats_cross_type_constant_skips_like_the_matching_one` | a cross-type constant skips as the matching one does, on the identical predicate (since #1712; it skipped nothing) | | `test_export_stats_float_less_than_skips` | control: a float < predicate below every bound skips | | `test_export_stats_float_greater_equal_skips_nothing` | a float >= predicate above every bound still skips nothing | | `test_export_stats_quoted_bigint_literal_skips` | a quoted literal takes a bigint column's type and skips | -| `test_export_stats_unquoted_bigint_literal_skips_nothing` | the same digits unquoted are an integer, and skip nothing | +| `test_export_stats_unquoted_bigint_literal_skips_as_well` | the same digits unquoted are an integer, and skip as well | | `test_export_stats_wide_digits_skip_a_bigint` | digits too wide for an integer are a bigint, and skip | | `test_export_stats_decimal_literal_casts_the_column` | a decimal-point literal casts the column, not the constant | | `test_export_stats_quoted_literal_casts_the_constant` | control: the quoted form casts the constant to the column's type | | `test_export_stats_quoted_real_literal_skips` | a quoted literal takes a real column's type and skips | -| `test_export_stats_unquoted_real_literal_skips_nothing` | the same literal unquoted is double precision, and skips nothing | +| `test_export_stats_unquoted_real_literal_skips_as_well` | the same literal unquoted is double precision, and skips as well | | `test_export_stats_quoted_smallint_literal_skips` | a quoted literal takes a smallint column's type and skips | -| `test_export_stats_unquoted_smallint_literal_skips_nothing` | the same digits unquoted are an integer, and skip nothing (int2) | +| `test_export_stats_unquoted_smallint_literal_skips_as_well` | the same digits unquoted are an integer, and skip as well (int2) | | `test_export_stats_adorned_date_skips` | an adorned literal of the column's own type skips | | `test_export_stats_unadorned_date_skips` | the same date unadorned is unknown, takes the column's type and skips | -| `test_export_stats_timestamp_adorned_date_skips_nothing` | the same date adorned as a timestamp does not skip | +| `test_export_stats_timestamp_adorned_date_skips_as_well` | the same date adorned as a timestamp skips as well | | `test_export_stats_adorned_literal_keeps_its_type` | an adorned literal keeps its own type, not the column's | | `test_export_stats_unadorned_literal_takes_the_column_type` | control: the unadorned form resolves to the column's type instead | | `test_export_stats_integer_digits_prune_every_group` | control: a digit string that fits an integer prunes every group | -| `test_export_stats_int4_column_refuses_wide_digits` | digits too wide for an integer are a bigint, so an int4 column refuses | -| `test_export_stats_fdw_result_equals_the_oracle` | the FDW result equals the oracle for [P]: 20 tests, one per predicate the skip checks use | +| `test_export_stats_int4_column_skips_wide_digits` | digits too wide for an integer are a bigint, and an int4 column skips as well | +| `test_export_stats_fdw_result_equals_the_oracle` | the FDW result equals the oracle for [P]: 23 tests, one per predicate the skip checks use | | `test_export_stats_parser_finds_pyarrow_bounds` | control: the parser finds pyarrow's bounds | | `test_export_stats_parser_reports_deprecated_fields` | control: the parser reports the deprecated fields where they exist | @@ -12872,3 +12873,21 @@ The pytest twin of `test/parquet_timestamp_zone.sh` (#1706 item 2), every check | `test_ptz_a_fall_back_wall_time_is_what_the_cast_gives` | a naive wall time in the fall-back hour read into timestamptz is what the cast gives | | `test_ptz_a_legacy_file_into_timestamp_is_the_session_wall_time` | a legacy TIMESTAMP_MICROS file read into timestamp is the session-zone wall time of its instant | | `test_ptz_a_legacy_file_into_timestamptz_keeps_its_instant` | a legacy TIMESTAMP_MICROS file read into timestamptz keeps its instant | + +## 344. test_parquet_skip_cross_type.py: Parquet row-group skipping uses a constant of another type in the operator family + +The pytest twin of `test/parquet_skip_cross_type.sh` (#1712), every check by the shell's name. Over a file of 200,000 ascending rows in 4 row groups, each arm reads the skip count from EXPLAIN (ANALYZE): an int4 constant on the int8 column (greater-than, equality, and on the left of the operator), and a float4 constant below every row on the float8 column; the answers are compared with the same query in the column's own type, and a float4 NaN constant must still find a NaN row its group's statistics leave out. Before #1712 a constant of another type skipped nothing. The files are written by `python3` with pyarrow, so no test imports pyarrow. On a private database. + +### Every test + +| test | what it holds | +| --- | --- | +| `test_pxt_the_file_holds_four_row_groups` | premise: the file holds 4 row groups (pyarrow's own count) | +| `test_pxt_an_int8_constant_skips_three_groups` | control: an int8 constant on the int8 column skips 3 of the 4 row groups | +| `test_pxt_an_int4_constant_skips_three_groups` | an int4 constant on the int8 column skips 3 of the 4 row groups | +| `test_pxt_the_int4_constant_returns_the_int8_rows` | and returns the rows the int8 constant returns | +| `test_pxt_an_int4_equality_skips_three_groups` | an int4 constant in an equality on the int8 column skips 3 of the 4 row groups | +| `test_pxt_a_constant_on_the_left_is_compared_the_same_way` | a constant on the left of the operator is compared the same way | +| `test_pxt_a_float4_constant_below_every_row_skips_all_groups` | a float4 constant below every row skips all 4 row groups of the float8 column -- negative, because a float4 misread as float8 is a tiny positive number | +| `test_pxt_the_float4_constant_returns_the_float8_rows` | and returns the rows the float8 constant returns | +| `test_pxt_a_float4_nan_still_finds_the_nan_row` | a float4 NaN constant still finds the float8 column's NaN row | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 5e30d3814..1b54a4ae5 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -1212,4 +1212,8 @@ guard_tests 839 # one structural test, by collection over the cluster files. # cluster_tests 4231 -> 4249 for #1706: test_parquet_timestamp_zone.py, the twin of parquet_timestamp_zone.sh # (16 tests, one per check), and two mixed-kind hour() arms in test_iceberg_fdw.py, by collection. -cluster_tests 4249 +# cluster_tests 4249 -> 4258 for #1712's test_parquet_skip_cross_type.py, the twin of parquet_skip_cross_type.sh: +# 9 tests, one per check, by collection over the cluster files. +# cluster_tests 4258 -> 4261 for #1712's three soundness arms in test_parquet_export_stats.py +# (parametrized), by collection over the cluster files. +cluster_tests 4261 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 834787edf..4093a5795 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -207,6 +207,7 @@ "cancel_decode", "bench_guards", "catalog_delete_index", + "parquet_skip_cross_type", "parquet_timestamp_zone", "catalog_filter_reads", "catalog_plan_index", diff --git a/test/pytest/test_parquet_export_stats.py b/test/pytest/test_parquet_export_stats.py index a980ce862..6949cd207 100644 --- a/test/pytest/test_parquet_export_stats.py +++ b/test/pytest/test_parquet_export_stats.py @@ -144,6 +144,9 @@ ("id < -1", "the FDW result equals the oracle for [id < -1]"), ("id < -5000000000", "the FDW result equals the oracle for [id < -5000000000]"), ("alln IS NULL", "the FDW result equals the oracle for [alln IS NULL]"), + ("i8 < 5::int", "the FDW result equals the oracle for [i8 < 5::int]"), + ("i8 < 5", "the FDW result equals the oracle for [i8 < 5]"), + ("f4 < 0.4", "the FDW result equals the oracle for [f4 < 0.4]"), ] @@ -745,9 +748,10 @@ def test_export_stats_matching_constant_type_skips(es, expect): "control: the same column skips when the constant matches its type") -def test_export_stats_cross_type_constant_skips_nothing(es, expect): - expect.num(es["skipped"]["WHERE i8 < 5::int"], 0, - "a cross-type constant skips nothing, on the identical predicate") +def test_export_stats_cross_type_constant_skips_like_the_matching_one(es, expect): + # Since #1712 a constant of another type in the family skips (was: nothing). + expect.num(es["skipped"]["WHERE i8 < 5::int"], NGROUPS, + "a cross-type constant skips as the matching one does, on the identical predicate") def test_export_stats_float_less_than_skips(es, expect): @@ -765,9 +769,9 @@ def test_export_stats_quoted_bigint_literal_skips(es, expect): "a quoted literal takes a bigint column's type and skips") -def test_export_stats_unquoted_bigint_literal_skips_nothing(es, expect): - expect.num(es["skipped"]["WHERE i8 < 5"], 0, - "the same digits unquoted are an integer, and skip nothing") +def test_export_stats_unquoted_bigint_literal_skips_as_well(es, expect): + expect.num(es["skipped"]["WHERE i8 < 5"], NGROUPS, + "the same digits unquoted are an integer, and skip as well") def test_export_stats_wide_digits_skip_a_bigint(es, expect): @@ -790,9 +794,9 @@ def test_export_stats_quoted_real_literal_skips(es, expect): "a quoted literal takes a real column's type and skips") -def test_export_stats_unquoted_real_literal_skips_nothing(es, expect): - expect.num(es["skipped"]["WHERE f4 < 0.4"], 0, - "the same literal unquoted is double precision, and skips nothing") +def test_export_stats_unquoted_real_literal_skips_as_well(es, expect): + expect.num(es["skipped"]["WHERE f4 < 0.4"], NGROUPS, + "the same literal unquoted is double precision, and skips as well") def test_export_stats_quoted_smallint_literal_skips(es, expect): @@ -800,9 +804,9 @@ def test_export_stats_quoted_smallint_literal_skips(es, expect): "a quoted literal takes a smallint column's type and skips") -def test_export_stats_unquoted_smallint_literal_skips_nothing(es, expect): - expect.num(es["skipped"]["WHERE i2 < 500"], 0, - "the same digits unquoted are an integer, and skip nothing (int2)") +def test_export_stats_unquoted_smallint_literal_skips_as_well(es, expect): + expect.num(es["skipped"]["WHERE i2 < 500"], 1, + "the same digits unquoted are an integer, and skip as well (int2)") def test_export_stats_adorned_date_skips(es, expect): @@ -815,9 +819,9 @@ def test_export_stats_unadorned_date_skips(es, expect): "the same date unadorned is unknown, takes the column's type and skips") -def test_export_stats_timestamp_adorned_date_skips_nothing(es, expect): - expect.num(es["skipped"]["WHERE d < TIMESTAMP '2000-01-05 00:00:00'"], 0, - "the same date adorned as a timestamp does not skip") +def test_export_stats_timestamp_adorned_date_skips_as_well(es, expect): + expect.num(es["skipped"]["WHERE d < TIMESTAMP '2000-01-05 00:00:00'"], 3, + "the same date adorned as a timestamp skips as well") def test_export_stats_adorned_literal_keeps_its_type(es, expect): @@ -835,9 +839,9 @@ def test_export_stats_integer_digits_prune_every_group(es, expect): "control: a digit string that fits an integer prunes every group") -def test_export_stats_int4_column_refuses_wide_digits(es, expect): - expect.num(es["skipped"]["WHERE id < -5000000000"], 0, - "digits too wide for an integer are a bigint, so an int4 column refuses") +def test_export_stats_int4_column_skips_wide_digits(es, expect): + expect.num(es["skipped"]["WHERE id < -5000000000"], NGROUPS, + "digits too wide for an integer are a bigint, and an int4 column skips as well") @pytest.mark.parametrize("pred,name", SOUND) diff --git a/test/pytest/test_parquet_skip_cross_type.py b/test/pytest/test_parquet_skip_cross_type.py new file mode 100644 index 000000000..015a80d31 --- /dev/null +++ b/test/pytest/test_parquet_skip_cross_type.py @@ -0,0 +1,175 @@ +"""Parquet row-group skipping uses a constant of another type in the column's operator +family (#1712). The pytest twin of `test/parquet_skip_cross_type.sh`, every check by the +shell's name. + +The group-exclusion check refused any constant whose type was not the column's, so +`WHERE id > 150000` (an int4 literal) on a bigint column read every row group. Over a +file of 200,000 ascending rows in 4 row groups, each arm reads the skip count from +EXPLAIN (ANALYZE): int4 constants on the int8 column (greater-than, equality, and the +constant on the left), a float4 constant on the float8 column, the answers compared with +the same query written in the column's own type, and a float4 NaN constant that must +still find the float8 column's NaN row. + +The files are written by `python3` with pyarrow, as the shell writes them, so no test +imports pyarrow: each declines by the shell's skip name first, then asks for the module +fixture. ON A PRIVATE DATABASE. +""" +import re +import subprocess +import sys + +import psycopg +import pytest + +from pgc_vacuity import query_error + +DB = "pgc_parquet_skip_cross_type" + +WRITER = r''' +import sys, pyarrow as pa, pyarrow.parquet as pq +w = sys.argv[1] +ids = list(range(200000)) +pq.write_table(pa.table({"id": pa.array(ids, pa.int64()), + "f": pa.array([float(i) for i in ids], pa.float64())}), + f"{w}/xt.parquet", row_group_size=50000) +pq.write_table(pa.table({"f": pa.array([1.0, 2.0, 3.0, float("nan")], pa.float64())}), + f"{w}/xn.parquet", row_group_size=2) +print(pq.ParquetFile(f"{w}/xt.parquet").metadata.num_row_groups) +''' + + +def _failed(exc): + return query_error(str(exc).splitlines()[0] if str(exc) else type(exc).__name__) + + +def _value(dsn, sql): + """-> the first column of the first row, or query_error() carrying the failure.""" + try: + with psycopg.connect(dsn, autocommit=True) as conn: + row = conn.execute(sql).fetchone() + except psycopg.Error as exc: + return _failed(exc) + return None if row is None else row[0] + + +def _run(dsn, sql): + """-> None, or query_error() when the statement failed.""" + try: + with psycopg.connect(dsn, autocommit=True) as conn: + conn.execute(sql) + except psycopg.Error as exc: + return _failed(exc) + return None + + +def _skipped(dsn, query): + """-> the "Row Groups Skipped" count of EXPLAIN (ANALYZE), or query_error().""" + try: + with psycopg.connect(dsn, autocommit=True) as conn: + plan = "\n".join(r[0] for r in conn.execute( + f"EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) {query}").fetchall()) + except psycopg.Error as exc: + return _failed(exc) + m = re.search(r"Row Groups Skipped: (\d+)", plan) + return int(m.group(1)) if m else query_error(f"no skip count in: {plan.splitlines()[:1]}") + + +@pytest.fixture(scope="module") +def pxt(pgc_cluster, server_dir): + with psycopg.connect(pgc_cluster.dsn(), autocommit=True) as a: + a.execute(f"DROP DATABASE IF EXISTS {DB} WITH (FORCE)") + a.execute(f"CREATE DATABASE {DB}") + try: + dsn = pgc_cluster.dsn(dbname=DB) + w = server_dir + wrote = subprocess.run([sys.executable, "-c", WRITER, str(w)], capture_output=True, text=True) + for f in ("xt", "xn"): + if (w / f"{f}.parquet").exists(): + (w / f"{f}.parquet").chmod(0o644) + r = {"groups": int(wrote.stdout.strip()) if wrote.returncode == 0 and wrote.stdout.strip().isdigit() + else query_error(f"the writer failed: {wrote.stderr.strip().splitlines()[-1:]}")} + # A failed statement here makes every later reading a query_error, which + # reddens its check by name rather than erroring the fixture. + for sql in ("CREATE EXTENSION IF NOT EXISTS pgcolumnar", + "CREATE SERVER pq FOREIGN DATA WRAPPER pgcolumnar_parquet", + f"CREATE FOREIGN TABLE xt (id int8, f float8) SERVER pq OPTIONS (path '{w}/xt.parquet')", + f"CREATE FOREIGN TABLE xn (f float8) SERVER pq OPTIONS (path '{w}/xn.parquet')"): + _run(dsn, sql) + r["int8 skip"] = _skipped(dsn, "SELECT count(*) FROM xt WHERE id > 150000::int8") + r["int4 skip"] = _skipped(dsn, "SELECT count(*) FROM xt WHERE id > 150000") + r["int4 rows"] = _value(dsn, "SELECT count(*) FROM xt WHERE id > 150000") + r["int8 rows"] = _value(dsn, "SELECT count(*) FROM xt WHERE id > 150000::int8") + r["int4 eq skip"] = _skipped(dsn, "SELECT count(*) FROM xt WHERE id = 150001") + r["left skip"] = _skipped(dsn, "SELECT count(*) FROM xt WHERE 150000 < id") + r["float4 skip"] = _skipped(dsn, "SELECT count(*) FROM xt WHERE f < -1::float4") + r["float4 rows"] = _value(dsn, "SELECT count(*) FROM xt WHERE f < 50000::float4") + r["float8 rows"] = _value(dsn, "SELECT count(*) FROM xt WHERE f < 50000::float8") + r["nan rows"] = _value(dsn, "SELECT count(*) FROM xn WHERE f = 'NaN'::float4") + yield r + finally: + with psycopg.connect(pgc_cluster.dsn(), autocommit=True) as a: + a.execute(f"DROP DATABASE IF EXISTS {DB} WITH (FORCE)") + + +def _readings(request, expect): + """Decline by the shell's skip name first; only then build the fixture.""" + if expect.require_module("pyarrow.parquet", + name="pyarrow not available; Parquet cross-type skip suite needs it") is None: + return None + return request.getfixturevalue("pxt") + + +def test_pxt_the_file_holds_four_row_groups(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["groups"], 4, "premise: the file holds 4 row groups (pyarrow's own count)") + + +def test_pxt_an_int8_constant_skips_three_groups(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["int8 skip"], 3, "control: an int8 constant on the int8 column skips 3 of the 4 row groups") + + +def test_pxt_an_int4_constant_skips_three_groups(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["int4 skip"], 3, "an int4 constant on the int8 column skips 3 of the 4 row groups") + + +def test_pxt_the_int4_constant_returns_the_int8_rows(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["int4 rows"], r["int8 rows"], "and returns the rows the int8 constant returns") + + +def test_pxt_an_int4_equality_skips_three_groups(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["int4 eq skip"], 3, + "an int4 constant in an equality on the int8 column skips 3 of the 4 row groups") + + +def test_pxt_a_constant_on_the_left_is_compared_the_same_way(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["left skip"], 3, "a constant on the left of the operator is compared the same way") + + +def test_pxt_a_float4_constant_below_every_row_skips_all_groups(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["float4 skip"], 4, + "a float4 constant below every row skips all 4 row groups of the float8 column") + + +def test_pxt_the_float4_constant_returns_the_float8_rows(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["float4 rows"], r["float8 rows"], "and returns the rows the float8 constant returns") + + +def test_pxt_a_float4_nan_still_finds_the_nan_row(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["nan rows"], 1, "a float4 NaN constant still finds the float8 column's NaN row") diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 5d7c24419..609fa168d 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -261,6 +261,7 @@ SUITES=( parquet_level_width parquet_nested parquet_nested_import + parquet_skip_cross_type parquet_timestamp_zone partition_lifecycle partition_rows From 26fb02f2036c673912f0445cbef41cfe1f1a6432 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 22 Oct 2026 19:13:28 +0000 Subject: [PATCH 3/4] fix: Parquet skipping never compares a timestamp column with a timestamptz constant (#1712) OffgridwithJD's review of #1737 measured a wrong answer. For a timestamp column and a timestamptz constant, timestamp_cmp_timestamptz turns each statistic, a wall time, into an instant in the session zone, and that is not monotonic across a spring-forward gap. In New York a row group holding 2024-03-10 02:00, 02:30 and 03:00 has a converted max of 07:00Z but holds 02:30, which is 07:30Z. So `ts > '2024-03-10 07:15+00'::timestamptz` skipped the group and returned no row; a heap copy returns row 1. The same held for >= 07:15Z and = 07:30Z. The group-exclusion check now refuses that pair before choosing the comparison function. A timestamp constant on a timestamp column still skips, and a timestamptz column with a timestamp constant is unchanged. docs/limitations.md and the CHANGELOG entry say so. parquet_skip_cross_type and its twin gain 7 checks over a naive timestamp file in 2 row groups, every session in New York: the three predicates against the heap copy, a same-type control that still skips 1 group, its answer against the heap, and two premises. Measured: - Red: on the rebased head without the guard, PG17, the >, >= and = arms fail by the same names in shell (13 passed + 3 failed = 16) and twin (3 failed, 13 passed), each "got none want 1". - Mutant: deleting the guard line reddens the same three names in both harnesses, which dates their ledger rows. - Green: parquet_skip_cross_type 16/16 and parquet_export_stats 165/165, shell and twin, on each of PG15 to PG19. parquet_timestamp_zone 16/16 shell and twin on PG17. - Guards 804 passed. docs_style passes. cluster_tests 3252 -> 3259 (collected 3259). Ledger 6963 + 7 = 6970 rows, 4146 -> 4150 never, 2817 -> 2820 dated; the gate passes against origin/main, ceiling 75. compare_to_bash: every bash property is covered. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +- docs/limitations.md | 5 +- src/columnar_parquet_reader.c | 10 +++ test/check_ledger.tsv | 7 ++ test/check_ledger_budget.txt | 17 +++- test/parquet_skip_cross_type.sh | 37 +++++++- test/pytest/TESTS.md | 9 +- test/pytest/expected_tests.txt | 4 +- test/pytest/test_parquet_skip_cross_type.py | 97 ++++++++++++++++++--- 9 files changed, 174 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20b087f0b..50fa74a13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -449,8 +449,10 @@ true until the next version shipped. column of a Parquet foreign table read every row group; only `150000::int8` skipped. The check now compares through the operator family's cross-type comparison, as the native columnar scan already did. Answers were always correct; - this only reads less (#1712). `test/parquet_skip_cross_type.sh` and - `test_parquet_skip_cross_type.py`. + this only reads less (#1712). A `timestamp` column compared with a `timestamptz` + constant still does not skip: across a spring-forward gap the session zone's + conversion does not keep the statistics' order. `test/parquet_skip_cross_type.sh` + and `test_parquet_skip_cross_type.py`. - **`export_parquet` no longer marks a `timestamp` column as a UTC instant.** Both `timestamp` and `timestamptz` were written as the converted type `TIMESTAMP_MICROS`, which the Parquet spec defines as adjusted to UTC. Every other reader (pyarrow, diff --git a/docs/limitations.md b/docs/limitations.md index 2b657adc0..243816e63 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -1095,7 +1095,10 @@ correct rows; these are the conditions under which it can skip at all: `bigint_col > 5` skips, though `5` is an `int4`. So do `real_col < 0.4` and `ts >= DATE '2026-01-01'` on a `timestamp` column. A comparison that casts the column instead does not skip. An example is `bigint_col < 5.0`, where `5.0` is a - `numeric`. + `numeric`. A `timestamp` column compared with a `timestamptz` constant does not + skip either. The comparison turns each wall time into an instant in the session's + time zone. Across a spring-forward gap that does not keep their order, so the + statistics would not bound the rows. - The row group's statistics must carry both a minimum and a maximum, and the interval must not be inverted. Two cases decode to an interval that the reader does not trust for skipping. The first is an unsigned Parquet column that diff --git a/src/columnar_parquet_reader.c b/src/columnar_parquet_reader.c index 6a6f1c363..a47601d97 100644 --- a/src/columnar_parquet_reader.c +++ b/src/columnar_parquet_reader.c @@ -4571,7 +4571,17 @@ pqfdw_clause_excludes_group(ImpLeaf *leaf, PqChunk *ch, int strategy, * path does (#1712). A bare integer literal is int4, so requiring the column's * own type left `WHERE id > 150000` on a bigint column skipping nothing. A * family with no such function, or no family, means no skip. + * + * Except a timestamp column against a timestamptz constant: the comparison + * converts each statistic, a wall time, to an instant in the session zone, + * which is not monotonic across a spring-forward gap. In New York a group + * holding 02:00, 02:30 and 03:00 has max 07:00Z but a row at 07:30Z, so + * `ts > '07:15+00'` would skip it (OffgridwithJD's review of #1737). Never + * skip that pair. A timestamptz column and a timestamp constant is safe: the + * statistics are instants, and only the one constant is converted. */ + if (leaf->plan.typid == TIMESTAMPOID && con->consttype == TIMESTAMPTZOID) + return false; if (con->consttype == leaf->plan.typid) vsConst = &tce->cmp_proc_finfo; else diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index b1e1daace..93e0c6266 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -5810,12 +5810,19 @@ parquet_nested_import parquet_nested_import round-trips after delete 15;16;17;18 parquet_skip_cross_type parquet_skip_cross_type a constant on the left of the operator is compared the same way 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the comparison function is called with the constant first, not the column's value parquet_skip_cross_type parquet_skip_cross_type a float4 NaN constant still finds the float8 column's NaN row 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the float NaN check reads the constant as the column's float type parquet_skip_cross_type parquet_skip_cross_type a float4 constant below every row skips all 4 row groups of the float8 column 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the column type's own comparison function is used for a constant of another type +parquet_skip_cross_type parquet_skip_cross_type a timestamptz constant after a spring-forward gap finds the timestamp column's row the heap finds (>) 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a timestamp column is compared with a timestamptz constant through the cross-type function +parquet_skip_cross_type parquet_skip_cross_type a timestamptz constant after a spring-forward gap finds the timestamp column's row the heap finds (>=) 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a timestamp column is compared with a timestamptz constant through the cross-type function +parquet_skip_cross_type parquet_skip_cross_type a timestamptz constant equal to a row's instant after a spring-forward gap finds that row, as the heap does 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a timestamp column is compared with a timestamptz constant through the cross-type function parquet_skip_cross_type parquet_skip_cross_type an int4 constant in an equality on the int8 column skips 3 of the 4 row groups 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the comparison function is called with the constant first, not the column's value parquet_skip_cross_type parquet_skip_cross_type an int4 constant on the int8 column skips 3 of the 4 row groups 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the comparison function is called with the constant first, not the column's value parquet_skip_cross_type parquet_skip_cross_type and returns the rows the float8 constant returns 15;16;17;18;19 never - +parquet_skip_cross_type parquet_skip_cross_type and returns the rows the heap returns 15;16;17;18;19 never - parquet_skip_cross_type parquet_skip_cross_type and returns the rows the int8 constant returns 15;16;17;18;19 never - +parquet_skip_cross_type parquet_skip_cross_type control: a timestamp constant on the timestamp column skips 1 of the 2 row groups 15;16;17;18;19 never - parquet_skip_cross_type parquet_skip_cross_type control: an int8 constant on the int8 column skips 3 of the 4 row groups 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: the comparison function is called with the constant first, not the column's value +parquet_skip_cross_type parquet_skip_cross_type premise: in New York the heap copy finds row 1 for ts > 07:15Z, ts >= 07:15Z and ts = 07:30Z 15;16;17;18;19 never - parquet_skip_cross_type parquet_skip_cross_type premise: the file holds 4 row groups (pyarrow's own count) 15;16;17;18;19 never - +parquet_skip_cross_type parquet_skip_cross_type premise: the gap file holds 2 row groups (pyarrow's own count) 15;16;17;18;19 never - parquet_timestamp_zone parquet_timestamp_zone a legacy TIMESTAMP_MICROS file read into timestamp is the session-zone wall time of its instant 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a column with only a legacy TIMESTAMP_* converted type reads as not UTC-adjusted parquet_timestamp_zone parquet_timestamp_zone a legacy TIMESTAMP_MICROS file read into timestamptz keeps its instant 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a column with only a legacy TIMESTAMP_* converted type reads as not UTC-adjusted parquet_timestamp_zone parquet_timestamp_zone a naive file read into timestamptz in Tokyo is its wall time there, as the cast gives 15;16;17;18;19 2026-10-22 columnar_parquet_reader.c: a naive column read into timestamptz is converted in America/New_York, not the session zone diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 51327c27c..e332815a2 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -7099,4 +7099,19 @@ suites_not_covered 27 # never 4541 + 3 = 4544 # dated 3464 + 0 = 3464 # 4544 + 3464 == 8008 -checks_never_observed_red 4544 +# +# #1712, parquet_skip_cross_type's spring-forward arms, with its twin (jdatcmd): 4544 -> 4548. +# OffgridwithJD's review measured a timestamp column against a timestamptz constant: the +# cross-type comparison converts each wall-time statistic to a session-zone instant, which +# is not monotonic across a spring-forward gap, so in New York `ts > '2024-03-10 07:15+00'` +# skipped the group holding 02:30 (07:30Z). The reader now refuses that pair. 7 checks are +# added, green on each of 15-19 in shell and twin alike. 3 are dated by one mutation, the +# guard deleted, which reddens the same three names in both twins: the >, >= and = arms +# against the heap copy. The two premises, the same-type skip control and its answer +# arrive never. suites_not_covered does not move. +# +# ledger rows 8008 + 7 = 8015 +# never 4544 + 4 = 4548 +# dated 3464 + 3 = 3467 +# 4548 + 3467 == 8015 +checks_never_observed_red 4548 diff --git a/test/parquet_skip_cross_type.sh b/test/parquet_skip_cross_type.sh index 3f61c1ada..e3e4d119a 100755 --- a/test/parquet_skip_cross_type.sh +++ b/test/parquet_skip_cross_type.sh @@ -35,10 +35,19 @@ pq.write_table(pa.table({"id": pa.array(ids, pa.int64()), # NaN sits in the second group; Parquet statistics leave it out of min and max pq.write_table(pa.table({"f": pa.array([1.0, 2.0, 3.0, float("nan")], pa.float64())}), f"{w}/xn.parquet", row_group_size=2) -print(pq.ParquetFile(f"{w}/xt.parquet").metadata.num_row_groups) +# a naive timestamp column in 2 row groups: group 0 spans New York's 2024-03-10 +# spring-forward gap (ids 0-2), group 1 is the hour before it (ids 3-5) +import datetime as dt +gap = [dt.datetime(2024, 3, 10, h, m) for h, m in ((2, 0), (2, 30), (3, 0), (1, 0), (1, 10), (1, 20))] +pq.write_table(pa.table({"id": pa.array(range(6), pa.int64()), "ts": pa.array(gap, pa.timestamp("us"))}), + f"{w}/xg.parquet", row_group_size=3) +print(pq.ParquetFile(f"{w}/xt.parquet").metadata.num_row_groups, + pq.ParquetFile(f"{w}/xg.parquet").metadata.num_row_groups) PY )" -chmod 644 "$W/xt.parquet" "$W/xn.parquet" +GGROUPS="${NGROUPS#* }" +NGROUPS="${NGROUPS%% *}" +chmod 644 "$W/xt.parquet" "$W/xn.parquet" "$W/xg.parquet" psql_run "CREATE SERVER pq FOREIGN DATA WRAPPER pgcolumnar_parquet; CREATE FOREIGN TABLE xt (id int8, f float8) SERVER pq OPTIONS (path '$W/xt.parquet'); CREATE FOREIGN TABLE xn (f float8) SERVER pq OPTIONS (path '$W/xn.parquet');" @@ -74,4 +83,28 @@ check_num "and returns the rows the float8 constant returns" \ check_num "a float4 NaN constant still finds the float8 column's NaN row" \ "$(q "SELECT count(*) FROM xn WHERE f = 'NaN'::float4;")" "1" +# A timestamp column and a timestamptz constant (OffgridwithJD's review of #1737). +# The cross-type comparison converts each statistic, a wall time, to an instant in +# the session zone, and that conversion is not monotonic across a spring-forward +# gap. In New York, group 0's max 03:00 becomes 07:00Z, yet its row 02:30 becomes +# 07:30Z. Every session below runs in New York; the heap copy is the oracle. +psql_run "CREATE FOREIGN TABLE xg (id int8, ts timestamp) SERVER pq OPTIONS (path '$W/xg.parquet'); + CREATE TABLE xg_heap AS SELECT * FROM xg;" +qny() { PGOPTIONS="-c TimeZone=America/New_York" q "$1"; } +ids() { qny "SELECT coalesce(string_agg(id::text, ',' ORDER BY id), 'none') FROM $1 WHERE $2;"; } +check_num "premise: the gap file holds 2 row groups (pyarrow's own count)" "$GGROUPS" "2" +check_text "premise: in New York the heap copy finds row 1 for ts > 07:15Z, ts >= 07:15Z and ts = 07:30Z" \ + "$(ids xg_heap "ts > '2024-03-10 07:15+00'::timestamptz") $(ids xg_heap "ts >= '2024-03-10 07:15+00'::timestamptz") $(ids xg_heap "ts = '2024-03-10 07:30+00'::timestamptz")" \ + "1 1 1" +check_text "a timestamptz constant after a spring-forward gap finds the timestamp column's row the heap finds (>)" \ + "$(ids xg "ts > '2024-03-10 07:15+00'::timestamptz")" "$(ids xg_heap "ts > '2024-03-10 07:15+00'::timestamptz")" +check_text "a timestamptz constant after a spring-forward gap finds the timestamp column's row the heap finds (>=)" \ + "$(ids xg "ts >= '2024-03-10 07:15+00'::timestamptz")" "$(ids xg_heap "ts >= '2024-03-10 07:15+00'::timestamptz")" +check_text "a timestamptz constant equal to a row's instant after a spring-forward gap finds that row, as the heap does" \ + "$(ids xg "ts = '2024-03-10 07:30+00'::timestamptz")" "$(ids xg_heap "ts = '2024-03-10 07:30+00'::timestamptz")" +check_num "control: a timestamp constant on the timestamp column skips 1 of the 2 row groups" \ + "$(PGOPTIONS="-c TimeZone=America/New_York" skipped "SELECT count(*) FROM xg WHERE ts > '2024-03-10 02:00'::timestamp")" "1" +check_text "and returns the rows the heap returns" \ + "$(ids xg "ts > '2024-03-10 02:00'::timestamp")" "$(ids xg_heap "ts > '2024-03-10 02:00'::timestamp")" + pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index d17facb4a..139e73e45 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -12876,7 +12876,7 @@ The pytest twin of `test/parquet_timestamp_zone.sh` (#1706 item 2), every check ## 344. test_parquet_skip_cross_type.py: Parquet row-group skipping uses a constant of another type in the operator family -The pytest twin of `test/parquet_skip_cross_type.sh` (#1712), every check by the shell's name. Over a file of 200,000 ascending rows in 4 row groups, each arm reads the skip count from EXPLAIN (ANALYZE): an int4 constant on the int8 column (greater-than, equality, and on the left of the operator), and a float4 constant below every row on the float8 column; the answers are compared with the same query in the column's own type, and a float4 NaN constant must still find a NaN row its group's statistics leave out. Before #1712 a constant of another type skipped nothing. The files are written by `python3` with pyarrow, so no test imports pyarrow. On a private database. +The pytest twin of `test/parquet_skip_cross_type.sh` (#1712), every check by the shell's name. Over a file of 200,000 ascending rows in 4 row groups, each arm reads the skip count from EXPLAIN (ANALYZE): an int4 constant on the int8 column (greater-than, equality, and on the left of the operator), and a float4 constant below every row on the float8 column; the answers are compared with the same query in the column's own type, and a float4 NaN constant must still find a NaN row its group's statistics leave out. A second file, a naive timestamp column whose first row group spans New York's 2024-03-10 spring-forward gap, is read in New York: a timestamptz constant must find the rows a heap copy finds, because converting wall-time statistics to instants does not keep their order across the gap, and a timestamp constant must still skip. Before #1712 a constant of another type skipped nothing. The files are written by `python3` with pyarrow, so no test imports pyarrow. On a private database. ### Every test @@ -12891,3 +12891,10 @@ The pytest twin of `test/parquet_skip_cross_type.sh` (#1712), every check by the | `test_pxt_a_float4_constant_below_every_row_skips_all_groups` | a float4 constant below every row skips all 4 row groups of the float8 column -- negative, because a float4 misread as float8 is a tiny positive number | | `test_pxt_the_float4_constant_returns_the_float8_rows` | and returns the rows the float8 constant returns | | `test_pxt_a_float4_nan_still_finds_the_nan_row` | a float4 NaN constant still finds the float8 column's NaN row | +| `test_pxt_the_gap_file_holds_two_row_groups` | premise: the gap file holds 2 row groups (pyarrow's own count) | +| `test_pxt_the_heap_finds_row_one_for_each_gap_predicate` | premise: in New York the heap copy finds row 1 for ts > 07:15Z, ts >= 07:15Z and ts = 07:30Z | +| `test_pxt_a_timestamptz_constant_after_a_gap_finds_the_heap_row_gt` | a timestamptz constant after a spring-forward gap finds the timestamp column's row the heap finds (>) | +| `test_pxt_a_timestamptz_constant_after_a_gap_finds_the_heap_row_ge` | a timestamptz constant after a spring-forward gap finds the timestamp column's row the heap finds (>=) | +| `test_pxt_a_timestamptz_equality_after_a_gap_finds_the_heap_row` | a timestamptz constant equal to a row's instant after a spring-forward gap finds that row, as the heap does | +| `test_pxt_a_timestamp_constant_skips_one_gap_group` | control: a timestamp constant on the timestamp column skips 1 of the 2 row groups | +| `test_pxt_the_timestamp_constant_returns_the_heap_rows` | and returns the rows the heap returns | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 1b54a4ae5..190fe6c22 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -1216,4 +1216,6 @@ guard_tests 839 # 9 tests, one per check, by collection over the cluster files. # cluster_tests 4258 -> 4261 for #1712's three soundness arms in test_parquet_export_stats.py # (parametrized), by collection over the cluster files. -cluster_tests 4261 +# cluster_tests 4261 -> 4268 for #1712's seven spring-forward arms in test_parquet_skip_cross_type.py, +# one per check, by collection over the cluster files. +cluster_tests 4268 diff --git a/test/pytest/test_parquet_skip_cross_type.py b/test/pytest/test_parquet_skip_cross_type.py index 015a80d31..65adf2820 100644 --- a/test/pytest/test_parquet_skip_cross_type.py +++ b/test/pytest/test_parquet_skip_cross_type.py @@ -8,7 +8,10 @@ EXPLAIN (ANALYZE): int4 constants on the int8 column (greater-than, equality, and the constant on the left), a float4 constant on the float8 column, the answers compared with the same query written in the column's own type, and a float4 NaN constant that must -still find the float8 column's NaN row. +still find the float8 column's NaN row. A second file holds a naive timestamp column +whose first row group spans New York's 2024-03-10 spring-forward gap: read in New York, +a timestamptz constant must find what a heap copy finds, and a timestamp constant must +still skip (OffgridwithJD's review of #1737). The files are written by `python3` with pyarrow, as the shell writes them, so no test imports pyarrow: each declines by the shell's skip name first, then asks for the module @@ -34,18 +37,30 @@ f"{w}/xt.parquet", row_group_size=50000) pq.write_table(pa.table({"f": pa.array([1.0, 2.0, 3.0, float("nan")], pa.float64())}), f"{w}/xn.parquet", row_group_size=2) -print(pq.ParquetFile(f"{w}/xt.parquet").metadata.num_row_groups) +import datetime as dt +gap = [dt.datetime(2024, 3, 10, h, m) for h, m in ((2, 0), (2, 30), (3, 0), (1, 0), (1, 10), (1, 20))] +pq.write_table(pa.table({"id": pa.array(range(6), pa.int64()), "ts": pa.array(gap, pa.timestamp("us"))}), + f"{w}/xg.parquet", row_group_size=3) +print(pq.ParquetFile(f"{w}/xt.parquet").metadata.num_row_groups, + pq.ParquetFile(f"{w}/xg.parquet").metadata.num_row_groups) ''' +# The gap arms run in New York, as the shell's do (OffgridwithJD's review of #1737). +ZONE = "-c TimeZone=America/New_York" +GT = "ts > '2024-03-10 07:15+00'::timestamptz" +GE = "ts >= '2024-03-10 07:15+00'::timestamptz" +EQ = "ts = '2024-03-10 07:30+00'::timestamptz" +SAME = "ts > '2024-03-10 02:00'::timestamp" + def _failed(exc): return query_error(str(exc).splitlines()[0] if str(exc) else type(exc).__name__) -def _value(dsn, sql): +def _value(dsn, sql, options=""): """-> the first column of the first row, or query_error() carrying the failure.""" try: - with psycopg.connect(dsn, autocommit=True) as conn: + with psycopg.connect(dsn, autocommit=True, options=options) as conn: row = conn.execute(sql).fetchone() except psycopg.Error as exc: return _failed(exc) @@ -62,10 +77,10 @@ def _run(dsn, sql): return None -def _skipped(dsn, query): +def _skipped(dsn, query, options=""): """-> the "Row Groups Skipped" count of EXPLAIN (ANALYZE), or query_error().""" try: - with psycopg.connect(dsn, autocommit=True) as conn: + with psycopg.connect(dsn, autocommit=True, options=options) as conn: plan = "\n".join(r[0] for r in conn.execute( f"EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) {query}").fetchall()) except psycopg.Error as exc: @@ -83,17 +98,23 @@ def pxt(pgc_cluster, server_dir): dsn = pgc_cluster.dsn(dbname=DB) w = server_dir wrote = subprocess.run([sys.executable, "-c", WRITER, str(w)], capture_output=True, text=True) - for f in ("xt", "xn"): + for f in ("xt", "xn", "xg"): if (w / f"{f}.parquet").exists(): (w / f"{f}.parquet").chmod(0o644) - r = {"groups": int(wrote.stdout.strip()) if wrote.returncode == 0 and wrote.stdout.strip().isdigit() - else query_error(f"the writer failed: {wrote.stderr.strip().splitlines()[-1:]}")} + counts = wrote.stdout.split() + if wrote.returncode == 0 and len(counts) == 2 and all(c.isdigit() for c in counts): + r = {"groups": int(counts[0]), "gap groups": int(counts[1])} + else: + failed = f"the writer failed: {wrote.stderr.strip().splitlines()[-1:]}" + r = {"groups": query_error(failed), "gap groups": query_error(failed)} # A failed statement here makes every later reading a query_error, which # reddens its check by name rather than erroring the fixture. for sql in ("CREATE EXTENSION IF NOT EXISTS pgcolumnar", "CREATE SERVER pq FOREIGN DATA WRAPPER pgcolumnar_parquet", f"CREATE FOREIGN TABLE xt (id int8, f float8) SERVER pq OPTIONS (path '{w}/xt.parquet')", - f"CREATE FOREIGN TABLE xn (f float8) SERVER pq OPTIONS (path '{w}/xn.parquet')"): + f"CREATE FOREIGN TABLE xn (f float8) SERVER pq OPTIONS (path '{w}/xn.parquet')", + f"CREATE FOREIGN TABLE xg (id int8, ts timestamp) SERVER pq OPTIONS (path '{w}/xg.parquet')", + "CREATE TABLE xg_heap AS SELECT * FROM xg"): _run(dsn, sql) r["int8 skip"] = _skipped(dsn, "SELECT count(*) FROM xt WHERE id > 150000::int8") r["int4 skip"] = _skipped(dsn, "SELECT count(*) FROM xt WHERE id > 150000") @@ -105,6 +126,12 @@ def pxt(pgc_cluster, server_dir): r["float4 rows"] = _value(dsn, "SELECT count(*) FROM xt WHERE f < 50000::float4") r["float8 rows"] = _value(dsn, "SELECT count(*) FROM xt WHERE f < 50000::float8") r["nan rows"] = _value(dsn, "SELECT count(*) FROM xn WHERE f = 'NaN'::float4") + for table in ("xg", "xg_heap"): + for name, pred in (("gt", GT), ("ge", GE), ("eq", EQ), ("same", SAME)): + r[f"{table} {name}"] = _value( + dsn, f"SELECT coalesce(string_agg(id::text, ',' ORDER BY id), 'none') FROM {table} WHERE {pred}", + ZONE) + r["same skip"] = _skipped(dsn, f"SELECT count(*) FROM xg WHERE {SAME}", ZONE) yield r finally: with psycopg.connect(pgc_cluster.dsn(), autocommit=True) as a: @@ -173,3 +200,53 @@ def test_pxt_a_float4_nan_still_finds_the_nan_row(request, expect): r = _readings(request, expect) if r is not None: expect.num(r["nan rows"], 1, "a float4 NaN constant still finds the float8 column's NaN row") + + +def test_pxt_the_gap_file_holds_two_row_groups(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["gap groups"], 2, "premise: the gap file holds 2 row groups (pyarrow's own count)") + + +def test_pxt_the_heap_finds_row_one_for_each_gap_predicate(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text((r["xg_heap gt"], r["xg_heap ge"], r["xg_heap eq"]), ("1", "1", "1"), + "premise: in New York the heap copy finds row 1 for ts > 07:15Z, ts >= 07:15Z and ts = 07:30Z") + + +def test_pxt_a_timestamptz_constant_after_a_gap_finds_the_heap_row_gt(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["xg gt"], r["xg_heap gt"], + "a timestamptz constant after a spring-forward gap finds the timestamp column's row " + "the heap finds (>)") + + +def test_pxt_a_timestamptz_constant_after_a_gap_finds_the_heap_row_ge(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["xg ge"], r["xg_heap ge"], + "a timestamptz constant after a spring-forward gap finds the timestamp column's row " + "the heap finds (>=)") + + +def test_pxt_a_timestamptz_equality_after_a_gap_finds_the_heap_row(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["xg eq"], r["xg_heap eq"], + "a timestamptz constant equal to a row's instant after a spring-forward gap finds that row, " + "as the heap does") + + +def test_pxt_a_timestamp_constant_skips_one_gap_group(request, expect): + r = _readings(request, expect) + if r is not None: + expect.num(r["same skip"], 1, + "control: a timestamp constant on the timestamp column skips 1 of the 2 row groups") + + +def test_pxt_the_timestamp_constant_returns_the_heap_rows(request, expect): + r = _readings(request, expect) + if r is not None: + expect.text(r["xg same"], r["xg_heap same"], "and returns the rows the heap returns") From 0aeb420f048e08d9d261c4ba6c96857439a6dd32 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 22 Oct 2026 17:26:13 +0000 Subject: [PATCH 4/4] fix: a fixture port depends on the xdist worker, so -n 2 runs no longer collide (#1742) A twin that starts its own S3 or HTTP fixture server takes the port from pick_port(slot) in a module fixture. Under -n, a module's tests are split across workers, so every worker runs that fixture. The port depended on the slot only, so two workers probed the same free port at the same moment and the second bind failed with "Address already in use" (#1635, #1714, #1721). CI runs the cluster files serially, so CI stayed green; the guard job and developers run -n. pick_port now starts each worker in its own block of 64 ports (WORKER_STRIDE), with the slot as an offset inside it. The worker comes from PYTEST_XDIST_WORKER, so no fixture changes. Slot 0 is the worker's own cluster, which make_cluster now asks for through the same function. A slot that does not fit in a block is refused. The walk on a busy port, the AUX band and the ephemeral-floor contract are unchanged. test_runshape.py gains test_two_workers_taking_a_fixture_port_do_not_collide: an inner -n 2 run where each worker takes pick_port(27), waits until the other has a port, then binds it. On main it is red: "the two workers were given two different ports: got 1 want 2", and the inner bind fails with [Errno 98] Address already in use. With the fix it is green. Measured on PG17 (a copy of the prefix): - test_iceberg_objstore.py under -n 2: main 3 of 3 red ("premise: sigv4 fixture server is up: got '0' want '1'"; "Connection refused"); fixed 16 passed 3 of 3, and 16 passed serially. - test_objstore_listing.py from #1714 under -n 2: as is, 13 failed ("the private cluster did not start"); with this pgc_cluster.py, 15 passed 3 of 3 and serially. - test_connection.py and test_hardening.py under -n 2: 20 passed. guard_tests 476 -> 477, by collection over NO_CLUSTER; TESTS.md section 10 gains a row. The guards (477), test_harness_deps.py and test_capability_decline.py with --pg-config pass over 21182d05. Review (OffgridwithJD): a 64-port block holds slots 0 to 31, and main's twins already ask for 32 to 35 (the stack for up to 41), so slot 32 raised at fixture time: test_iceberg_rest.py "14 errors", ValueError "slot 32 does not fit in a worker's block of 64 ports". WORKER_STRIDE is now 96 (slots 0 to 47; the AUX band's 2000 ports still give 20 workers). Two guards keep the stride and the corpus from drifting apart, red first on the 64 stride: every literal pick_port slot fits a worker's block (it named the 32-35 files), and the corpus's highest slot (35) gets a port in the band. Green: test_runshape.py 18 passed; the five twins that errored pass serially (14, 11, 29, 10, 8); iceberg_rest + iceberg_objstore under -n 2, 30 passed; guards 807 passed on main 9ab6a4cf (guard_tests 804 -> 807). Co-Authored-By: Claude Opus 5.5 --- test/pytest/TESTS.md | 14 ++++ test/pytest/expected_tests.txt | 6 +- test/pytest/pgc_cluster.py | 46 +++++++++++-- test/pytest/test_runshape.py | 114 +++++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 7 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 139e73e45..8a9b6c02d 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1694,6 +1694,20 @@ This closes the part of `guard-as-teardown-fixture-still-reports-passed` close the family: a guard anyone adds later in a teardown is still a guard that cannot fail its test, and nothing refuses that shape in general. +### Two workers must not get one fixture port + +A twin that starts its own S3 or HTTP fixture server takes the port from +`pick_port(slot)` in a module fixture. Under `-n`, a module's tests are split across +workers, so every worker runs that fixture. When the port depended on the slot only, +two workers probed the same free port and the second bind failed with `Address +already in use` (#1742). CI runs the cluster files serially, so only `-n` runs met it. + +| test | what it asserts | how it fails | +| --- | --- | --- | +| `test_two_workers_taking_a_fixture_port_do_not_collide` | an inner `-n 2` run where each worker takes `pick_port(27)`, waits for the other, then binds it: two workers, two ports, inner run green | with the port independent of the worker (main before #1742), both workers get one port and the second bind fails | +| `test_every_literal_fixture_slot_fits_a_workers_block` | every literal `pick_port()` in the corpus fits a worker's block of WORKER_STRIDE ports, naming any file that asks for more | +| `test_the_highest_slot_in_the_corpus_gets_a_port` | the corpus's highest slot gets a port inside the AUX band, so the stride and the corpus cannot drift apart | + ## 11. test_zonemap_boundaries.py: exact boundaries ### `test_exact_zonemap_boundaries` diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 190fe6c22..9f27ceaf5 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -326,7 +326,11 @@ # which needs no cluster: 3 tests, by collection over NO_CLUSTER. # guard_tests 804 -> 806 for #1613's test_a_refused_comparison_is_counted_and_named in # test_check_records.py, serial and xdist, by collection over NO_CLUSTER. -guard_tests 839 +# guard_tests 839 -> 840 for #1742's test_two_workers_taking_a_fixture_port_do_not_collide in +# test_runshape.py, by collection over NO_CLUSTER. +# guard_tests 840 -> 842 for #1749's review: every literal fixture slot fits a worker's block, +# and the corpus's highest slot gets a port, in test_runshape.py, by collection over NO_CLUSTER. +guard_tests 842 # The complement: tests that need the driver and a throwaway cluster. Until #1016 these ran # in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index 8eee9c9b1..4c41e6214 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -92,22 +92,56 @@ def port_is_free(port, host="127.0.0.1"): return True -def pick_port(slot, floor=None, is_free=port_is_free): - """A free port in the AUX band for this worker slot. +# THE PORT DEPENDS ON THE WORKER AS WELL AS THE SLOT (#1742). A module fixture that +# starts its own server (an S3 fixture, a private cluster) asks for a fixed slot, +# and under `-n` a module's tests are split across workers, so every worker runs +# that fixture. When the start depended on the slot only, two workers probed the +# same free port at the same moment, and the second bind failed with "Address +# already in use" (#1635, #1714, #1721). Each worker now starts in its own block of +# WORKER_STRIDE ports, and a slot is an offset inside that block. Slot 0 is the +# worker's own cluster (make_cluster); fixtures use 1 and up. +# +# 96, not 64 (#1749 review): main's twins already ask for slots 32 to 35 and the +# stack up to 41, and a block of 64 ports holds slots 0 to 31. 96 holds 0 to 47, and +# the AUX band's 2000 ports still give 20 workers, more than any -n here. The guard +# test_every_literal_fixture_slot_fits_a_workers_block keeps the two from drifting. +WORKER_STRIDE = 96 + + +def worker_slot(worker_id=None): + """The xdist worker's number: 3 for `gw3`, and 0 outside xdist. + + Read from PYTEST_XDIST_WORKER when no id is given, which xdist sets in every + worker, so a fixture calling pick_port needs no argument to be worker-aware. + """ + if worker_id is None: + worker_id = os.environ.get("PYTEST_XDIST_WORKER") + if worker_id in (None, "", "master"): + return 0 + digits = str(worker_id).lstrip("gw") + return int(digits) if digits.isdigit() else 0 + + +def pick_port(slot, floor=None, is_free=port_is_free, worker_id=None): + """A free port in the AUX band for this slot of this worker. Walks on collision instead of trusting the band, because the band only makes a collision unlikely and this harness has already been broken once by a port that something else was holding. """ + if not 0 <= slot * 2 < WORKER_STRIDE: + raise ValueError(f"slot {slot} does not fit in a worker's block of " + f"{WORKER_STRIDE} ports") lo, hi = aux_band(floor if floor is not None else read_ephemeral_floor()) span = hi - lo - start = slot * 2 + start = worker_slot(worker_id) * WORKER_STRIDE + slot * 2 for step in range(span): port = lo + ((start + step) % span) if is_free(port): return port raise RuntimeError( - f"no free port in the AUX band [{lo}, {hi}) for worker slot {slot}") + f"no free port in the AUX band [{lo}, {hi}) for slot {slot} of worker " + f"{worker_slot(worker_id)}") class Cluster: @@ -781,8 +815,8 @@ def make_cluster(pg_config, worker_id): inside `stop()`; failing to stop it must not mask the original error, so the cleanup is itself guarded. """ - slot = 0 if worker_id in (None, "master") else int(str(worker_id).lstrip("gw") or 0) - port = pick_port(slot) + slot = worker_slot(worker_id) + port = pick_port(0, worker_id=worker_id) root = pathlib.Path(tempfile.mkdtemp(prefix=f"pgc-pytest-{slot}-")) cluster = None try: diff --git a/test/pytest/test_runshape.py b/test/pytest/test_runshape.py index 9875b4cd1..0acf0afd2 100644 --- a/test/pytest/test_runshape.py +++ b/test/pytest/test_runshape.py @@ -3,6 +3,9 @@ Each is measured in VACUITY_MODES.md and each is worth more than the per-assertion guards, because a single occurrence silently removes many tests at once. """ +import pathlib +import re + def test_layer_fails_when_a_collected_test_never_reports(pytester, expect): @@ -338,3 +341,114 @@ def test_body(guard_in_teardown, expect): result = pytester.runpytest("-p", "pgc_vacuity") expect.outcomes(result, "a teardown refusal leaves the test reported as passed", passed=1, failed=0, errors=1) + + +def test_two_workers_taking_a_fixture_port_do_not_collide(pytester, monkeypatch, expect): + """Two xdist workers asking `pick_port` for the same slot get different ports (#1742). + + A twin that starts its own S3 or HTTP fixture server takes its port from + `pick_port(slot)` in a module fixture. Under `-n 2` a module's tests are split + across workers, so each worker runs that fixture. On main the port depended on + the slot only: both workers probed the same free port and the second bind failed + with `Address already in use` (#1635, #1714, #1721). CI ran the cluster files + serially, so only the guard job and developers met it. + + The inner tests take their port, wait until both workers have one, and only then + bind it, so the probe-then-bind race happens every run rather than by chance. A + subprocess, because xdist's workers are. + """ + import pathlib + here = pathlib.Path(__file__).resolve().parent + meet = pytester.path / "meet" + meet.mkdir() + monkeypatch.setenv("PYTHONPATH", f"{pytester.path}:{here}") + monkeypatch.setenv("PGC_PORT_MEET", str(meet)) + pytester.makepyfile(test_ports=""" + import os, pathlib, socket, time + from pgc_cluster import pick_port + + MEET = pathlib.Path(os.environ["PGC_PORT_MEET"]) + + def _wait_for(suffix, n): + for _ in range(600): + if len(list(MEET.glob("*" + suffix))) >= n: + return + time.sleep(0.05) + + def _take(expect, me): + port = pick_port(27) + (MEET / (me + ".pick")).write_text( + os.environ.get("PYTEST_XDIST_WORKER", "none") + " " + str(port)) + _wait_for(".pick", 2) + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + try: + s.bind(("127.0.0.1", port)) + s.listen() + got = "bound" + except OSError as e: + got = "bind failed: " + str(e) + (MEET / (me + ".tried")).write_text(got) + _wait_for(".tried", 2) + expect.text(got, "bound", "this worker alone holds port " + str(port)) + + def test_first(expect): + _take(expect, "first") + + def test_second(expect): + _take(expect, "second") + """) + result = pytester.runpytest_subprocess("-p", "pgc_vacuity", "-p", "no:cacheprovider", + "-n", "2") + picks = sorted(p.read_text().split() for p in meet.glob("*.pick")) + workers = sorted({w for w, _ in picks}) + ports = sorted({port for _, port in picks}) + # The premise: the race is only measured if the two picks ran on two workers. + expect.text(" ".join(workers), "gw0 gw1", + "premise: the two inner tests ran on two different workers") + expect.num(len(ports), 2, "the two workers were given two different ports") + out = result.stdout.str() + expect.text("green" if result.ret == 0 else f"exit {result.ret}: " + f"{[l for l in out.splitlines() if 'bind failed' in l or 'FAILED' in l][:4]!r}", + "green", "and each worker bound its own port, so the inner run is green") + + +# ---- every slot the corpus asks for fits a worker's block (#1742 review) ---------- +# +# A slot that does not fit raises at fixture time, on the cluster job, as every test +# of that file erroring. These two read the corpus instead, so the next twin that asks +# for a slot past the block fails here, in the guard job, by name. Measured before +# the stride was raised: main's twins ask for slots 32 to 35, and a 64-port block +# holds 0 to 31. +_SLOT_CALL = re.compile(r"\bpick_port\(\s*(\d+)\s*[,)]") + + +def _corpus_slots(): + here = pathlib.Path(__file__).resolve().parent + found = {} + for f in sorted(here.glob("*.py")): + for m in _SLOT_CALL.finditer(f.read_text()): + found.setdefault(int(m.group(1)), []).append(f.name) + return found + + +def test_every_literal_fixture_slot_fits_a_workers_block(expect): + from pgc_cluster import WORKER_STRIDE + slots = _corpus_slots() + expect.at_least(len(slots), 5, "premise: the corpus scan found the fixture slots in use") + too_big = sorted(f"{s} ({', '.join(sorted(set(files)))})" for s, files in slots.items() + if not s * 2 < WORKER_STRIDE) + expect.text(", ".join(too_big) or "none", "none", + "every literal pick_port slot fits a worker's block of ports") + + +def test_the_highest_slot_in_the_corpus_gets_a_port(expect): + from pgc_cluster import aux_band, pick_port + top = max(_corpus_slots()) + lo, hi = aux_band(32768) + try: + port = pick_port(top, floor=32768, is_free=lambda p: True, worker_id="gw3") + reading = "in the band" if lo <= port < hi else f"outside the band: {port}" + except ValueError as exc: + reading = f"refused: {exc}" + expect.text(reading, "in the band", + f"the highest slot the corpus uses ({top}) gets a port in the AUX band")