The analyze() comment still claims null_frac comes from the zone maps (#485)

[ChronicallyJD]

Found while reviewing #497, which documents exactly this change. This is my own omission from #488.

#488 moved null_frac off the zone maps and onto the same read as n_distinct, because the zone-map counts describe what was written and a DELETE left the fraction normalised against rows the table no longer held. It did not update the COMMENT ON FUNCTION, which on current main still tells the user the opposite:

taking null_frac exactly from the zone maps rather than sampling (#414)

User-visible through \df+ pgcolumnar.analyze and through the extension script. It is the kind of stale claim that gets believed precisely because it sits next to correct code.

The replacement states what is true now, and adds the property that matters more than the source:

null_frac, n_distinct and the most-common frequencies all come from that read, so they describe one population (#485)

That identity is what #485 was actually about — the wrong value mattered less than two statistics in one pg_stats row being normalised against different populations.

One line, no behaviour change, no test change.

[jdatcmd]

The change is right and the reasoning for it is right. One line short, in the same file.

Verified

The stale claim is on current main at pgcolumnar--1.0-alpha.sql:1428, it is user-visible through \df+, and the replacement is accurate: after #488 all three statistics do come from the one read. Adding the shared-population property rather than just correcting the source is the better fix, since that identity is what #485 was actually about.

The same claim survives 443 lines up

pgcolumnar--1.0-alpha.sql:985, the function's own header comment:

 * Collected so far, all of it exact rather than sampled: null_frac from the zone
 * maps (metadata only, no data read), n_distinct from reading one column, and
 * from that same read the most-common values with their frequencies and a
 * histogram of what remains once those are excluded.

null_frac from the zone maps (metadata only, no data read) is exactly the sentence this PR exists to remove, and the parenthetical makes it a stronger claim than the one at 1428: it does not merely say where the number comes from, it promises no data read. After #488 that read always happens.

It is a two-line wrap, which is why a single-line grep for the phrase does not find it. That is presumably how it survived the sweep.

Worth fixing here rather than in a third PR, since anyone who reads this one will reasonably assume the file no longer makes the claim.

Checked and clean

I swept the rest of the tree for the same shape. The only other occurrence is pgcolumnar--1.0-alpha.sql:1347, which is #488's own new comment explaining why nv is not derived as totalrows minus a zone-map null_frac. That one is correct and should stay.

Requesting changes only for the 985 hunk. With that in, this is ready.

[jdatcmd]

Re-reviewed at 935e5879. Unchanged since my review: still +1/-1, and pgcolumnar--1.0-alpha.sql:985 still reads null_frac from the zone maps (metadata only, no data read).

Second pass on the replacement text itself, since a one-line PR deserves the line being checked rather than only the line it removes: it is accurate. After #488, null_frac, n_distinct and the most-common frequencies do all come from the one read, and those three are exactly the terms in the null_frac + sum(mcv_freqs) + rest = 1 identity the header comment cites at 1125, so naming those three rather than listing the histogram as well is right for the point being made.

Only the 985 hunk is outstanding.

[ChronicallyJD]

Fixed and pushed as 54f6b03.

You are right that it is the stronger claim: (metadata only, no data read) promises something the code no longer does, where the line I fixed only named a source.

On how it survived my sweep — it wraps across two lines, so a single-line grep for the phrase cannot match it, exactly as you say. I re-swept the whole tree with a multi-line pattern rather than a longer grep. Four occurrences remain and all four are correct: my new past-tense sentence here, :1352 (your call, and I agree it stays — it is #488's note that nv is deliberately not derived that way), and two past-tense descriptions of the defect in test/analyze_function.sh.

The replacement leads with the property rather than the source, since "one read" is what makes the statistics describe one population, and that is the thing a future edit should be reluctant to break:

Collected so far, all of it exact rather than sampled, and all of it from ONE read of the column [...] One read is the property that matters, not merely the source of each number.

[jdatcmd]

54f6b039 fixes it, and does more than I asked for in the right direction.

I asked for the claim at :985 to be removed. What landed replaces it with the property rather than a corrected source:

all of it exact rather than sampled, and all of it from ONE read of the column

and then says why that is the point, including that null_frac came from the zone maps until #485 and was wrong after a DELETE because those counts describe what was written. A reader who arrives at that comment now learns the thing the old text was hiding.

Checked the new text rather than only the removal, since a one-line PR earns that: after #488 all four statistics do come from the single read, and "exact rather than sampled" still holds for each of them, the histogram included (its bounds are actual column values, and #488 moved their positions without making them estimates).

Approving.