fix: count a skipped vector where decode skips it, not where rows are produced (#542) - #543
Conversation
3aa8b50 to
e83c20f
Compare
|
Correcting the gate claim in the description. The fix is unchanged and still green; the suite list I gated on was assembled from memory rather than derived, and it was incomplete. I asked which suites actually reference the counters this change touches, instead of recalling which ones felt relevant: Six: I had gated on three, and one of the three I picked ( All six, pg18a,
Nothing red, so no change to the fix. The reason for saying it anyway: "I ran the relevant suites" is a scope claim, and scope claims should be counted rather than asserted. The query above is what produced this list, and it belongs in the PR rather than in my head. One more disclosure, since it bears on how much the numbers above are worth. My first attempt at this run reported all three suites passing — and it was worthless. The The run above asserts, and exits non-zero on failure of any: the fetch succeeded, the checkout landed, |
ChronicallyJD
left a comment
ChronicallyJD
left a comment
There was a problem hiding this comment.
Reviewed by running it, not by reading it. The fix is right and I would merge
it. One finding, and it is a claim in a comment rather than a defect in code.
Verified
Checked out e83c20f and built clean. native_fold_skipguard, native_vecskip,
native_vecdecode and native_exact_selection all pass.
Removal proof, which is the check that matters: deleting the new counting block
turns two suites red on exact values, not on inequalities.
FAIL and reports the 58 it skipped, on its own plan (#542): got [0] want [58]
FAIL so decoded plus skipped is the group's vectors, on the FOLD arm: got [1] want [59]
FAIL vectors removed > 0 (native_vecskip)
The nativeSkipVec versus maxVecCount - groupVecDecoded reasoning is right, and
right for the reason given: the two-pass loop makes groupVecDecoded the max
across both passes, so on a zero-matching range the subtraction reports 0 skipped
for a group where every vector was ruled out.
The finding: "the same total on the scalar arm as before" is not true
The comment says counting at decode time
reports the same total on the scalar arm as before and a correct one on the
fold arm.
The first half does not hold for a scan that stops early. Same table, same
predicate, scalar arm, one group of 59 vectors:
| query | main 3e86a7e |
this PR |
|---|---|---|
WHERE k BETWEEN 30000 AND 30100 |
58 | 58 |
... LIMIT 1 |
29 | 58 |
WHERE k BETWEEN 200 AND 100000 LIMIT 1 |
0 | 0 |
The old counter incremented as the producer stepped over vectors, so a LIMIT
that stopped it halfway counted half of them. The new one counts what decode
ruled out, which does not depend on how many rows were consumed.
The new number is the better one and I am not asking for a behaviour change.
It is the only one that satisfies the invariant this PR adds: on main the
LIMIT 1 row is 1 + 29 = 30 against a 59-vector group, and here it is
1 + 58 = 59. The old number conflated "ruled out by the mask" with "stepped
over before we stopped", which is the same conflation #542 is about, one layer
down.
Only the sentence needs to change. Something like: the scalar arm's total is
unchanged for a scan that runs to completion, and becomes larger for one that
stops early, because it now reports what decode ruled out rather than how far the
producer got.
Worth fixing rather than waving through, because it is a comment asserting
something that has not been established, and a reader hitting a changed LIMIT
number later will find the comment telling them it cannot have changed.
Not a finding, recorded so it is not re-derived
native_vecskip's SELECT id FROM n WHERE id BETWEEN 100 AND 200 reports the
same skip count on both trees, so the suites that assert exact skip counts on
full scans are unaffected. I checked before assuming.
… produced (commandprompt#542) vectorsSkipped was incremented in pgcolumnar_native_skip_current_vector, which only the row path enters. The batch fold takes a group whole through PgColumnarReadFoldNextGroup and never produces rows one at a time, so it never reached the counter: a fold decoding 1 of 59 vectors reported Columnar Vectors Skipped: 0 That is worse than printing nothing. It answers the question wrongly, and it is the number commandprompt#522 added so a plan could say whether per-vector skipping happened on the fold path -- the path where it matters, since commandprompt#512 is about the fold and the skip vector disagreeing. It is now counted from the final nativeSkipVec, after both decode passes, which both paths share. Counted from the MASK and deliberately not as (maxVecCount - groupVecDecoded). That subtraction looks equivalent and is not, because commandprompt#452 phase 1b-ii made the loop two-pass: pass 0 decodes the qual columns against the mask the ZONE MAPS built, refine_skipvec sharpens it to "no row in this vector matches", and pass 1 decodes the payload columns against the sharpened mask. groupVecDecoded is the max across columns over BOTH passes, so on a zero-matching range it is the qual column's full 32 while the payload columns decoded nothing -- the subtraction reports 0 skipped for a group in which every vector was ruled out. That is not hypothetical: it made native_exact_selection's "zone maps rule out nothing" premise read -32, a count that cannot exist. The mask is what says what was skipped, so ask it. The mask-based count agrees with the row path on every fixture measured -- 58 on native_fold_skipguard's, 32 on native_exact_selection's, 30 on a 1b-ii shape -- and adds the fold arm, where the number was 0. arm before after Decoded scalar 58 58 1 fold 0 58 1 so decoded + skipped == the group's 59 vectors on BOTH arms, where it previously held on the scalar arm alone. It also repairs an invariant commandprompt#541 depends on. vectorsRuledOutByValue is declared "subset of vectorsSkipped" and is incremented during mask refinement, which both arms run; on the fold arm it read 1 beside a vectorsSkipped of 0, a subset larger than its superset. The suite half matters as much. native_fold_skipguard read its premise off the SCALAR arm -- "same table, same predicate, same reader" -- and that proxy is what hid this: the premise for the check under test was measured on a plan that was not the one under test. It now reads the fold plan and asserts exact values, making it a WORK-DONE assertion rather than another correctness one. A fold that silently stopped skipping and decoded all 59 vectors passes every correctness check we own, because decoding everything reads real values rather than holes. Also corrects two statements the suite had outlived: a header describing a world where "the fold does NOT honour the skip vector" (commandprompt#452 phase 1b-i ended that), and a check named "the fold answers correctly while decode skips nothing", which contradicted the premise directly above it even when written. Gated on pg18a and pg19a: native_exact_selection 16/16, native_fold_skipguard 6/6, native_vecdecode 24/24, ungrouped_vector_agg 45/45, 0 warnings, built from make clean with no stale objects (commandprompt#536). Removal proof: putting the counter back on the row path fails exactly the two new checks, got [0] want [58] and got [1] want [59].
e83c20f to
27da1ab
Compare
|
Finding accepted and fixed at
Your reading is right, including that the new number is the better one. The old counter incremented as the producer stepped over vectors, so a Only the sentence changed. The comment now carries the table and says why the totals differ, instead of claiming they cannot. The claim was mine and it was exactly what I have been filing against other people all day: a comment asserting something that had not been established, which a reader hitting a changed Re-gated on all six suites derived from the identifiers this change touches:
pg18a, clean build, zero stale objects. |
1 participant
Closes #542. Rebased onto
3e86a7e(post-#541), and the rebase found a real bug in the first version of this fix — see "What changed" below.vectorsSkippedwas incremented inpgcolumnar_native_skip_current_vector, which only the row path enters. The batch fold takes a group whole throughPgColumnarReadFoldNextGroupand never produces rows one at a time, so it never reached the counter. It is now counted from the finalnativeSkipVec, after both decode passes, which both paths share.chunk_group_row_limit => 1024, 59 vectors,k BETWEEN 30000 AND 30100decoded + skipped == 59now holds on both arms, where it previously held on the scalar arm alone.What changed after the rebase, and why it matters to a reviewer
The first version computed
maxVecCount - groupVecDecoded. That is wrong under #541's two-pass loop and I would not have found it by reading:refine_skipvecsharpens it to "no row in this vector matches";groupVecDecodedis the max across columns over both passes. On a zero-matching range it is the qual column's full 32 while the payload columns decoded nothing, so the subtraction reports 0 skipped for a group in which every vector was ruled out.native_exact_selectioncaught it, and caught it well — its premise is stated as a subtraction, so it printedA negative count is a quantity that cannot exist. A premise phrased as "these two numbers differ" would have reported a mismatch; this one reported an impossibility, which is what made the cause obvious.
Counting the mask agrees with the row path on every fixture measured — 58 on
native_fold_skipguard's, 32 onnative_exact_selection's, 30 on a 1b-ii shape — and adds the fold arm. An earlier draft of this PR claimed the scalar total was unchanged and then that a 30→29 shift was a correction; both came from the broken subtraction. Neither is true of what is here now: the scalar arm is unchanged everywhere.It repairs an invariant #541 depends on
vectorsRuledOutByValueis declaredsubset of vectorsSkipped(columnar_reader.c:153) and incremented during mask refinement, which both arms run. On merged main, fold arm, an even-valued table andk BETWEEN 30001 AND 30001:A subset larger than its superset. This PR makes both arms report the same total, so the declaration holds.
The suite half
native_fold_skipguardread its premise off the scalar arm — "same table, same predicate, same reader" — and that proxy is what hid this defect: the premise for the check under test was measured on a plan that was not the one under test. It now reads the fold plan and asserts exact values (decoded 1, skipped 58, sum 59).That makes it a work-done assertion, and that is the point. A fold that silently stopped skipping and decoded all 59 vectors passes every correctness check in the tree — decoding everything reads real values, never an undecoded hole, so the poison check passes and every parity check passes and the saving disappears in silence.
Two statements the suite had outlived are also corrected: a header describing a world where "the fold does NOT honour the skip vector" (#452 phase 1b-i ended that), and a check named "the fold answers correctly while decode skips nothing", which contradicted the premise directly above it even when written.
Gate
native_exact_selectionnative_fold_skipguardnative_vecdecodeungrouped_vector_aggBuilt from
make cleanwithfind -name '*.o'returning 0 first, per #536.Removal proof. Putting the counter back on the row path, tests untouched:
Exactly the two new checks, with the exact values the defect produces.
Not fixed here, and worth its own issue
decodeSkippedVectors(columnar_reader.c, immediately below this change) isgroupVecDecoded < maxVecCount— the same subtraction, with the same two-pass flaw. On the fixture above it computes32 < 32→ false, claiming decode skipped nothing for a group where pass 1 skipped every vector. It feeds the #512 fallback guard. That guard is no longer load-bearing, since the fold honours the mask directly, so I do not believe it is reachable today — but it is a safety check computing a wrong answer, and correcting it is a separate change from this one.I have not merged this and will not.