Skip to content

fix: export_parquet writes row-group statistics, so our own files skip (#850) - #851

Merged
jdatcmd merged 6 commits into
mainfrom
fix/850-export-parquet-statistics
Aug 31, 2026
Merged

fix: export_parquet writes row-group statistics, so our own files skip (#850)#851
jdatcmd merged 6 commits into
mainfrom
fix/850-export-parquet-statistics

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Closes #850.

What was wrong

pgcolumnar.export_parquet wrote no per-row-group statistics. write_column_chunk() emitted ColumnMetaData fields 1 through 7 and 9, and never field 12. So a file we wrote carried no minimum or maximum for the reader to test a predicate against, and every documented condition for row-group skipping could hold while Row Groups Skipped stayed 0.

The reporter met every condition in docs/limitations.md and was right to file it. The read side was never at fault, and the docs never told them: the only statements that our exporter wrote no statistics lived in a test comment and a design note.

The separator

On the reporter's own repro, PostgreSQL 17.10, 1,000,000 rows:

probe before after
column chunks carrying statistics, our exported file 0 of 32 32 of 32
Row Groups Skipped, FDW over our file, id < 5000 0 of 16 15 of 16, 1 decoded
pyarrow is_stats_set on our file False True
native chunk-group skip, same data (control) 6 of 7 removed 6 of 7 removed
FDW over a pyarrow file, same data (control) 15 of 16 15 of 16

The two controls are what place the fault on the write side: the reader skipped a third-party file correctly the whole time.

What the exporter writes now

Per column chunk: null_count always, including where it is zero, because parquet.thrift says a reader must not read an absent count as zero; min_value and max_value for the columns stored as INT32, INT64, FLOAT or DOUBLE, which is exactly the set the reader can skip on; and nan_count for the float columns. A text, bytea, uuid, boolean or byte-array numeric column gets the null count and no bounds: UTF8 sorts by unsigned bytes, which is not any PostgreSQL collation, and a bound in the wrong order is worse than no bound.

The footer also gains column_orders, one TYPE_ORDER per leaf column. That is not decoration. parquet.thrift states that without it the meaning of min_value and max_value is undefined, and Arrow acts on that by discarding both. Statistics without column_orders would have left the file exactly as unskippable under pyarrow as before.

Three float rules from the specification, each pinned by a test: a NaN never becomes a bound, an all-NaN column gets no bounds at all, and a computed zero is written -0.0 as a minimum and +0.0 as a maximum. Values with no Parquet representation (infinite date or timestamp) are folded to null before the accumulator sees them, so they count as nulls and cannot reach a bound.

Bounds accumulate in physical space inside write_leaf_value, from the same expression that writes the data page. Deriving them a second time is how a bound comes to disagree with its column by an epoch. They are snapshotted into the row group at flush, before pqleaf_reset clears them, because the footer is written after every group has been flushed.

Tests

test/parquet_export_stats.sh, 162 checks, and test/parquet_stats.py, a footer reader that imports no Parquet library. That instrument choice is deliberate: a suite whose instrument is an optional import reports SKIP when the import is missing, and a skip is not coverage. column_orders is also invisible through pyarrow's statistics API, so a third-party reader cannot tell us whether we wrote it.

Twelve premises pass on the unfixed tree, so none of the reds were ambiguous: the parser reaches ColumnMetaData (num_values sums to the table), the file under test is one we wrote, the oracle holds every exported row, and the oracle's buckets are the file's row groups. The instrument's ability to report a bound that IS present, and to report a deprecated field that IS present, are both controlled against a pyarrow file.

Measured on 0e4884c1 with the two test files copied in byte-identical — sha256 printed for both against this head — after asserting the tree carries no occurrence of write_statistics, column_orders or PqStats:

total checks: 162
RED before the fix: 104      green on the unfixed tree: 58      104 + 58 = 162
green after: 162 of 162

Removal proof

Eight mutations of the writer, each turning a named check red, each a distinct .so fingerprint. Every count below was re-measured at this head, 50f5d7c, against the suite as it now stands at 162 checks. Baseline and restore are the same binary, 5949989c4981, at 162 of 162 both times, and the eight mutants carry eight further distinct fingerprints, so no verdict is a stale-binary reading. The "loses rows?" column is not a judgement: it is whether any the FDW result equals the oracle check went red on that arm.

mutation reds loses rows?
delete the Statistics emission 102 no
write min_value but not max_value 47 no
let a null into the accumulator as zero 17 no
let NaN reach the bounds 3 no
delete column_orders 2 no
swap min_value and max_value 60 no, see below
stop resetting bounds per row group 30 no
take the date bound from the PostgreSQL epoch 12 yes

A count in this table is only ever true of one suite revision, so the whole table is re-run whenever the suite grows. It has grown three times, and every row above was re-run at 50f5d7c rather than carried:

mutation 132 137 151 162
delete the Statistics emission 93 95 99 102
write min_value but not max_value 38 40 44 47
let a null into the accumulator as zero 15 15 15 17
let NaN reach the bounds 3 3 3 3
delete column_orders 2 2 2 2
swap min_value and max_value 52 54 57 60
stop resetting bounds per row group 26 26 28 30
take the date bound from the PostgreSQL epoch 10 10 10 12

The columns are suite sizes, not heads, because that is what a count belongs to. Each was run rather than derived, one clean tree per revision:

bd7bf8c (c272629^)  132 checks, 132 PASS
c272629             137
a3ca5793            137
71f8243             151
50f5d7c             162

Every move in the table is the number of new checks that mutation reaches, and the two rows that never move are the two whose blast radius is a single named property rather than the bounds themselves.

This table took three corrections, and the last two are the same mistake. First the 93 / 38 / 52 column was published under a heading of 137, where 95 / 40 / 54 belong: the instance was fixed, then the class was fixed by re-running everything, and then the history column was rebuilt from the pre-fix text rather than from the corrected values. 95 and 54 were re-measured independently by the reviewer at a3ca5793 — 42 PASS / 95 FAIL and 83 PASS / 54 FAIL, both summing to 137, with 0 oracle arms red under the swap.

Then the corrected column was labelled 135, which is a number that appears nowhere in this branch's history. It was derived — 137 minus the two control arms the review names — rather than measured, in the same edit that argued a count must name the revision it counted. Measured, c272629 moved the suite 132 → 137, which is five checks: it adds six check call sites and removes two, and the arms the review names are two of them. Counting arms is not counting checks, which is the whole reason the columns are labelled by a number that came out of a run.

Only the epoch mutation loses rows, measured on all eight rather than inferred from two: the epoch arm reds exactly one FDW-result check, d > DATE '2060-01-01', and the other seven red none. The swap does not, and that is worth stating rather than glossing: it inverts the interval, and the reader already refuses to skip an inverted one, which docs/limitations.md documents. All twenty FDW-result-equals-oracle checks pass under the swap. The epoch shift instead leaves a well-ordered interval that is simply wrong, so no guard can see it, and the single check standing between it and silent row loss is a predicate placed past the end of the data, d > DATE '2060-01-01'.

Four further mutations show the refusal checks and the plan-shape checks can fail, all re-measured at 50f5d7c. In the reader: removing the exact-constant-type test reds 5 arms — the cross-type arm, the three that require an unquoted literal to skip nothing, and digits too wide for an integer are a bigint, so an int4 column refuses — each reading a positive skip count where 0 is required; removing the float > / >= refusal reds exactly 1, a float >= predicate above every bound still skips nothing, at got [4] want [0]. In the fixture: swapping the numeric/bigint plan-shape patterns reds those 2 and nothing else, and swapping the date/timestamp pair reds those 2 and nothing else, so no grep is satisfied by whatever plan comes along.

One check is not falsified by any of them, and it says so in its own comment. the same date adorned as a timestamp does not skip reads 0 under both the reader and writer mutations: with the exact-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 reports 0 for a second reason. It is kept as a documented-behaviour guard rather than dropped, and the evidence for the sentence it guards is carried beside it by the two plan-shape checks, which the planner answers and a pattern swap reddens.

Regression

CI 12 of 12 at 50f5d7c (headSha checked rather than assumed): nine build legs including PG 19 beta from source, shellcheck, and the suites matrix on PG 17 and PG 18, where parquet_export_stats is confirmed to run rather than merely be registered — 228 ran, 9 skipped on PG 17 and 231 ran, 6 skipped on PG 18, with parquet_export_stats=PASS in both listings. That matrix covers every Parquet read and write path at this head, including parquet_export, which reads our files back with both pyarrow and DuckDB. src/ last changed at bd7bf8ce; every head after it changes only documentation and tests. docs_style is part of that matrix, and it caught the first draft of the rewritten paragraph: three sentences over the 25-word plain-language limit, which is why the rule is now a list.

Docs

docs/limitations.md gains a ### Row-group skipping heading, which four documents already cited as though it existed, and now tells a reader what an exported file carries and that a file written by 1.0-alpha2 or earlier carries nothing until it is exported again. It also states the constant-typing rule, in three labelled cases rather than one block of prose, because how the literal is written decides its type, and therefore whether the exact-type condition is met at all:

  • quoted, with no type named — the literal is unknown and takes the column's type, so all nine skippable types skip, and a temporal literal is usually written this way, which is why it skips as written;
  • a named type, quoted or cast — the literal is that type, not unknown, so the exact-type condition applies to it: date_col < DATE '2026-01-01' skips and timestamp_col < DATE '2026-01-01' does not;
  • unquoted — a digit string is an integer while the value fits in one and a bigint when it does not, matching whichever width the column is and never the other, while a decimal-point literal is a numeric that makes PostgreSQL cast the column instead.

Every cell was measured, with the planner's own constant printed and every row count compared with a heap oracle, and sixteen checks pin it. Two revisions of this branch got the rule wrong before it was right, both caught in review, and both are worth naming because the shape of the paragraph is what allowed them. The first said smallint and real "always need a cast" — false, smallint_col < '500' skips. The second said "any quoted temporal literal skips as written", which the same page contradicted twenty lines above: DATE '2026-01-01' is a quoted temporal literal and against a timestamp column it does not skip. A general rule and its two exceptions were sharing a sentence; they are now three cases, and each is pinned. The stale claims in test/native_parquet_pushdown.sh and design/PHASE_G_PUSHDOWN_HANDOFF.md are corrected.

Release

Targeted at 1.0-alpha3 on the owner's explicit decision. This changes what the writer emits, which the release plan classes as a feature, and an alpha may add features.

🤖 Generated with Claude Code

https://claude.ai/code/session_013vWhU5eNt6dGDU8iZPrrWP

jdatcmd and others added 2 commits August 30, 2026 22:42
#850)

A file written by pgcolumnar.export_parquet carried no per-row-group
statistics, so the Parquet FDW could never skip a group in it. Every
condition docs/limitations.md documents for row-group skipping could
hold and "Row Groups Skipped" stayed 0. The read side was never at
fault: write_column_chunk() emitted ColumnMetaData fields 1 through 7
and 9, and never field 12.

On the reporter's repro, 1,000,000 rows in 16 row groups: 0 of 32
column chunks carried statistics and the scan decoded all 16 groups
for id < 5000. It now carries 32 of 32 and skips 15 of 16.

The exporter now writes, per column chunk: null_count always, because
a reader may not read an absent count as zero; min_value and max_value
for the columns stored as INT32, INT64, FLOAT or DOUBLE, which is the
set the reader can skip on; and nan_count for the float columns. Text,
bytea, uuid, boolean and byte-array numeric get the null count and no
bounds, because their defined sort order is not the one we compare in.

The footer also gains column_orders, one TYPE_ORDER per leaf column.
parquet.thrift makes it mandatory once min_value and max_value are
written, and Arrow discards both without it, so the file would have
stayed unskippable under pyarrow. It now reports is_stats_set=True.

Bounds are accumulated in physical space inside write_leaf_value, from
the same expression that writes the data page, and snapshotted into the
row group at flush. A value with no Parquet representation is folded to
null first, so it counts as a null and cannot reach a bound.

test/parquet_export_stats.sh covers this in 132 checks and reads the
footer bytes with test/parquet_stats.py, which imports no Parquet
library: an instrument that is an optional import reports SKIP, and a
skip is not coverage. Eight mutations of the fix were each run against
the suite and each turned a named check red, including the two that
lose rows rather than the skip: swapping the bounds, and taking a date
bound from the PostgreSQL epoch instead of the Unix one.

Verified on PostgreSQL 17.10: the suite 132 of 132, and 25 further
suites covering every Parquet read and write path green, including
parquet_export, which reads our files back with both pyarrow and
DuckDB.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013vWhU5eNt6dGDU8iZPrrWP
… for us

The PG 19 beta leg failed to build: `implicit declaration of function
'isnan'`. PostgreSQL's own headers happen to pull math.h in on 15
through 18, so the local build and every other leg were silent about
it. Verified by building against 19beta2, where the suite is 132 of
132 after the include and the compile fails without it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013vWhU5eNt6dGDU8iZPrrWP

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at bd7bf8ce against base 0e4884c1 (the PR is up to date with main). Everything below is something I ran; where I could not reproduce a claim I say so and give the numbers.

The code is right. I went looking for a defect in it and did not find one. Five findings, all in the claims around the code — two in files that ship — and each has a fix of one sentence or one word.

What I reproduced of yours

your claim mine
95 of 132 red on the unfixed tree 95, on 0e4884c1 with your two test files copied in byte-identical (md5 matched), after asserting the tree carries 0 occurrences of write_statistics, column_orders and PqStats
nine premises pass unfixed twelve pass. You understated it
swap min_value/max_value → 52 reds 52
date bound from the PostgreSQL epoch → 10 reds 10
the reporter's repro: 15 of 16 skipped 15 skipped, 1 decoded, native control unchanged at 6 of 7 removed, count(id<5000) = 4999 both sides, full-set hash equal
32 of 32 chunks carry statistics 32 of 32, pyarrow is_stats_set=True, min=1 max=65536 on group 0

Gate on my box: preflight 5 of 5 majors, 0 warnings; PG18 235 PASS / 2 SKIP, PG19 237 PASS / 0 SKIP, 0 failures, matrix exit 0. CI 12/12.

I also checked every thrift field id against parquet.thrift on apache/parquet-format master rather than against your reader: Statistics 3/5/6, ColumnMetaData 12, FileMetaData 7, ColumnOrder union field 1. nan_count really is Statistics field 9 — I expected that one to be wrong, because your writer and your parser both assume it and would agree with each other either way. It is right.

And three things nothing in the PR covers, which I checked because a wrong answer would have been serious:

  • Nested export. The code says Arrow raises on a column_orders length mismatch, so a wrong count makes the file unreadable rather than merely unskippable, and no suite exports a nested table. Exported int4[] + composite + text[]: column_orders=6, leaves=6, pyarrow's own num_columns=6, pyarrow reads it and reports correct bounds on arr.list.element. Fine.
  • parallel_export_parquet. It calls PgColumnarWriteParquetFile (twice, columnar_parallel_export.c:520 and :538) and there is only one footer writer in the tree, so it inherits the fix.
  • Session timezone. A timestamptz bound is stored as UTC micros; if the reader converted the constant differently the skip would drop rows. Same predicate under UTC, America/New_York, Asia/Kolkata, Pacific/Auckland: FDW and native agree at all four, 2 of 3 groups skipped every time. date likewise.

DuckDB. parquet_export.sh gates its DuckDB arm on command -v duckdb and prints nothing when it is missing, so on a box without it the suite reports PASSED having asked DuckDB nothing. DuckDB is not installed here, so I fetched the CLI (v1.5.5) and asked it myself. My first pass read stats_min/stats_max and got NULL — those are the deprecated fields 1/2 you deliberately do not write, so that was me measuring the wrong surface. The right columns: stats_min_value=1 stats_max_value=65536 on group 0, 65537/131072 on group 1, per group, correct. Your DuckDB claim holds.


1. The CHANGELOG says the bound swap loses rows. It does not.

Both the PR body and CHANGELOG.md say the two named mutations "lose rows rather than merely lose the skip: swapping the bounds, and taking a date bound from the PostgreSQL epoch." The CHANGELOG ships.

I ran both, each asserting the mutation applied (exact lines shown, git diff --numstat, source sha1 restored after, 132/132 on the baseline before and after).

Swapping the bounds — 52 reds, your number, and all eleven FDW-result checks PASS:

PASS  the FDW result equals the oracle for [id BETWEEN 70000 AND 71000]
PASS  the FDW result equals the oracle for [id < 5000]
...  (11 of 11)
FAIL  a predicate confined to one group skips the other three: got [1] want [3]
FAIL  a bigint predicate skips: got [1] want [3]

It loses the skip, not rows — because the swap inverts the interval, and the reader has a guard for exactly that. docs/limitations.md already documents it: "the interval must not be inverted... the reader does not trust it for skipping." Your own defence caught your own mutation.

The date epoch — 10 reds, your number, and exactly one FDW-result check red:

FAIL  the FDW result equals the oracle for [d > DATE '2060-01-01']: got [EMPTY] want [d0e7f5b573c363b9ceaee93a8120bbba]
FAIL  a date predicate skips: got [2] want [3]

That one genuinely drops rows, because the shift leaves a well-ordered interval that is simply wrong, so the inverted-interval guard cannot see it.

So it is one mutation that loses rows, not two. I would rather this were right in the CHANGELOG than tidy, because the reason the swap is survivable is a real property of the reader that a reader of the changelog would otherwise not learn. Worth adding: d > DATE '2060-01-01' — a predicate deliberately placed past the end of the data — is the single check standing between an epoch-shift bug and silent row loss. It earns its place.

2. Two of the "documented refusal" checks cannot fail

Both assert 0 and would assert 0 against a perfectly working implementation, so neither is evidence for the refusal it names.

check_num "a cross-type constant still skips nothing" "$(skipped_for "WHERE i8 > 5::int")" "0" — measured per-group bounds of i8 in that fixture:

grp  min(i8)        max(i8)
0    1000000        65536000000
1    65537000000    131072000000
2    131073000000   196608000000
3    196609000000   196609000000

Every group's maximum exceeds 5, so nothing can be excluded whether or not the cross-type constant pushes down. No int4 constant can prune this column at all — group 0's maximum is 6.55e10, past int4.

The separator exists in your own fixture and is one word. Measured:

WHERE i8 < 5::int      Row Groups Skipped: 0
WHERE i8 < 5::bigint   Row Groups Skipped: 4

Same value, only the constant's type differs, and the refusal is now the only thing that can produce the 0.

check_num "a float >= predicate still skips nothing (NaN is not in the bounds)" "$(skipped_for "WHERE f4 >= 1.0::real")" "0" — group 0's max(f4) is 32768, so every group satisfies >= 1.0 and 0 is forced by the data. And f4 in es_c is g * 0.5 — it holds no NaN at all, so the reason in the name is not present in the fixture. The NaN rules are pinned properly on the nan_c fixture; this check is not one of them. Either rename it to what it measures, or move it to nan_h.f8a, which does carry both a NaN and bounds.

3. docs/limitations.md claims parity with other writers, and numeric is the counterexample

A table exported from pgColumnar skips on the same predicates as a file from any other writer.

Measured, same reader, same predicate, same logical data, numeric(10,2):

file physical type bounds Row Groups Skipped
ours FLBA(16) has_min=0 has_max=0 0 of 3
pyarrow, store_decimal_as_integer=True INT64 min=0 max=1638375 2 of 3

parquet_kind_for_type sends every numeric(p,s) with p<=38 to PQ_FIXED_LEN_BYTE_ARRAY, so we can never emit a skippable numeric column. The bullet directly underneath already says so — "byte-array numeric column carries a null count and no bounds" — and the untouched conditions list above says "one written as an INT32 or INT64 DECIMAL does skip." The sentence contradicts both. Dropping it costs nothing; the two bullets say the true thing.

4. [Row-group skipping](limitations.md) names a section that does not exist

There is no Row-group skipping heading in docs/limitations.md. The text is prose at line 833 under ## Reading external Parquet, and the link carries no anchor while its four neighbours in the same file do (limitations.md#import-and-export-type-coverage, sql-reference.md#import-and-export, and two more). docs_style does not check link targets, so the gate cannot catch this.

Your new export paragraph also lands under Reading external Parquet — a file we exported is not external, and a reader looking for what their own exports carry will not look there.

One ### Row-group skipping heading above line 833 fixes the link, gives the new paragraph somewhere to belong, and makes the reporter's own words in #850 ("limitations.md's Row-group skipping section"), your PR body, the CHANGELOG and parquet_export_stats.sh's header comment all resolve to a real place. Four documents already name this section as though it exists.

5. parquet_stats.py computes two fields nothing reads

has_dep_min and has_dep_max are parsed, carried through parse_statistics, and printed on every CHUNK line. Nothing in parquet_export_stats.sh asserts on either — I grepped the whole tree, the only hits are inside the instrument.

That is the one claim in the PR body the instrument was built to pin and does not: "The deprecated min (2) and max (1) are not written." And you have the positive control already, because pyarrow does write them — the pyarrow file in my numeric probe came back has_dep_min=1 has_dep_max=1. Two check_nums over countchunks and it is pinned in both directions.

6. Minor: the nine types are right, but almost none of them skip on a bare literal

The list is true. I built a 3-group fixture with disjoint ranges per group and a predicate confined to group 0, and every one skipped 2 of 3 with the row count matching the oracle:

c_i2 < (-16384+100)::smallint          2   c_d  < DATE '2000-01-11'                2
c_i4 < 100::integer                    2   c_t  < TIME '00:00:00.1'                2
c_i8 < 100000::bigint                  2   c_ts < TIMESTAMP '2000-01-01 00:01:40'  2
c_f4 < 50.0::real                      2   c_tstz < TIMESTAMPTZ '...+00'           2
c_f8 < 25.0::double precision          2

Written the way a user would write them, they do not:

c_i2 < -16284    0        c_i8 < 100000    0        c_f4 < 50.0    0

A bare integer literal is int4 and a bare 50.0 is numeric, so of the nine only integer skips without a cast. The exact-type condition is documented one paragraph above, but this new paragraph is the reassuring "it works now" text and a reader may stop at it. A half-clause — "with a constant of the column's exact type; see the condition above" — would save them.


Not this PR, but I found it running your gate

test/run_all_versions.sh is mode 100644 in git and has been since db53812 (2026-08-09). docs/testing.md gives the bare invocation five times (lines 157, 190, 218, 273, 413) and every one of them fails with Permission denied. CI is unaffected because both workflows call it as bash test/run_all_versions.sh. Your two new files are correctly 100755. I will file it separately unless you would rather fold in chmod +x.


Not approving yet, and not because any of this is hard. Findings 1 and 3 are false sentences in files that ship — the CHANGELOG and the docs site — which is the same bar I held #830 to. Push 1, 3 and 4 and I will approve on sight; 2, 5 and 6 are yours to take or leave.

The fix itself is good work, the instrument choice was right, and the reporter got a correct and complete answer.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Filed the mode problem as #852, and two things I said about it above are wrong.

The commit is 56ae5f8eb (2026-08-16), not db53812. I got db53812 from git log --oneline -3 -- test/run_all_versions.sh, which lists commits that touched the file — it does not answer which one changed the mode. Searching the actual mode transitions gives exactly one across the whole history of test/, and it is mine: a perf commit that added one line to SUITES and took the executable bit off on the way past.

And it is 103 files, not one. 95 of 248 test/*.sh and 8 of 12 test/*.py. The other 102 were born 100644 and never regressed, so it is one regression plus a long-standing habit, not a single accident. 30 documented invocations name one of them.

Nothing here changes anything about this PR — your two new files are both correctly 100755, and the matrix is unaffected because run_all_versions.sh runs suites as bash "$builddir/test/${s}.sh". #852 has the full measurement and a three-step fix; say the word and I will send it, or fold chmod +x in here if you would rather it rode along.

The five findings in my review above stand as written.

…il (#851 review)

Six findings from review, all in the claims around the code rather than
in the code.

1. The CHANGELOG said swapping min_value and max_value loses rows. It
   does not, and my own mutation log said so: all eleven FDW-result
   checks passed under that mutation. The swap inverts the interval and
   the reader already refuses to skip on an inverted one, which is
   documented behaviour that caught the mutation. One of the eight
   mutations loses rows, the date epoch shift, and the changelog now
   says which check stands between it and silent row loss.

2. Two refusal checks asserted 0 against a fixture that forces 0:
   every group's max(i8) exceeds 5 and every group's max(f4) exceeds
   1.0, so `i8 > 5::int` and `f4 >= 1.0` were unprunable whether the
   refusal existed or not. Each now runs beside a predicate differing
   in one respect that does skip. Proved load-bearing by mutating the
   refusals themselves in the reader: removing the exact-constant-type
   check reds the cross-type arm alone, 4 against 0, and removing the
   float > / >= refusal reds the float arm alone, 4 against 0.

3. docs/limitations.md claimed a file we export skips on the same
   predicates as one from any other writer. False for numeric: we write
   every numeric as a byte-array DECIMAL, which never skips, while
   another writer may store it as an INT64 DECIMAL, which does. The
   sentence is gone and the difference is stated.

4. Four documents cited a "Row-group skipping" section that did not
   exist, and the features.md link carried no anchor while its
   neighbours all do. The section exists now.

5. parquet_stats.py parsed and printed has_dep_min and has_dep_max and
   nothing asserted on either, leaving "we do not write the deprecated
   fields" unpinned. Pinned in both directions: none in our files, and
   four in pyarrow's, which does write them.

6. docs/limitations.md now says a bare integer literal is an integer
   and a bare 50.0 is a numeric, so of the nine types listed only
   integer skips without a cast.

Suite 137 of 137 and docs_style 9 of 9 on PostgreSQL 17.10.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013vWhU5eNt6dGDU8iZPrrWP
@jdatcmd

jdatcmd commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

All six addressed in c272629. Findings 1 and 3 were false sentences I shipped, and you were right about both. Taking them in the order that matters.

1. You are right, and my own log said so

I checked before editing, and the evidence was already in the mutation log I ran yesterday:

$ grep "the FDW result equals the oracle" mut850_M9.log | grep -c '^PASS'
11
$ grep "the FDW result equals the oracle" mut850_M12.log | grep '^FAIL'
FAIL  the FDW result equals the oracle for [d > DATE '2060-01-01']

Eleven of eleven pass under the swap. I read the truncated head of that log, saw 52 reds and the skip counters among them, and wrote "loses rows" for both mutations without checking which checks were red. The number was right and the claim on top of it was not.

Your explanation is the part worth keeping, and the CHANGELOG now carries it: the swap inverts the interval, the reader refuses to skip on an inverted one, and that refusal is documented. My own defence caught my own mutation. It also names d > DATE '2060-01-01' as the single check standing between an epoch shift and silent row loss, because you are right that a predicate placed past the end of the data is doing real work there.

2. Rewritten, then proved they can fail

You are right that both were forced by the fixture. max(i8) in group 3 is 1.96609e11 and max(f4) in group 0 is 32768, so i8 > 5 and f4 >= 1.0 are unprunable whether or not the refusals exist.

Each now runs beside a predicate differing in one respect that does skip. But a rewrite is not evidence, so I mutated the refusals themselves in the reader:

mutation to columnar_parquet_reader.c result
drop con->consttype != leaf->plan.typid cross-type arm alone reds, got 4 want 0; float arm and both controls pass
drop the float > / >= early return float arm alone reds, got 4 want 0; cross-type arm and both controls pass

136 PASS / 1 FAIL each, distinct .so fingerprints. Each check now fails for its own reason and nothing else's.

On the float one you are right twice: f4 holds no NaN, so the reason in the old name was not present in the fixture. The refusal does not depend on the column holding a NaN, and the name says that now. The NaN rules stay pinned where they belong, on nan_c.

3. Gone, and the counterexample is stated

parquet_kind_for_type sends every numeric(p,s) with p<=38 to PQ_FIXED_LEN_BYTE_ARRAY, so the parity sentence was false in a way a user would hit. It is replaced by the difference itself: this exporter writes every numeric as a byte-array DECIMAL and one from us never skips, while another writer may store it as an INT32 or INT64 DECIMAL, which does.

4. The section exists now

### Row-group skipping above the prose at 833, and docs/features.md links to limitations.md#row-group-skipping like its four neighbours. The new export paragraph now has somewhere to belong rather than sitting under Reading external Parquet, which was the wrong home for a file we wrote ourselves.

5. Pinned in both directions

You found the one claim the instrument was built for and did not make. Two checks over our files, none carrying field 1 or field 2, and one over pyarrow's, which does carry them, four of four. The parser's output is now measured rather than merely printed.

6. Taken

docs/limitations.md now says a bare integer literal is an integer and a bare 50.0 is a numeric, so of the nine types only integer skips without a cast, with the example spelled out. Your table is what made the case: the paragraph is the reassuring text and a reader can stop at it.


On the things you checked that the PR does not cover. The nested export, parallel_export_parquet and the four timezones are all checks I should have run and did not. The column_orders length mismatch in particular: I wrote the comment saying Arrow raises on it, and then tested only flat tables, where the count cannot be wrong in an interesting way. Thank you for closing that.

The DuckDB detail is worth recording somewhere durable: stats_min/stats_max are the deprecated fields we deliberately do not write, so a probe reading those columns reports NULL on a correct file. That is a trap laid by this very change, and you fell into it and climbed out before reporting, which is more than the PR did for you.

On run_all_versions.sh being 100644. Confirmed independently, git ls-files -s gives 100644 for it and 100755 for both new files. Please file it separately: it predates this branch by three weeks, it is not about #850, and it deserves its own removal proof rather than a chmod buried in a statistics PR.

Suite is 137 of 137 and docs_style 9 of 9 on 17.10; CI re-running now. Ready for another look.

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed at c272629, delta only (bd7bf8ce...c2726296, 4 files, +55/-14). Base is still 0e4884c1, so nothing under it moved.

All six are fixed. Verified, not read:

how I checked it
1 CHANGELOG mechanism now states the inverted-interval refusal and names d > DATE '2060-01-01'. Correct — see the one number below
2 the two refusal checks on the unfixed tree the two controls red (got [0] want [4]) and the two refusals pass. That is the right structure: the control carries the discrimination, the refusal carries the refusal
3 parity sentence gone; the numeric counterexample is stated and matches parquet_kind_for_type
4 the anchor ### Row-group skipping at limitations.md:833, slug row-group-skipping, and features.md:217 links to it
5 deprecated fields 2 negatives over our files + 1 positive over pyarrow's, all 4 green at head. The pyarrow control is what gives the two negatives teeth — on the unfixed tree they pass trivially, since a file with no Statistics has no deprecated ones either. Right call
6 the bare-literal caveat taken, and it is wrong — see 2 below, and it is my fault

Suite 137 of 137 at head. Gate on my box: preflight 5 of 5, 0 warnings; PG18 235 PASS / 2 SKIP, PG19 237 PASS / 0 SKIP, 474 PASS, 0 failures. CI 12/12. docs_style, harness_selftest, native_parquet_pushdown, parquet_export_stats all green on both majors.


1. The CHANGELOG's "52" is stale, and this commit is what staled it

The swap loses 52 checks and no rows.

Measured at c272629, mutation asserted applied and source sha1 restored after:

reds now: 54          (was 52 at bd7bf8ce)
FDW-result checks red: 0 of 11
FAIL  control: the same column skips when the constant matches its type: got [1] want [4]
FAIL  control: a float < predicate below every bound skips: got [1] want [4]

"and no rows" is exactly right. The count is not: the two extra reds are the two skip-count controls you added in this same commit, and an inverted interval breaks them too. 52 was measured before those arms existed.

This is the same trap the finding was about, one iteration down — a count quoted from a run that predates the arms. 54, or drop the number and keep the sentence, which is the part that carries the meaning anyway.

2. "Only integer skips without a cast" is wrong for two of the nine — and the rule is mine

I gave you that generalisation in my review table. I had measured exactly two bare predicates, c_i8 < 100000 and c_f4 < 50.0, both came back 0, and I wrote a rule about column types from them without checking how PostgreSQL types the literal. It is a rule about the literal's inferred type. Measured on a fixture that spans past int4, with the planner's own constant shown:

bare predicate planner's constant skipped
c_i4 < 100 100 2 of 3
c_i8 < 5000000000 '5000000000'::bigint 2 of 3
c_f8 < 25.0 '25'::double precision 2 of 3
c_f4 < 50.0 '50'::double precision 0
c_i2 < -16284 '-16284'::integer 0

All five return the oracle's row count. With the prescribed cast, all four of the failing-to-skip forms skip 2 of 3, so nothing else is going on.

Two reasons the claim breaks:

  • an integer literal too large for int4 is typed bigint, so a bigint column skips uncast whenever the constant needs the width;
  • float8 op numeric promotes the constant, because float8 is the preferred type in the numeric category, so double precision skips uncast always.

smallint and real are the two that genuinely cannot match a bare literal — no literal is ever typed int2 or float4.

And the example teaches a rule that is not the rule:

c_i8 < 100000            0 skipped     (100000 fits int4, so the literal is an integer)
c_i8 < 5000000000        2 of 3        (same column, same operator, larger literal)

c_i8 < 100000 is right for a reason the sentence does not give. Something like: "the constant's inferred type must match, and a bare literal is typed by its value — 100000 is an integer but 5000000000 is a bigint. smallint and real never match a bare literal; cast those."

Small, but while you are in there: c_i8 is the column name from my probe script, not one a reader has. Two paragraphs up the file already says bigint_col > 5::int.

3. The PR description still says everything the CHANGELOG now denies

The four lines are still live in the body above:

  • line 35 — "132 checks" (now 137)
  • line 39 — "Red before the fix: 95 of 132 failing. Green after: 132 of 132"
  • line 52 — "| swap min_value and max_value | 52, incl. the FDW dropping rows |"
  • line 56 — "The last two are the ones that matter: they lose rows rather than merely losing the skip"

The corrected figures, so you have numbers rather than just a correction — measured on author/main 0e4884c1 with your two test files copied in byte-identical (md5 matched), after asserting the tree carries 0 occurrences of the three new identifiers:

total checks: 137
RED before the fix: 97
premises passing unfixed: 12 of 12
green after: 137 of 137

Every merge in this repo's recent history is a merge commit, so the body does not reach git log — but squash is enabled on the repo, and it would if anyone used it. Either way it is the durable record on GitHub sitting beside a CHANGELOG that now says the opposite.

4. Minor, and only because you said otherwise

The new export paragraph now has somewhere to belong rather than sitting under Reading external Parquet.

It is still under it — ### Row-group skipping at line 833 is a child of ## Reading external Parquet at 778. That is fine and I would not move it now: the heading was the thing that mattered and the link resolves. Only flagging it because the sentence claims more than the diff does.


Still not approving, and I want to be plain that this is the same bar and not a moved one. Last time I said 1, 3 and 4 and I would approve on sight. You pushed all six and I would have. What stops me is that the fix introduced two new false sentences in files that ship — a wrong count in the CHANGELOG and a wrong rule in limitations.md — and holding for those is the identical test I applied to the first two.

The wrong rule is mine. I handed it to you from two measurements and a generalisation I had no business making. If you would rather not touch the branch again for it, say so and I will open a follow-up against main carrying both fixes, the way #847 went.

Everything substantive about this change is now right, and finding 2's rewrite is better than what I asked for — pairing each refusal with a one-difference control, then mutating the reader to show each fails for its own reason, is the version I should have proposed.

 review)

Two false sentences, both introduced by the commit that fixed the last
two.

The count. The CHANGELOG said swapping the bounds loses 52 checks. It
loses 54 at this head, because the two skip-count controls added in the
same commit are broken by an inverted interval too, and 52 was measured
before those arms existed. The same trap as the finding it came from,
one iteration down: a number quoted from a run that predates the arms.

The rule, which was wrong for six of the nine types. docs/limitations.md
said only integer skips without a cast. Measured with the planner's own
constant printed, on a fixture whose bigint column spans past int4:

  c_i4 < 100                  100                          skips 3 of 4
  c_i8 < 5000000000           '5000000000'::bigint         skips 3 of 4
  c_i8 < 100000               100000                       skips 0
  c_f8 < 25.0                 '25'::double precision       skips 3 of 4
  c_f4 < 50.0                 '50'::double precision       skips 0
  c_i2 < -32668               '-32668'::integer            skips 0
  d  < '2000-01-05'           '2000-01-05'::date           skips 3 of 4
  t  < '00:00:01'             '00:00:01'::time             skips 3 of 4
  ts < '2000-01-01 00:30:00'  ::timestamp                  skips 3 of 4
  tz < '2000-01-01 00:30:00+00' ::timestamptz              skips 3 of 4

A bare literal is typed by its own value, not by the column. A quoted
date, time or timestamp literal is unknown-typed and resolves to the
column's type, so all four temporal types skip as written. An integer
literal too large for int4 is a bigint, so a bigint column skips when
the constant needs the width and not otherwise. A decimal literal is
numeric and promotes to double precision, which is the preferred type
in that category. Only smallint and real can never match a bare
literal, because no literal is ever typed int2 or float4.

The documented cross-type trap still holds: ts >= DATE '2026-01-01'
skips nothing, an explicitly typed constant of the wrong type.

The example also used a column name from the reviewer's probe script
rather than one a reader has; it now matches bigint_col two paragraphs
above.

Suite 137 of 137 and docs_style 9 of 9 on PostgreSQL 17.10.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013vWhU5eNt6dGDU8iZPrrWP
@jdatcmd

jdatcmd commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

All three fixed in a3ca579, and the PR body is rewritten. Both of your counts reproduce exactly.

1. 52 is stale, and it is 54

Reproduced at c272629, mutation asserted applied:

total checks: 137
reds: 54          FDW-result reds: 0 of 11
FAIL  control: the same column skips when the constant matches its type: got [1] want [4]
FAIL  control: a float < predicate below every bound skips: got [1] want [4]

The two extra reds are exactly the two controls I added in that commit, and an inverted interval breaks them for the same reason it breaks everything else. You are right that this is the original finding one iteration down: I quoted a number from a run that predated the arms it was counting. The CHANGELOG says 54.

2. The rule was wrong, and wronger than you found

Do not take this one back. You gave me a generalisation from two measurements and said so plainly; I put it in a shipping document without measuring it, which is the failure. And it is wrong for six of the nine, not two, because neither of us checked the temporal types.

Measured with the planner's own constant printed, EXPLAIN VERBOSE on a fixture whose bigint column spans past int4:

bare predicate planner's constant skipped
c_i4 < 100 100 3 of 4
c_i8 < 5000000000 '5000000000'::bigint 3 of 4
c_i8 < 100000 100000 0
c_f8 < 25.0 '25'::double precision 3 of 4
c_f4 < 50.0 '50'::double precision 0
c_i2 < -32668 '-32668'::integer 0
d < '2000-01-05' '2000-01-05'::date 3 of 4
t < '00:00:01' '00:00:01'::time 3 of 4
ts < '2000-01-01 00:30:00' ::timestamp 3 of 4
tz < '2000-01-01 00:30:00+00' ::timestamptz 3 of 4

A quoted literal is unknown and resolves to the column's type, so all four temporal types skip as written. That is four more that my sentence told a reader to cast unnecessarily.

So the rule is: a bare literal is typed by its own value, not by the column. integer and double precision skip. The four temporal types skip. bigint skips only when the literal is too large for an integer, which is why bigint_col < 5000000000 skips and bigint_col < 100000 does not. Only smallint and real can never match a bare literal, because no literal is ever typed int2 or float4.

The documented cross-type trap still holds, measured on the same fixture: ts >= DATE '2026-01-01' skips 0.

c_i8 is gone; the example uses bigint_col, matching the paragraph above it.

3. Body rewritten

132 to 137, 95 to 97, twelve premises, and the mutation table now carries a "loses rows?" column whose answer is yes exactly once. Your corrected figures are in it, attributed to the run rather than to me remembering them.

4. You are right, and I overclaimed

### Row-group skipping at 833 is a child of ## Reading external Parquet at 778. The paragraph did not move; only the heading appeared. My sentence claimed the placement was fixed and it was not, and I would rather that be noted than quietly true-ish. Agreed it should stay where it is.


Suite 137 of 137, docs_style 9 of 9 on 17.10, CI re-running.

Three rounds, and each has caught a false sentence of mine in a file that ships. The bar has not moved and I would not want it to: this change's whole claim is that a bound a reader trusts must be right, and a changelog is read the same way.

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed at a3ca5793, delta only (c2726296...a3ca5793, 2 files, +7/-4). Base still 0e4884c1.

Approving. The code has been right since bd7bf8ce and three rounds of me trying to break it have not. Gate at this head, third run: preflight 5 of 5, 0 warnings; PG18 235 PASS / 2 SKIP, PG19 237 PASS / 0 SKIP, 474 PASS, 0 failures; suite 137 of 137. CI 12 of 12 completed and successful (checked on .status, not .conclusion, which is "" and not null while a check runs). CHANGELOG says 54. The PR body carries 137 / 97 / twelve premises and a "loses rows?" column answering yes exactly once.

You were right that I had it wrong in the other direction too, and the temporal cases are real — I confirmed all four independently. Two things below, neither of which I am holding the approval on, and I want to be explicit that the second is a proportionality call rather than a claim that the sentence is fine.

1. The quoted-literal mechanism is not temporal — it is every type

A quoted date, time or timestamp literal takes the column's type, so those skip as written. ... No literal is typed smallint or real, so those two always need a cast.

The first sentence is true and the reason is not confined to temporal types: a quoted literal is unknown and resolves to the other operand's type, whatever that is. So the second sentence does not follow. Every cell, three literal forms across nine types, planner's own constant printed, three row groups with the predicate confined to group 0, and every row's count matched the oracle:

unquoted quoted cast
smallint '-16284'::integer0 '-16284'::smallint2 2
integer 100 → 2 100 → 2
bigint 1000000; '5000000000'::bigint → 2 '100000'::bigint2 2
real '50'::double precision0 '50'::real2 2
double precision '25'::double precision → 2 2
date/time/timestamp/timestamptz n/a resolves to the column's type → 2 2

smallint_col < '100' and real_col < '50.0' both skip. So "those two always need a cast" is false, and the four temporal types are not a special case — they are the general case, which only looks special because a date is always written quoted.

The whole rule in one shape: an unquoted numeric literal is typed by its value; a quoted literal is unknown and takes the column's type. Unquoted, integer and double precision match, bigint matches only when the value needs the width, smallint and real never. Quoted or cast, all nine match.

The documented cross-type trap still holds on the same fixture: c_ts >= DATE '2026-01-01' skips 0, c_i8 > 5::int skips 0.

Why I am not holding for this. Rounds 1 and 2 were sentences that would change what a reader concluded about the software — the parity claim would send someone hunting a bug that is not there. This one tells a reader to write a cast that works. It is wrong, and it is wrong in the safe direction, and I now have every cell measured so the correction is mechanical rather than another partial rule from a partial sample. Holding a fourth round over an over-cautious sentence would be raising the bar, not keeping it where it was, and #850's reporter has been waiting since this morning. Fix it forward.

2. The mutation table is stale beyond the row I reported — the whole table predates the arms

I gave you one number and you fixed one row. The cause applies to the table:

"delete the Statistics emission"   PR body says 93     measured at a3ca5793: 95
FAIL  control: the same column skips when the constant matches its type: got [0] want [4]
FAIL  control: a float < predicate below every bound skips: got [0] want [4]

Same two controls, same reason as 52 → 54. write min_value but not max_value is very likely 40 rather than 38 for the same reason; I did not run the remaining six. This is the PR body, which does not ship and which merge-commit history will not carry, so it is not a merge concern — but the fix is the class, not the instance: re-run the eight once at final head, or say which suite revision the counts were taken at, the way line 49 already does for the baseline.

What I checked this round and found nothing wrong with

### Row-group skipping is where we agreed it stays. The c_i8 name is gone and the example reads bigint_col. The CHANGELOG's mechanism paragraph is accurate as written, including "and no rows", which I re-measured at 0 of 11 FDW-result checks red under the swap.


Three rounds, three false sentences, and each one you found in your own evidence once you looked at which checks were red rather than how many. That is the same discipline the change itself is about, and it is why I am comfortable approving code whose whole promise is that a bound a reader trusts is right.

Merging is yours. #852 is open separately for the executable bits and does not touch this.

`docs/limitations.md` told a reader that `smallint` and `real` "always need a
cast". They do not. A quoted literal is `unknown` and resolves to the other
operand's type, so `smallint_col < '500'` skips and `smallint_col < 500` does
not. The rule was also stated as though it were special to `date`, `time` and
`timestamp`. It is not: those literals are simply always quoted, which is why
they skip as written.

The corrected rule, measured across the nine skippable types in three literal
forms, planner's constant printed beside every skip count, and every row count
compared with a heap oracle:

- quoted or cast, all nine match the column and skip;
- an unquoted digit string is an `integer`, or a `bigint` where the value is too
  wide for one, so it matches `integer` and `double precision` always and
  `bigint` only at the wider values;
- an unquoted literal with a decimal point is a `numeric`, matching
  `double precision` alone, and against an integer column PostgreSQL casts the
  column rather than the constant;
- `smallint` and `real` match no unquoted literal at all.

Nine checks in `test/parquet_export_stats.sh` pin it, 137 to 151 including five
further FDW-against-oracle arms. Each pair differs in one respect, and each
check was shown able to fail. Delete the reader's exact-constant-type refusal
and the three arms that require a skip count of 0 read a positive one instead;
stop the writer emitting statistics and the four arms that require a skip read 0;
swap the two plan-shape checks' patterns and both go red.

One candidate check was dropped rather than shipped. Asserting that
`i8 < 5.0` skips nothing reads 0 for two independent reasons -- the matcher
rejects a left argument that is not a bare `Var`, and the exact-type test would
reject a `numeric` constant -- so no single mutation could turn it red. What
replaced it asserts the plan shape the documentation actually claims.

The wrong rule was mine to ship; it came from a review table built on two
measurements, and this branch measures every cell.
@jdatcmd

jdatcmd commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Both items from your approval are addressed at 71f8243, which is a new head after you approved a3ca5793. Nothing in src/ changed: the diff is docs/limitations.md, CHANGELOG.md, and nine new checks.

1. The rule now says what you measured

You were right that the mechanism is not temporal. docs/limitations.md now leads with it: how the literal is written decides its type, and therefore whether the exact-type condition is met at all. Quoted or cast, all nine skippable types skip, because a quoted literal is unknown and takes the column's type. A quoted temporal literal is that general case, not an exception to it. Unquoted, a digit string is an integer — or a bigint where the value is too wide for one — and a decimal-point literal is a numeric, against which PostgreSQL casts the column rather than the constant. The sentence you flagged, that smallint and real "always need a cast", is gone.

Nine checks pin it, so the next revision of that paragraph has to survive a suite rather than a reviewer. Each pair differs in one respect — a pair of quotes, or the width of the value — so the arm reading 0 is refusing rather than having nothing to prune:

check skips
i8 < '5' quoted, takes the column's type 4
i8 < 5 same digits unquoted, an integer 0
i8 < 5000000000 too wide for an integer, so a bigint 3
f4 < '0.4' quoted 4
f4 < 0.4 unquoted, double precision 0
i2 < '500' quoted 1
i2 < 500 same digits unquoted 0
i8 < 5.0 plan carries ::numeric on the column 1 (plan shape)
i8 < '5' plan carries ::bigint on the constant 1 (plan shape)

The i2 arms skip 1 rather than 4 because i2 is deliberately not ascending in this fixture; one skipped group is still a skip that a cross-type constant cannot produce. Five of the predicates also joined the FDW-against-oracle loop, which is now sixteen arms.

One candidate check was dropped rather than shipped. Asserting that i8 < 5.0 skips nothing reads 0 for two independent reasons — the matcher rejects a left argument that is not a bare Var, and the exact-type test would reject a numeric constant — so no single mutation could turn it red. It is replaced by the two plan-shape checks above, which assert what the documentation actually claims.

2. The table is re-run, and the rule is now the class

You asked for the class, not the instance. The whole removal proof was re-run at 71f8243 against the suite at 151 checks — the eight writer mutations, the two reader mutations, and a fixture mutation. Baseline and restore are the same binary, 77127539e8f3, at 151 of 151 both times; the eight mutants carry eight further distinct fingerprints.

mutation 137-check suite 151-check suite loses rows?
delete the Statistics emission 95 99 no
write min_value but not max_value 40 44 no
let a null into the accumulator as zero 15 15 no
let NaN reach the bounds 3 3 no
delete column_orders 2 2 no
swap min_value and max_value 54 57 no
stop resetting bounds per row group 26 28 no
take the date bound from the PostgreSQL epoch 10 10 yes

Every move is accounted for by the nine new checks, and by which of them the mutation reaches: four each for the two that remove the bounds outright, three for the swap (the smallint arm survives it: the one group it skips is a single row, so exchanging that group's minimum and maximum exchanges a number with itself), two for the reset. The epoch arm still reds exactly one FDW-result check, d > DATE '2060-01-01', and the other seven red none.

The reader and fixture arms, same head:

  • remove the exact-constant-type test → 4 reds: the cross-type arm and three of the new ones, each reading a positive skip count where 0 is required.
  • remove the float > / >= refusal → 1 red, a float >= predicate above every bound still skips nothing, at got [4] want [0].
  • swap the two plan-shape checks' EXPLAIN patterns → 2 reds, both of them and nothing else.

The RED-before count moved too, and it is re-measured rather than carried: on 0e4884c1 with the two test files copied in byte-identical, 101 red and 50 green, 101 + 50 = 151. The twelve premises still pass on the unfixed tree, and so does control: the quoted form casts the constant to the column's type, which is a planner fact and independent of anything we write.

Where this leaves the approval

Your approval is against a3ca5793; this is 71f8243. CI is 12 of 12 on the new head (run 33419678987, headSha checked), with parquet_export_stats=PASS inside both suites listings — 228 ran, 9 skipped on PG 17, 231 ran, 6 skipped on PG 18. Merging is still the owner's call, and I am not merging on an approval that predates the head.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Round 4, at 71f8243. My APPROVE was submitted at a3ca579. GitHub carried it
forward to this head; treat it as not covering 71f8243.
One sentence this
commit adds is false, and the page contradicts it twenty lines above.

What I re-measured, and it all holds

Independently, on PG18a, against a fixture of my own — 262,144 rows, four row
groups, nine columns, one per skippable type, every literal derived from the row
that opens group 2 so no arithmetic of mine could put the constant in the wrong
place:

  • Quoted, all nine skip. int2 int4 int8 float4 float8 date time timestamp timestamptz, three of four groups each, with the cast form beside each one as
    a control also at three. Your corrected sentence is right, including the two
    the earlier revision got wrong: c_f4 < '32768' gives '32768'::real and
    skips 3.
  • Unquoted, exactly as you now state it. c_i2 < -8192 → 0,
    c_f4 < 32768 → 0, c_i4 < 65537 → 3, c_i8 < 65537000000000 → 3,
    c_i8 < 65537 → 0, c_f8 < 32768 → 3, c_f8 < 32768.5 → 2.
  • The decimal-point literal casts the column, and the planner says so.
    (gft.c_i8)::numeric < 32768.5, and the same for c_i4 and c_i2. Your two
    plan-shape checks are asserting the thing that is actually true.
  • The nine checks, 3 / 4 / 2, exactly. Reader's exact-constant-type test
    deleted → 4 reds, three of them among the nine. Writer's statistics emission
    deleted → 99 reds, four among the nine. Plan-shape patterns swapped → 2 reds
    and nothing else, 149/2 of 151. Your table's 99 for the Statistics row is
    the number I get.
  • Baseline at this head: 151 PASS, 0 FAIL, 137 + 14.

My first cut at the statistics mutation removed write_statistics(b, c, m) and
left PutField(b, &mlast, 12, TC_STRUCT) above it. That is a dangling field
header, the footer stops parsing, and the suite dies before any check — 0 PASS,
1 FAIL. Not a measurement of anything. Both lines have to go, and then it is 99.

And you fixed the class, not the instance. Last round I reported one stale
row and you re-ran all eight, with the arithmetic of the growth written out.
95 → 99 is right.

The finding: "any quoted temporal literal skips as written" is false

docs/limitations.md, new text:

Quote the literal, or cast it, and all nine types skip: a quoted literal is
unknown and takes the column's type. That is why any quoted temporal
literal skips as written.

The page's own conditions list, twenty lines above, in text this PR does not
touch:

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.

DATE '2026-01-01' is a quoted temporal literal. Both sentences cannot be true.
Measured, and the second one is:

PREDICATE                                SKIPPED  FDW ROWS  HEAP ROWS
c_d  < DATE '2179-06-08'                    3      65536      65536
c_d  < '2179-06-08'                         3      65536      65536
c_d  < TIMESTAMP '2179-06-08 00:00:00'      0      65536      65536
c_ts >= TIMESTAMP '2100-01-01 00:00:00'     4          0          0
c_ts >= DATE '2100-01-01'                   0          0          0
c_tz >= TIMESTAMPTZ '2100-01-01 00:00:00+00' 4         0          0
c_tz >= DATE '2100-01-01'                   0          0          0

The first three lines are the ones that matter, because they are not degenerate:
the same 65,536 rows every time, and the same answer as the heap. Two forms
prune three of four row groups; the third prunes none. Nothing is lost — the
counts agree — but a reader who writes the third form is told by this page that
they get the first form's pruning, and they do not.

The mechanism you describe is right; the word is "any". DATE '...' is not
unknown, it is a date, so it follows the exact-type rule like every other
adorned constant. Only the unadorned quoted string takes the column's type.

I do not think this is a nit, and the reason is #850 itself: the reporter's
whole experience was "every documented condition holds and Skipped stays 0". A
reader with a timestamp column who writes ts >= DATE '2026-01-01', on the
strength of this sentence, arrives at exactly that place a second time.

Suggested, one clause:

That is why an unadorned quoted temporal literal skips as written. A literal
that names its own type — DATE '2026-01-01' — is that type, not unknown,
and follows the exact-type rule: against a timestamp column it does not
skip.

Minor, yours to take or leave

"It matches integer and double precision always." The integer half is
false in the same way the bigint half is true: a digit string too wide for an
int4 is a bigint.

c_i4 > 5000000000    skipped 0    ((gft.c_i4 > '5000000000'::bigint))
c_i4 > 196608        skipped 3    ((gft.c_i4 > 196608))

I am reporting it as wording only, and here is why it is not more than that:
every predicate that exhibits it is degenerate. A constant outside int4 range
makes the comparison constant-true or constant-false on an int4 column, so no
reader loses pruning they could have had. "at every value an integer column
can hold" would close it, or dropping "always". The double precision half I
could not falsify: c_f8 < 32768, c_f8 < 32768.5, c_f8 < 5000000000 and
c_f8 < 99999999999999999999999 all produce a double precision constant.

Where that leaves it

Fix the one clause and I re-approve on sight; the minor is yours. Everything
else in this commit I checked and it is right, including the nine checks and the
whole re-measured mutation table. CI was 11 of 12 green with one build leg still
running when I looked.

Nothing here touches src/. Measured on PG18a assert, .so 8c0ca9567ecc.

Round 4 of review found a sentence this branch added that the same page
contradicts twenty lines above, and it is the second one that is true.

  new, mine:  "That is why any quoted temporal literal skips as written."
  existing:   "A cross-type comparison such as ts >= DATE '2026-01-01'
               against a timestamp column ... does not skip."

`DATE '2026-01-01'` is a quoted temporal literal, so both cannot hold. The
mechanism was right and the word "any" was wrong. Only an UNADORNED quoted
literal is `unknown` and takes the column's type. A literal that names its own
type is that type, and the exact-type condition applies to it like any other
constant. Measured on the suite's own fixture, one date written three ways:

  d < DATE '2000-01-05'                   3 of 4 groups skipped
  d < '2000-01-05'                        3 of 4
  d < TIMESTAMP '2000-01-05 00:00:00'     0 of 4

The same reviewer's minor is fixed with it: an unquoted digit string is an
`integer` only while the value fits in one. Against an int4 column a wider
literal is a `bigint`, and so is refused:

  id < -1                                 4 of 4 groups skipped
  id < -5000000000                        0 of 4

Both arms return no rows, so the pair measures the refusal rather than the
data: on identical output one prunes every group and the other prunes none.

Seven checks, 151 to 162, four of them further FDW-against-oracle arms. Six are
falsified by a single mutation: two go red when the writer stops emitting
statistics, one when the reader's exact-constant-type refusal is deleted, one
more of the width pair under that same mutation, and the two plan-shape checks
when the patterns they grep for are swapped.

The seventh is not, and it says so in its own comment rather than leaving a
reader to find out. `d < TIMESTAMP ...` reads 0 under both mutations: with the
type refusal deleted, the timestamp datum read as a date lands outside this
fixture's range in the direction that prunes nothing. It is kept as a
documented-behaviour guard, and what carries the evidence beside it is the
planner's own output, which names the constant's type.

The paragraph is now three labelled cases rather than one block of prose. That
is not decoration: the previous shape is what let a general rule and its two
exceptions sit in the same sentence, which is how "any" got in.
@jdatcmd

jdatcmd commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed at 50f5d7c. You were right, and the second sentence was the true one.

The sentence

docs/limitations.md no longer says "any". The mechanism was right and the word was wrong: only an unadorned quoted literal is unknown and takes the column's type. A literal that names its own type is that type, and the exact-type condition applies to it like any other constant.

The paragraph is now three labelled cases — quoted with no type named, a named type, unquoted. That is not presentation. The old one-block shape is what let a general rule and its two exceptions share a sentence, which is how "any" got in, and it is the second time this branch has shipped a wrong sentence out of that same shape.

Measured on this suite's own fixture, one date written three ways, and the width pair beside it:

d < DATE '2000-01-05'                  3 of 4 skipped      id < -1             4 of 4
d < '2000-01-05'                       3 of 4              id < -5000000000    0 of 4
d < TIMESTAMP '2000-01-05 00:00:00'    0 of 4

Both id arms return no rows, so that pair measures the refusal and not the data: on identical output one prunes every group and the other prunes none. Your minor is fixed with it — a digit string is an integer only while the value fits in one.

Seven checks, 151 to 162, four of them further FDW-against-oracle arms, which is now twenty.

The one arm that cannot fail, said in the file

the same date adorned as a timestamp does not skip reads 0 under both mutations. With the exact-type refusal deleted, the timestamp datum read as a date lands outside this fixture's range in the direction that prunes nothing, so it reports 0 for a second reason. Its own comment says that, where the next reader meets it, and the two plan-shape checks beside it carry the evidence instead: swap their patterns and both go red.

The table, re-run at the final head

Everything below is 50f5d7c, suite at 162. Baseline and restore are the same binary, 5949989c4981, at 162 of 162 both times; the eight writer mutants carry eight further distinct fingerprints.

mutation 137 151 162 loses rows?
delete the Statistics emission 93 99 102 no
write min_value but not max_value 38 44 47 no
let a null into the accumulator as zero 15 15 17 no
let NaN reach the bounds 3 3 3 no
delete column_orders 2 2 2 no
swap min_value and max_value 52 57 60 no
stop resetting bounds per row group 26 28 30 no
take the date bound from the PostgreSQL epoch 10 10 12 yes, 1 of 20

Reader and fixture arms at the same head: exact-constant-type test removed → 5 reds, now including digits too wide for an integer are a bigint, so an int4 column refuses; float > / >= refusal removed → 1; numeric/bigint patterns swapped → 2; date/timestamp patterns swapped → 2. RED-before, re-measured on 0e4884c1 with the two test files copied in byte-identical: 104 red, 58 green, 104 + 58 = 162.

Your note about the body was right when you sent it and is now stale in the other direction: the body carries all of the above.

Two things you found that I did not

double precision at bigint width. I wrote "it always matches double precision, whose operand is widened" and could not test it on this fixture, which has no float8 column. You rebuilt yours so a constant past int4 max sits inside the range and got 3 skipped on the unquoted bigint-width form. That is the non-degenerate case, and it is the sentence I would otherwise have shipped on reasoning alone.

Re-reading the whole section rather than the hunk. A PR can falsify a line it does not touch, which is exactly what this one did twenty lines up. That is the check that caught this round, and it is worth more than the fix.

docs_style caught my first draft, three sentences over the 25-word limit, which is the other reason the rule is a list now.

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

APPROVE at 50f5d7c. Round 5. Both round-4 findings are fixed, and this is a
review of this head — my earlier APPROVE was submitted at a3ca5793 and
GitHub carried it forward twice.

The finding is closed, and I checked it the way it was found

docs/limitations.md now says, in the Named type case, that
timestamp_col < DATE '2026-01-01' does not skip — which is what the same page's
conditions list has said all along. I re-read the whole ### Row-group skipping
section at this head rather than the hunk, because a push can falsify a line it
does not touch. No contradiction left.

The three-case shape is the right fix rather than a wording patch: the old
one-block paragraph is what let a general rule and its two exceptions share a
sentence.

Verified, not read

Baseline at 50f5d7c: 162 checks, 162 PASS, 0 FAIL on PG18a. 151 + 11 = 162,
derived before reading your body.

Which mutation kills each of the seven new non-oracle checks, run here:

mutation total reds of the eleven new
reader: delete the exact-constant-type refusal 5 1digits too wide for an integer are a bigint, so an int4 column refuses, got [4] want [0]
writer: delete the statistics emission (both lines) 102 3 — the two date-skip arms and the int4 control
fixture: swap the new date/timestamp plan-shape patterns 2 2, and nothing else
none of them 1the same date adorned as a timestamp does not skip

1 + 3 + 2 + 1 = 7. Your table's 102 for the Statistics row, your 5 for the
reader mutation and your 2 for the pattern swap are the numbers I get.

Your self-declaration is exactly right, and I tried to break it. I ran the
reader mutation specifically to kill the same date adorned as a timestamp does
not skip
, and it stays green for the reason your comment gives. Keeping it is
the right call: the rule is that a check which cannot fail is not evidence, not
that it must be deleted, and the plan-shape pair beside it carries the evidence.
Deleting it would remove the documented-behaviour guard for the exact predicate
this round was about.

Your int4 pair is better than my review was. I wrote that every predicate
exhibiting the width limit is degenerate, and offered you a control that differed
in value and type. id < -1 (4 of 4) against id < -5000000000 (0 of 4)
differ in one thing and return identical output — no rows either side — so the
pair measures the refusal and not the data. That is the isolation I said could
not be built on an integer column.

The one word left, attacked

It always matches double precision, whose operand is widened.

My round-4 grid could not test that: c_f8 topped out at 131072, so every
constant past int4 range was degenerate. Rebuilt with c_f8 spanning
1e6..2.62e11 so a wide constant sits inside the range:

c_f8 < '65537000000'::float8    3 skipped   control
c_f8 < 65537000000              3 skipped   ('65537000000'::double precision)
c_f8 < 1500000                  3 skipped   integer-width, returns 1 row
c_f8 < 65537000000.5            2 skipped   numeric, decimal point
c_i4 < 65537   3 skipped        c_i4 < 5000000000   0 skipped

Every form resolves to a double precision constant. I could not falsify
"always".

Regression

CI 12 of 12 SUCCESS, run 33428795842, matched on head_sha rather than
assumed. parquet_export_stats=PASS in both suite listings — 231 ran, 6 skipped on PG 18 and 228 ran, 9 skipped on PG 17 — so the suite ran rather
than merely being registered. MERGEABLE/CLEAN.

Merge

Agreed order: #851, then #853, then #855, each of mine rebasing on main in
turn. All three pairs conflict in CHANGELOG.md and nothing else; I confirmed
that with git merge-tree --write-tree myself.

The merge is yours. I approve, and I merge nothing.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Not a withdrawal — my approval at 50f5d7c stands. Two cells in the new
three-revision table are wrong, and they are the two this review already
corrected once.

The 137 column:

mutation table says measured
delete the Statistics emission 93 95
swap min_value and max_value 52 54

Re-measured just now, not quoted from my earlier comment — I am contradicting a
published number for the second time on the same row, so it deserved a fresh run.
A detached worktree at a3ca5793, clean, PG18a:

baseline                                    checks 137   137 PASS    0 FAIL
delete the Statistics emission (both lines) checks 137    42 PASS   95 FAIL   42 + 95 = 137
swap min_value and max_value                checks 137    83 PASS   54 FAIL   83 + 54 = 137
                                            oracle arms red under the swap: 0

The swap's 0 oracle arms also re-confirms the "loses rows? no" cell beside it.

These are the same two cells as round 3: the table said 93 and 52, I measured 95
and 54, and your 71f8243 body carried the corrected 95 → 99. Rebuilding the
history as a column appears to have re-derived the first entry from the original
table text rather than from the corrected value, so the correction survived in
one place and not the other.

Worth saying plainly, because the column exists to make the growth auditable:
every other cell in it I either measured myself or can reconcile. 102, 5,
2 and 2 at this head are the numbers I get, 99 at 71f8243 is the number I
got last round, and the growth arithmetic works from 95 and 54 as well as it does
from 93 and 52 — it is the starting values that are wrong, not the moves.

Fix forward whenever; it changes nothing about the code, the docs, or the merge.

@jdatcmd

jdatcmd commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in the body, and it is three cells rather than two: write min_value but not max_value had the same fault, 38 where 40 belongs. You flagged the two you re-ran; the third is the same row class and the same cause.

The table now has four columns, labelled by suite size rather than by head, because a count belongs to a suite revision and that is the whole point of the round-3 finding:

mutation 135 137 151 162
delete the Statistics emission 93 95 99 102
write min_value but not max_value 38 40 44 47
let a null into the accumulator as zero 15 15 15 17
let NaN reach the bounds 3 3 3 3
delete column_orders 2 2 2 2
swap min_value and max_value 52 54 57 60
stop resetting bounds per row group 26 26 28 30
take the date bound from the PostgreSQL epoch 10 10 10 12

135 is before c272629. What I had done was print the 135 column under a heading of 137, so the three rows those two control arms move were each one revision stale — in a table whose entire purpose was to stop exactly that.

Your diagnosis of the mechanism is right and it is worth writing down. Fix the instance, fix the class, then rebuild the presentation from the pre-fix source. The correction survived in the prose paragraph it was written into and died in the table that replaced it, because I re-derived the history from the original text instead of from the corrected values. The lesson is not "be careful"; it is that a corrected number has to be carried in exactly one place, and everything else has to be generated from it or re-measured. Labelling the columns by suite size is the closest this table gets to that: a cell now has to name which suite it counted, so a stale one is visible without a reviewer holding the history in their head.

Thank you for re-running 95 and 54 rather than quoting your own round-3 comment. Contradicting a published number for the second time on the same row deserves a fresh measurement, and 42 + 95 = 137 and 83 + 54 = 137 are what makes them checkable rather than remembered — including the 0 oracle arms under the swap, which re-confirms the "loses rows? no" cell beside it.

The 151 and 162 columns are my own runs at those heads. Nothing else in the body changed, and nothing in the tree changed: the head is still 50f5d7c, CI 12 of 12.

@jdatcmd
jdatcmd merged commit be834d7 into main Aug 31, 2026
12 checks passed
@jdatcmd

jdatcmd commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

135 was wrong and 132 is right. I re-ran it rather than take the number, one clean tree per revision, parquet_export_stats on PG 17.10:

bd7bf8c (c272629^)  132 checks, 132 PASS, 0 FAIL
c272629             137
a3ca5793            137
71f8243             151
50f5d7c             162

The body's table now reads 132 / 137 / 151 / 162, and the row is otherwise unchanged: 93 / 95 / 99 / 102.

Where 135 came from is the part worth writing down. I derived it — 137 minus the two control arms named in review — in the same edit that argued a count belongs to the revision it counted and must not be re-derived. The rule was correct and I broke it in the act of writing it.

Your second point is the sharper one: c272629 moved the suite 132 → 137, which is five checks. The commit adds six check call sites and removes two, and the two control arms the review names are among them. So even "two arms therefore two checks" would have been wrong in both directions. A number that came out of a run is the only kind that can label a column.

That is now three corrections on one table, and the third was mine to make in the same breath as the rule. The merge is unaffected — this is the PR body, the tree at 50f5d7c is what merged as be834d7, and main verifies at 162 PASS / 0 FAIL with harness_selftest 168 and docs_style 9.

OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Aug 31, 2026
…pt#852)

commandprompt#851 landed first and added test/parquet_export_stats.sh and test/parquet_stats.py,
so the top-level population under test/ is 262 at the base this branch now sits
on, not the 260 measured against 0e4884c. Both new files are 100755, so the
numerator is unchanged: 103 flagged either way, 102 with a shebang and no bit
plus crlf_listener.py with neither.

Re-measured on the rebased base rather than carried over, in both places that
name it, and the selftest header now says which commit moved it. A sentence
about a population is a measurement of a tree, and this branch changed trees.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Aug 31, 2026
…ompt#854)

Rebasing onto fe1f3a2 moved a number in two of the three places that name it,
and left the third alone. The difference is the point.

  test/selftest/310-...  "was tracked at 0e4884c ... renamed in e9de048, 135
                         commits earlier"        STILL TRUE, unchanged. It names
                         the commit it measured at, so the rebase cannot touch it.
  CHANGELOG.md           "renamed 135 commits earlier", unanchored. 146 at the
                         base this now sits on. Rewritten to name both commits.
  CONTEXT.md             "outlived its source by 135 commits", unanchored, in a
                         document read indefinitely. The count is decoration
                         there; the mechanism is the point, so it is gone.

Nothing about the defect or the fix changed: the tracked artifact, the two-byte
PEP 552 rewrite in cec104f, the ignore rules and the guard are all as they were.
This is only the arithmetic that measures a distance between two commits, and the
far end moved.

Two other counts in the pull request body are measurements of the same tree and
have moved with it: 26 tracked .py files is now 27, since commandprompt#851 added
test/parquet_stats.py, and compiling by hand now produces 27 .pyc across three
__pycache__ directories rather than the twelve entries git status showed against
0e4884c. The population this change removes is unmoved and re-measured: exactly
one tracked build artifact at fe1f3a2, and zero after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014SNUrAGntEBQXuhcK2qeo9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pgcolumnar_parquet FDW: Row Groups Skipped stays 0 on a fully sorted, correctly-typed bigint predicate

2 participants