refactor: one place prints the chunk-group statistics (#495) - #515
Conversation
) Five lines were printed by three nodes -- the scalar custom scan and both vectorized aggregates -- each from its own copy of the code. commandprompt#484 had to add "Usable Skip Predicates" in three places for that reason, and said why it could not do fewer: fixing one would leave a line of plan text meaning two different things depending on which node ran. That invariant was held by discipline. Now by structure. PgColumnarExplainPushedDown and PgColumnarExplainGroupStats are the only emitters, and the long explanations of WHY both numbers exist -- commandprompt#191 for the filter count describing the plan rather than the run, commandprompt#479 and commandprompt#477 for the usable count -- live once beside the code they describe instead of three times in three files. Two functions rather than one, because the nodes interleave: the ungrouped aggregate prints "Columnar Batch Fold" between the filter count and the group counters, so a single combined emitter would reorder its plan output. Splitting there keeps every plan byte-identical, which the 34 pre-existing checks in pushdown_report.sh confirm -- they read specific fields from plans of all three node types and all still pass. Behaviour is deliberately unchanged, including the asymmetry commandprompt#495 notes: the filter count still prints outside the "did we read anything" guard and the usable count inside it. That gap is real and reachable on the scalar node, but it is a semantics question and belongs to commandprompt#493; unifying the emission first is what makes commandprompt#493 a one-line change instead of a fourth edit to three places. ## The check that makes it durable Unifying three call sites into one is worth little if a FOURTH is undetectable -- that is how the three arose. pushdown_report.sh now asserts each of the five lines has exactly one emitter in src/, counted at the source, which is the only place a new caller is visible before it has drifted. Proved by growing a fourth the way a new node would, rather than by deleting the check: baseline 39 checks, 0 fails fourth emitter added 39 checks, 1 fail -> exactly one emitter for "Columnar Chunk Groups Total": got [2] want [1] A source grep rather than a behavioural assertion on purpose: the failure being guarded is "someone wrote a second emitter", which is a property of the code and not of any plan. The behavioural half -- that the nodes agree with each other -- is what the rest of the suite already checks. Refs commandprompt#495, commandprompt#484, commandprompt#479, commandprompt#493
-Wdeclaration-after-statement, caught by the matrix as FAIL PG18/PG19 (warnings) rather than by any suite -- every one of the five majors reported pushdown_report 39 checks / 0 fail while the build was dirty. Worth noting which gate caught it: the suites cannot see a warning, and I would have opened the PR on five green suite runs. Zero warnings now. Refs commandprompt#495
jdatcmd
left a comment
There was a problem hiding this comment.
Approved. Verified independently on 17 and 18, built clean per major with rm -f src/*.o between:
pushdown_report 39 · ungrouped_vector_agg 45 · parallel_vector_agg 29
native_agg 25 · zonemap_cost 19 0 fails, both majors
My first attempt at that verification was a false green and is worth recording, because it is this session's own failure shape and I nearly posted it. The clone silently failed, cd failed with it, and every suite reported fails=0 — because no test file existed to produce a FAIL line. What gave it away was the checks run column being empty: zero fails and zero checks is check "" "" wearing a report's clothes. I now assert the tree is present before running anything and print the count beside every result, because a fail count without a check count is not a result.
Which is an argument for your #513 in a different register: a run that measures nothing looks exactly like a run that measures everything and passes, unless the harness says out loud what it measured.
On the change itself
Two functions rather than one, because the aggregate node prints "Columnar Batch Fold" between the filter count and the group counters — a real constraint, discovered rather than assumed, and documented where the next person will hit it. Combining them to make the diff prettier would have silently reordered plan output.
The #191/#479/#477 rationales now living once beside the code is the substantive win. Three copies of a comment explaining why a line means what it means is worse than three copies of the code, because the copies drift in meaning rather than in behaviour and nothing detects that.
The durability check does what I asked and is the right instrument: a grep over source, because the failure being guarded is "someone wrote a second emitter", which is a property of the code and not of any run. A behavioural check cannot see a fourth emitter that agrees with the other three today. Proved by growing one rather than by deleting the check, which is the distinction that made half of today's proofs worthless before they were redone.
The finding in your body is the most valuable thing here
Your first version compiled with a -Wdeclaration-after-statement warning, and all five majors reported pushdown_report 39 checks / 0 fail while the build was dirty. A suite cannot see a compiler warning; only the matrix looks. That is a concrete argument for the matrix that neither of us had this morning, and it belongs in the PR body exactly where you put it.
Merging.
Fixes #495.
Five plan lines were printed by three nodes — the scalar custom scan and both
vectorized aggregates — each from its own copy of the code:
Now one each.
PgColumnarExplainPushedDownandPgColumnarExplainGroupStatsarethe only emitters, and the explanations of why both numbers exist — #191 for the
filter count describing the plan rather than the run, #479 and #477 for the usable
count — live once beside the code instead of three times in two files.
Two functions rather than one, because the nodes interleave: the ungrouped
aggregate prints
Columnar Batch Foldbetween the filter count and the groupcounters, so a single combined emitter would reorder its plan output. Splitting
there keeps every plan byte-identical.
Behaviour is unchanged, including the part #495 complains about
The filter count still prints outside the "did we read anything" guard and the
usable count inside it. That asymmetry is real and reachable on the scalar node,
and it is a semantics question that belongs to #493. Unifying the emission
first is what makes #493 a one-line change instead of a fourth edit to three
places — which was the ordering @jdatcmd and I agreed before I started.
The 34 pre-existing checks in
pushdown_report.share the evidence that nothingmoved: they read specific fields from plans of all three node types, and all pass
unchanged.
The check that makes it durable
Unifying three call sites is worth little if a fourth is undetectable — that is
how the three arose, and #484 had to add a line in three places for exactly that
reason.
pushdown_report.shnow asserts each of the five lines has exactly oneemitter in
src/, counted at the source, which is the only place a new caller isvisible before it has drifted.
Proved by growing a fourth the way a new node would, rather than by deleting the
check:
A source grep rather than a behavioural assertion on purpose: the failure guarded
against is "someone wrote a second emitter", which is a property of the code and
not of any plan. That the nodes agree with each other is what the rest of the
suite already checks.
What the gate caught that the suites could not
The first version compiled with one warning —
-Wdeclaration-after-statement, from declaring the stats struct after statements— and all five majors reported
pushdown_report39 checks / 0 fail while thebuild was dirty. A suite cannot see a compiler warning; only the matrix looks.
I would have opened this on five green suite runs. Fixed in the second commit;
zero warnings now.
Gate
pushdown_reportFull matrix: PG18 PASS (132 ran, 2 skipped), zero warnings. PG19 reports
temporal=FAIL, which is the audit container's missingbtree_gistand whichunmodified
mainfails identically there — measured, and written up on #505.Happy to re-run on the bench host if you would rather see it without that caveat.