test: port native_groupagg to pytest, with a cost arm that can fail (#289, #432) - #1167
Conversation
…ommandprompt#289, commandprompt#432) 33 names plus one template, `missing: 0`. The grouped vectorized aggregate is held to two oracles that are not substitutes for each other. A heap mirror holds the same rows, so exact aggregates must agree byte for byte, but it cannot judge float summation order -- the two storage types do not scan in the same order and float addition is not associative. A toggle differential runs the same query over the SAME columnar rows with the path off and on, and both arms read in the same order, so even float sums must be identical. That is what validates the order-preserving accumulators. Every comparison carries a node premise. A query the node quietly rejects runs the scalar Agg in BOTH arms, so the comparison is two identical runs agreeing with each other -- which is how sum(real) returning 0 got through an earlier version of the shell suite. ONE ARM IS STRONGER THAN THE ORIGINAL, BECAUSE THE ORIGINAL'S CANNOT FAIL (commandprompt#1162). `native_groupagg.sh` asks ten aggregates over ten DIFFERENT columns to cost more than one over one, and a scan projecting ten columns costs more whatever the folding charge is. Measured with the charge stripped of its aggregate-count factor, the exact defect commandprompt#349 fixed: 10 aggregates over 10 columns control 1253.01 mutant 803.01 still green 10 aggregates over 1 column control 800.50 mutant 350.50 reddens This file holds the projection fixed and forces the path, so the difference is exactly the term under test: 450.00 = cpu_operator_cost x 20,000 rows x 9 extra aggregates. Forcing matters -- with the charge applied the planner prices that shape out of the grouped path, so an unforced comparison changes which node it reads. Two premises assert both costed plans ARE the grouped node. Independent at every seam: the shell greps EXPLAIN text for the marker line while this reads FORMAT JSON and asks whether a node carries the PROPERTY; the shell compares server-side md5 while this compares sorted row tuples in Python; the shell sets the GUC on the DATABASE because each q() opens a connection, while this sets it on the session it holds. Proved by removal against src/columnar_vector.c on the Debian PostgreSQL 18 the CI cluster job uses. Control 76 checks passed; each mutation asserted to have matched its anchor exactly once and restored byte-identical; .so hash per cell: A control 3e77f7b94928 76 pass B deterministic-collation gate 45b0b7871974 plan: non-deterministic collation key falls back C folding charge's naggs factor 048828ac8797 ten aggregates cost more than one (commandprompt#349) D group-estimate bound bec883463f75 the bound is accurate ... (got 299940 want 12), +1 E pseudoconstant qual rejection 718fa73c1167 regress B2: gating WHERE falls back (not the node) The ICU-gated collation pair is its own test, because cannot_run declares a whole TEST unrunnable while check_skip declines one check. The five local source builds are --without-icu and it declines there; the pg_config the cluster job is given is --with-icu, so CI runs those arms. `cluster_tests` 441 -> 457, derived by collection. `guard_tests` re-derived in the same run and did NOT move: 382. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
|
Your headline claim reproduces exactly, and I ran it rather than reading it. The Stripping the aggregate-count factor —
Note also what the failure says: Restored: md5 back to One number in the PR body does not match the tool
The prose names the two costing premises and omits the two fixture ones. Not a defect in the change — all four are legitimate and the parity verdict is unaffected — but it is a count retyped into prose rather than printed from the tool, which is the thing that bit me twice today in the other direction. I would take the tool's four. A question rather than a findingYou said you would put a tripwire in this port — "a premise that the fixtures this file creates are this run's and not a leftover". I do not see one. The five premises are about loading, costing and the node, not ownership. I think that may be correct rather than forgotten, and I would rather ask than assume: If that is your reasoning, say so in the file — the next person will ask the same question, and "not needed here, and here is the property that makes it unnecessary" is worth more than silence. Everything else checkedHolding the approval only for the extras count and your answer on the tripwire. Neither is a blocker on the code. 🤖 Generated with Claude Code |
jdatcmd
left a comment
There was a problem hiding this comment.
Approving e2ba533d. 14 of 14, CLEAN.
The headline is verified from my side rather than read: stripping the aggregate-count factor reddens the port with got 'no [1=350.5 10=350.5]' want 'yes' while native_groupagg.sh reports 72 passed + 0 failed on the same binary. The .so moved 23c0b7d1d8e7 -> 3445dd801b19 and back, so each side measured its own build. Detail in my comment above.
Not blocking on either of my two points, and here is why. The extras count is PR-body prose, and the correct number is recorded in my review comment, so the history is accurate through that even if the body is not — correct it if you are pushing again anyway. The tripwire is a question whose likely answer is that it would be vacuous here, since pgc_conn drops a private schema per test and this port creates nothing cluster-global. Blocking a verified, green change on a premise that probably cannot fail would be the wrong trade.
If the tripwire reasoning is what I think it is, a sentence in the file is worth more than the premise would have been — the next person will ask the same question, and the answer is a property of the harness rather than of this port.
…pt#432) 56 names, `missing: 0`, no extras. Core ANALYZE samples 30,000 rows and those rows are spread across every row group, so every group is decoded for every column. pgcolumnar.analyze() reads ONE column, and the statistics from a full read are EXACT where core's are estimates. Exactness is the observable a sampled implementation cannot fake, which is what gets asserted. THE DISCRIMINATION DOES NOT DEPEND ON LUCK. `k` is one row in ten NULL, and 0.1 is a number a sampler reaches whenever it is lucky. `k7` is one row in SEVEN, so the truth is 0.142856 and core's estimate is always a whole number of sampled rows over 30,000 -- there is no whole k with k/30000 = 0.142856. Core cannot report that fraction whatever it draws. An earlier version of the shell suite gated on core being WRONG and failed a correct suite about one run in 130 (commandprompt#487). ONE NAMED REFUSAL, THREE UNNAMED. Four tests, split by fixture so only the first pays the 500,000-row build. The shell prints ONE check_skip and exits, so exactly one test names it; naming all four would publish three checks the original does not have. VERIFIED AT RUNTIME. compare_to_bash grades the NAME and proves the string is PRESENT, never that anything emits it. Spying on Expect._record: PG18 emits 55 of 56, and PG17 emits `pgcolumnar.analyze()` with verdict UNRUN. Proved by removal against pgcolumnar--1.0-alpha4.sql; control 55 checks passed: per-column target -> global default histogram honours the column's target (101 want 11), and the positional stride SET STATISTICS 0 skip disabled ran without raising (22012: division by zero) null_frac / non-null count null_frac exactly (0.11111111 want 0.1), and the DELETE arm (0.153846 want 0.133333) The .so hash is NOT the instrument for a SQL mutation and is deliberately not quoted: the C is unchanged, so it hashes identically in all four cells and four identical hashes would look like evidence and be none. The .sql md5 moves per cell, and .sql is in pgc_fingerprint's ROOT_SUFFIXES so the fingerprint moves and build_once reinstalls -- measured 542af67fbc25 unmutated, 1cd8f666717b mutated, restored. cluster_tests 458 -> 462, derived by collection; the branch stated 446 before the rebase, correctly, against a main that commandprompt#1167 has since moved. guard_tests unmoved at 382. The TESTS.md section number is derived from the document's highest heading rather than typed, after two branches claimed 70 in the same afternoon. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
Port of
native_groupagg.sh(#289, #432). 33 names plus one template,missing: 0, twoextras — both premises the shell suite does not have, explained below.
Two oracles, and neither substitutes for the other
A heap mirror holds the same rows, so exact aggregates must agree byte for byte. It
cannot judge float summation order: the two storage types do not scan in the same order and
float addition is not associative.
A toggle differential runs the same query over the SAME columnar rows with the path off
and on. Both arms read in the same order, so even float sums must be identical — which is
what validates the order-preserving accumulators.
Every comparison carries a node premise. A query the node quietly rejects runs the
scalar Agg in BOTH arms, so the comparison is two identical runs agreeing with each other.
That is how
sum(real)returning 0 got through an earlier version of this suite.One arm is stronger than the original, because the original's cannot fail (#1162)
native_groupagg.shasks ten aggregates over ten DIFFERENT columns to cost more than oneover one. A scan projecting ten columns costs more than one projecting one whatever the
folding charge is. Measured with the charge stripped of its aggregate-count factor — the
exact defect #349 fixed:
This file holds the projection fixed and forces the path, so the difference is exactly the
term under test: 450.00 =
cpu_operator_cost× 20,000 rows × 9 extra aggregates.Forcing is not a convenience. With the charge applied the planner prices ten
avg(a)over one column OUT of the grouped path and picks core's Agg; removing the charge makes the
grouped path win it. Without
enable_hashagg=off; enable_sort=offthe comparison silentlychanges which node it reads. The two extra names are the premises that both costed plans
ARE the grouped node.
Independent at every seam
grepEXPLAINtext for the marker lineEXPLAIN (FORMAT JSON), asking whether a node carries the PROPERTY, viaplan_marker, which refuses an empty plan rather than reading it as absencemd5(string_agg(...))computed by the serverq()opens a connectionRemoval proof
Five mutations of
src/columnar_vector.con the Debian PostgreSQL 18 the CI cluster job isgiven. Each asserted its anchor matched exactly once and that the file changed; each
restored and re-hashed byte-identical. The
.sohash is recorded per cell, because amutation that never reached the binary is the failure a red arm cannot show.
.so37d535c9dd6977a1d36c89a0plan: non-deterministic collation key falls backf9dc2a9d5faaten aggregates cost more than one (#349)—no [1=350.5 10=350.5]7d25ab46e1d9the bound is accurate ...(got 299940 want 12) andwhile a matching-type predicate still gets onef0ef8381416dregress B2: gating WHERE falls back (not the node)The gate, and a change to its classification you should know about
The local five-major gate no longer reaches ALL GREEN on 18 and 19. The ICU-gated
collation test declines there, because all five
/usr/local/pg*abuilds are--without-icu. Nothing failed —0 failon every leg — but anyone running the local gatewill now see 67 on six legs instead of three, and should not read that as a regression.
CI is unaffected. The
pg_configthe cluster job is given is--with-icu, so thosearms RUN there and the leg is
rc=0with0 unrun. Measured both ways rather thanreasoned:
I had this wrong first:
pg_config --configure | grep -c icureturns 1 for--without-icu,and I reported ICU as available on all five builds before a
CREATE COLLATIONsaidotherwise.
Why the collation pair is its own test
cannot_rundeclares a whole TEST unrunnable whilecheck_skipdeclines one check. Puttingthe ICU-gated pair in a function with anything else would make the refusal wider than the
shell suite's, and a refusal that takes unrelated arms down with it is a worse report than
the one it replaces.
Bookkeeping
cluster_tests442 → 458, derived by collection. This branch stated 457 before therebase — correctly, against a main at
c050f84. #1161 then moved the key 441 → 442underneath it and the rebase conflicted, because both sides had moved from 441. Neither
number survives and adding the deltas reaches a value no tree collects, which
expected_tests.txtsays in six places and which was true again here.The TESTS.md section number is derived from the document's own highest heading rather
than typed: #1161 took 70 while this was open and both claimed it.
guard_testsre-derivedin the same run and unmoved at 382.
Every arm in this file carries its measurement into the failure message (#1164) — checked
with the guard from #1166, which reports 0 lossy arms here.
🤖 Generated with Claude Code
https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs