fix: count a skipped vector where decode skips it, not where rows are produced (#542)

src/columnar_reader.c

@@ -1984,6 +1984,61 @@ pgcolumnar_native_load_group(PgColumnarReadState *rs)
 	}
 	rs->vectorsDecoded += (uint64) groupVecDecoded;
 
+	/*
+	 * Counted HERE, where the skipping is performed, and not where rows are
+	 * produced (#542).
+	 *
+	 * It used to be 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 that counter: a fold that decoded 1 of 59 vectors reported
+	 * "Columnar Vectors Skipped: 0". That is worse than printing nothing, because
+	 * it answers the question wrongly, and it is the number #522 added precisely
+	 * so a plan could say whether per-vector skipping happened on the fold path.
+	 *
+	 * Decode is shared by both paths, and it is the only place the final mask
+	 * exists.
+	 *
+	 * What this does to the scalar arm's total, stated exactly, because an
+	 * earlier version of this comment claimed it was unchanged and that is only
+	 * true of a scan that runs to completion:
+	 *
+	 *     query                                    before   after
+	 *     WHERE k BETWEEN 30000 AND 30100              58      58
+	 *     ... LIMIT 1                                  29      58
+	 *
+	 * The old counter incremented as the PRODUCER stepped over vectors, so a
+	 * LIMIT that stopped it halfway counted half of them. This one counts what
+	 * decode ruled out, which does not depend on how many rows were consumed. The
+	 * new number is the one that satisfies the invariant this change exists for:
+	 * on a 59-vector group the LIMIT row was 1 + 29 = 30, and is now 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.
+	 *
+	 * Counted from nativeSkipVec and NOT as (maxVecCount - groupVecDecoded).
+	 * That subtraction looks equivalent and is not, because #452 phase 1b-ii made
+	 * the loop two-pass: pass 0 decodes the qual columns against the mask the
+	 * ZONE MAPS built, refine_skipvec then sharpens it to "no row in this vector
+	 * matches", and pass 1 decodes the payload columns against the sharpened one.
+	 * 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 yields 0 skipped for a group
+	 * where every vector was ruled out. Measured: it made
+	 * native_exact_selection's "zone maps rule out nothing" premise read -32.
+	 *
+	 * The mask is the thing that says what was skipped, so ask it.
+	 */
+	if (rs->nativeSkipVec != NULL && rs->nativeVectorCount > 0)
+	{
+		int			v;
+		int			skipped = 0;
+
+		for (v = 0; v < rs->nativeVectorCount; v++)
+			if (rs->nativeSkipVec[v])
+				skipped++;
+		rs->vectorsSkipped += (uint64) skipped;
+	}
+
 	/*
 	 * The #512 tripwire's input, and it is measured rather than inferred from
 	 * the mask: a mask with nothing set in it skips nothing, and a decode that
@@ -2064,7 +2119,12 @@ pgcolumnar_native_skip_current_vector(PgColumnarReadState *rs)
 
 	rs->rowInGroup = rs->nativeVecStart[v + 1];
 	rs->nativeCurVec = v + 1;
-	rs->vectorsSkipped++;
+	/*
+	 * vectorsSkipped is NOT incremented here any more (#542). This function is
+	 * on the row path only, so counting here made the number a row-path counter
+	 * wearing a plan-wide name: correct on a sequential scan, silently 0 on the
+	 * batch fold. It is now counted in the group decode, which both paths share.
+	 */
 	return true;
 }
 

test/native_fold_skipguard.sh

@@ -3,21 +3,30 @@
 # pgColumnar: the batch fold must refuse a group whose decode skipped vectors
 # (#512).
 #
-# The fold at columnar_vector.c does NOT honour the skip vector. It reads every
-# vector and reaches the right answer by re-checking every value against the scan
-# keys, which is correct only while decode produces every vector -- and today it
-# does, because pgcolumnar_native_decode_chunk takes no skip mask at all.
+# That day has arrived. #452 phase 1b-i taught decode to skip the vectors the
+# zone maps rule out, and taught the fold to honour the same mask, so the text
+# that used to stand here -- "the fold does NOT honour the skip vector ... decode
+# produces every vector" -- is no longer true of any line in the tree.
 #
-# The day decode is taught to skip ruled-out vectors (#452 phase 1b) the decoded
-# buffer gains holes, and this loop would re-check UNINITIALISED memory: a wrong
-# aggregate, silently, and only on data whose zone maps rule something out. The
-# row producer is safe there; the fold is not. So the ordering constraint is
-# enforced by a guard rather than written in a comment.
+# What the #512 guard was for: decode gaining holes before the fold learned about
+# them would have made the fold re-check UNINITIALISED memory -- a wrong
+# aggregate, silently, and only on data whose zone maps rule something out. It
+# was an ORDERING guard, and it fired on exactly the change it was written for.
+# What survives it is a narrow fallback: a skip reported without the per-vector
+# map to honour it must ERROR rather than guess.
 #
-# This suite proves the guard is REACHABLE, which is the part that is easy to get
-# wrong: the fold only runs for a shape that qualifies for it, and the hazard only
-# exists where vectors are skipped. Both have to be true at once, and a check that
-# never reaches the fold would pass forever while guarding nothing.
+# So be exact about what this suite does and does not prove, because the two are
+# easy to confuse now that the guard is no longer load-bearing:
+#
+#   this suite          the fold is REACHABLE for a shape that skips vectors, and
+#                       that it accounts for the skipping it performs
+#   native_vecdecode    the fold is CORRECT there -- its poison check picks a
+#                       predicate that ACCEPTS 0xA5, so a fold reading an
+#                       undecoded hole counts rows that are not there
+#
+# Neutering the surviving fallback does NOT redden this suite, and that is now
+# the expected result rather than a hole: the path it guards is unreachable while
+# decode and the fold honour the same mask. Measured, not assumed.
 #
 # Usage:  test/native_fold_skipguard.sh [PG_CONFIG]
 # Written fresh for pgColumnar.
@@ -46,15 +55,32 @@ run() {  # run <setup> <sql>
 		-d "$PGC_DB" -At -c "$1" -c "$2" 2>&1
 }
 
-# ---- premise one: the reader really does skip vectors for this predicate ----
+# ---- premise one: the FOLD ARM does the skipping, and says so ---------------
+#
+# Read off the fold arm itself, which is the plan under test. It used to be read
+# off the scalar arm with the note "same table, same predicate, same reader, so
+# what it skips there it skips here" -- a proxy, and one that hid #542: the
+# counter was incremented only where rows are produced, so the fold reported
+# "Vectors Skipped: 0" while decoding 1 of 59 vectors. Measuring the premise on a
+# plan that is not the one under test is what let that sit unnoticed.
+#
+# Exact values, not "greater than zero". 60,000 rows at chunk_group_row_limit
+# 1024 is 59 vector positions in one group, and k BETWEEN 30000 AND 30100 lies
+# inside one of them, so decode must touch exactly 1 and skip exactly 58.
 #
-# Read off the SCALAR arm, because the vectorized aggregate node does not print
-# "Columnar Vectors Skipped" at all -- a reporting gap of its own. Same table,
-# same predicate, same reader, so what it skips there it skips here.
-vskip=$(run "$SET_OFF" "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) $Q" \
-	| grep "Columnar Vectors Skipped" | grep -oE '[0-9]+' | head -1)
-check "premise: the predicate rules vectors out at all" \
-	"$([ "${vskip:-0}" -gt 0 ] && echo yes || echo no)" "yes"
+# This is a WORK-DONE assertion, and it is the only kind that can catch a fold
+# that silently stops skipping. Every other check here and in native_vecdecode is
+# a correctness check, and a fold that decoded all 59 vectors would pass all of
+# them: decoding everything reads real values, never a hole, so the answers stay
+# right and the saving disappears in silence.
+GROUP_VECS=59
+fplan=$(run "$SET_ON" "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) $Q")
+vskip=$(grep "Columnar Vectors Skipped" <<<"$fplan" | grep -oE '[0-9]+' | head -1)
+vdec=$(grep "Columnar Vectors Decoded" <<<"$fplan" | grep -oE '[0-9]+' | head -1)
+check "premise: the fold arm decodes only the vector it needs" "${vdec:-none}" "1"
+check "and reports the 58 it skipped, on its own plan (#542)" "${vskip:-none}" "58"
+check "so decoded plus skipped is the group's vectors, on the FOLD arm" \
+	"$(( ${vdec:-0} + ${vskip:-0} ))" "$GROUP_VECS"
 
 # ---- premise two: the query actually reaches the fold -----------------------
 #
@@ -65,14 +91,18 @@ fold=$(run "$SET_ON" "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) $Q"
 	| grep -oE "Columnar Batch Fold: [a-z]+" | head -1)
 check "premise: the query reaches the batch fold" "$fold" "Columnar Batch Fold: yes"
 
-# ---- the guard is silent while decode produces every vector ----------------
+# ---- and it answers correctly while decode DOES skip -----------------------
 #
-# Today decode never skips, so the fold must run normally and answer correctly.
-# The heap arithmetic is the oracle: 101 rows, sum of 30000..30100.
+# The name matters. This check was called "the fold answers correctly while
+# decode skips nothing", which contradicted the premise directly above it even
+# when it was written, and became plainly false when #452 phase 1b-i landed.
+# Decode skips 58 of 59 vectors on this query; the fold answers over the one that
+# survives. The heap arithmetic is the oracle: 101 rows, sum of 30000..30100.
 want_n=101
 want_s=$(( (30000 + 30100) * 101 / 2 ))
 got=$(run "$SET_ON" "$Q" | tail -1)
-check "the fold answers correctly while decode skips nothing" "$got" "$want_n|$want_s"
+check "the fold answers correctly while decode skips 58 of 59 vectors" \
+	"$got" "$want_n|$want_s"
 
 # And the same answer without the fold, so the fold is not quietly wrong in a way
 # that matches a wrong expectation.