Skip to content

Compare zone min/max directly for integer types, and what the write path actually costs (#155) #160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jdatcmd merged 1 commit into from
Jul 26, 2026
Merged

Compare zone min/max directly for integer types, and what the write path actually costs (#155) #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 112 additions & 13 deletions src/columnar_write_state.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,41 @@
#include "utils/snapmgr.h"
#include "utils/typcache.h"

/*
* Direct comparison kinds for the zone min/max (issue #155).
*
* Tracking a chunk's min and max costs two comparisons per value per column, and
* routing each through fmgr is most of what they cost: removing min/max tracking
* entirely saved 15% of a five-int-column load, where the comparison itself is a
* subtraction. These are the types whose stored Datum can be read as a C value
* and compared without a collation, which is the same condition, and the same
* list, the vectorized filter's fast path uses (COLUMNAR_VECFAST_* in
* columnar_vector.c). Anything else keeps the fmgr path.
*
* float4 and float8 are deliberately NOT here even though the filter fast path
* takes them. btree float ordering puts NaN above every other value, which a C
* comparison gets wrong: every comparison against NaN is false, so NaN would read
* as equal and never become a chunk's maximum. A zone map with a maximum that is
* too low makes the reader skip a row group that does hold matching rows, and the
* query silently returns fewer rows. The filter path can take the same shortcut
* because it compares to answer one predicate, not to build stored bounds that a
* later scan trusts.
*/
typedef enum ColumnarFastCmp
{
COLUMNAR_FASTCMP_NONE = 0,
COLUMNAR_FASTCMP_I16,
COLUMNAR_FASTCMP_I32,
COLUMNAR_FASTCMP_I64
} ColumnarFastCmp;

/* per-column, per-write-state facts needed for the min/max skip list */
typedef struct ColumnarColumnDef
{
bool orderable; /* type has a default btree comparison proc */
FmgrInfo cmpFn; /* the comparison proc, when orderable */
Oid collation; /* collation to compare under */
ColumnarFastCmp fastCmp; /* direct comparison, when the type allows */

/*
* int2/int4 column: its exact sum fits an int64 accumulator, so the zone map
Expand Down Expand Up @@ -132,6 +161,46 @@ static void flush_ws_projections(ColumnarWriteState *writeState);
static ChunkGroupBuffer *columnar_start_chunk_group(ColumnarWriteState *writeState);
static void columnar_init_col_defs(ColumnarWriteState *writeState);

/*
* columnar_cmp_value
* Compare two values of a column under its ordering, taking the direct
* route when the type allows one and fmgr otherwise. The integer kinds
* reproduce their btree comparison exactly, so which route is taken can
* never change the answer.
*/
static inline int32
columnar_cmp_value(ColumnarColumnDef *def, Datum a, Datum b)
{
switch (def->fastCmp)
{
case COLUMNAR_FASTCMP_I16:
{
int16 x = DatumGetInt16(a);
int16 y = DatumGetInt16(b);

return (x < y) ? -1 : (x > y) ? 1 : 0;
}
case COLUMNAR_FASTCMP_I32:
{
int32 x = DatumGetInt32(a);
int32 y = DatumGetInt32(b);

return (x < y) ? -1 : (x > y) ? 1 : 0;
}
case COLUMNAR_FASTCMP_I64:
{
int64 x = DatumGetInt64(a);
int64 y = DatumGetInt64(b);

return (x < y) ? -1 : (x > y) ? 1 : 0;
}
case COLUMNAR_FASTCMP_NONE:
break;
}

return DatumGetInt32(FunctionCall2Coll(&def->cmpFn, def->collation, a, b));
}

/*
* columnar_init_col_defs
* Allocate and fill writeState->colDefs: for each column, resolve the btree
Expand Down Expand Up @@ -163,6 +232,31 @@ columnar_init_col_defs(ColumnarWriteState *writeState)
fmgr_info_copy(&writeState->colDefs[c].cmpFn,
&tce->cmp_proc_finfo, ColumnarWriteContext);
writeState->colDefs[c].collation = att->attcollation;

/*
* Resolve a direct comparison where the type permits one. These
* compare byte-for-byte under any collation, so the fast path
* cannot disagree with the operator it replaces; the zone map it
* feeds is read back through the same ordering.
*/
switch (att->atttypid)
{
case INT2OID:
writeState->colDefs[c].fastCmp = COLUMNAR_FASTCMP_I16;
break;
case INT4OID:
case DATEOID:
writeState->colDefs[c].fastCmp = COLUMNAR_FASTCMP_I32;
break;
case INT8OID:
case TIMESTAMPOID:
case TIMESTAMPTZOID:
writeState->colDefs[c].fastCmp = COLUMNAR_FASTCMP_I64;
break;
default:
writeState->colDefs[c].fastCmp = COLUMNAR_FASTCMP_NONE;
break;
}
}

/* int2/int4: exact sum fits int64, carried in the zone map (D5) */
Expand Down Expand Up @@ -414,25 +508,30 @@ ColumnarWriteRow(ColumnarWriteState *writeState, Relation rel,
}
else
{
int32 cmpMin = DatumGetInt32(
FunctionCall2Coll(&def->cmpFn, def->collation,
values[c], col->minValue));
int32 cmpMax = DatumGetInt32(
FunctionCall2Coll(&def->cmpFn, def->collation,
values[c], col->maxValue));

if (cmpMin < 0)
/*
* A value above the running maximum cannot also be below the
* running minimum, so the second comparison is only needed
* when the first does not settle it. Testing the maximum
* first makes the ascending case -- which is what a bulk load
* of a serial or timestamp column produces -- cost one
* comparison per value instead of two.
*/
int32 cmpMax = columnar_cmp_value(def, values[c],
col->maxValue);

if (cmpMax > 0)
{
if (!att->attbyval)
pfree(DatumGetPointer(col->minValue));
col->minValue = datumCopy(values[c], att->attbyval,
pfree(DatumGetPointer(col->maxValue));
col->maxValue = datumCopy(values[c], att->attbyval,
att->attlen);
}
if (cmpMax > 0)
else if (columnar_cmp_value(def, values[c],
col->minValue) < 0)
{
if (!att->attbyval)
pfree(DatumGetPointer(col->maxValue));
col->maxValue = datumCopy(values[c], att->attbyval,
pfree(DatumGetPointer(col->minValue));
col->minValue = datumCopy(values[c], att->attbyval,
att->attlen);
}
}
Expand Down
177 changes: 177 additions & 0 deletions test/write_minmax_fastpath.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
#!/usr/bin/env bash
#
# pgColumnar zone min/max via the direct comparison path (issue #155).
#
# Tracking a chunk's min and max costs two comparisons per value per column, and
# routing both through fmgr is most of what they cost. Integer-family columns now
# compare directly, and the maximum is tested first so an ascending load -- what a
# bulk load of a serial or timestamp column produces -- needs one comparison per
# value rather than two.
#
# The risk this carries is not a crash. A zone map drives row-group skipping, so a
# minimum that is too high or a maximum that is too low makes the reader skip a
# group that does hold matching rows and the query silently returns fewer. That is
# the failure this file is built to catch, so the checks are differential against
# a heap mirror over predicates that sit exactly on the chunk bounds.
#
# The float case has its own check and is the reason floats are deliberately NOT
# on the fast path. btree float ordering puts NaN above every other value; a C
# comparison gets that wrong, because every comparison against NaN is false, so
# NaN would read as equal and never become a chunk's maximum. A later change that
# adds float4/float8 to the fast list fails here rather than silently losing rows.
#
# Usage: test/write_minmax_fastpath.sh [PG_CONFIG]
# Written fresh for pgColumnar.

set -uo pipefail
. "$(dirname "${BASH_SOURCE[0]}")/lib.sh"

pgc_setup "${1:-/usr/local/pg17/bin/pg_config}"

ROWS=${PGC_MINMAX_ROWS:-200000}

# small chunk groups so there are many zone maps to get wrong, and so a predicate
# can fall inside one group and outside its neighbours
psql_run "DROP TABLE IF EXISTS mm_c; DROP TABLE IF EXISTS mm_h;
SET pgcolumnar.stripe_row_limit = 20000;
SET pgcolumnar.chunk_group_row_limit = 5000;
CREATE TABLE mm_c (
i2 smallint, i4 int, i8 bigint,
d date, ts timestamp, tz timestamptz,
asc_i int, desc_i int, rand_i int, neg_i int,
nullable_i int, t text
) USING pgcolumnar;
INSERT INTO mm_c SELECT
(g % 30000)::smallint,
g,
g::bigint * 1000000,
'2000-01-01'::date + (g % 10000),
'2000-01-01'::timestamp + (g || ' sec')::interval,
'2000-01-01'::timestamptz + (g || ' sec')::interval,
g,
$ROWS - g,
(g * 7919) % 100000,
-g,
CASE WHEN g % 5 = 0 THEN NULL ELSE g END,
't' || g
FROM generate_series(1, $ROWS) g;
CREATE TABLE mm_h (LIKE mm_c);
INSERT INTO mm_h SELECT * FROM mm_c;" >/dev/null 2>&1

echo "-- $ROWS rows, $(q "SELECT count(*) FROM pgcolumnar.row_group r
JOIN pgcolumnar.storage s ON s.storage_id = r.storage_id
WHERE s.relation_oid = 'mm_c'::regclass;") row groups"

# --- 1. every column agrees with heap under range predicates -------------------

# Predicates chosen to land on and around chunk-group boundaries, which is where a
# botched bound shows up: a group is skipped or kept wrongly only at its edges.
diff_count() { # label, predicate
local c h
c="$(q "SELECT count(*) FROM mm_c WHERE $2;")"
h="$(q "SELECT count(*) FROM mm_h WHERE $2;")"
[ "$c" = "$h" ] || echo "$1(columnar=$c heap=$h)"
}

bad=""
bad="$bad $(diff_count i4_eq "i4 = 5000")"
bad="$bad $(diff_count i4_bound "i4 BETWEEN 4999 AND 5001")"
bad="$bad $(diff_count i4_range "i4 > $((ROWS / 2)) AND i4 < $((ROWS / 2 + 17))")"
bad="$bad $(diff_count i2_eq "i2 = 12345")"
bad="$bad $(diff_count i2_range "i2 BETWEEN 100 AND 200")"
bad="$bad $(diff_count i8_range "i8 > 4999000000 AND i8 < 5001000000")"
bad="$bad $(diff_count date_range "d BETWEEN '2000-06-01' AND '2000-06-03'")"
bad="$bad $(diff_count ts_range "ts > '2000-01-01 01:00:00' AND ts < '2000-01-01 01:00:10'")"
bad="$bad $(diff_count tz_range "tz > '2000-01-01 01:00:00+00' AND tz < '2000-01-01 01:00:10+00'")"
bad="$bad $(diff_count asc_range "asc_i BETWEEN 19999 AND 20001")"
bad="$bad $(diff_count desc_range "desc_i BETWEEN 19999 AND 20001")"
bad="$bad $(diff_count rand_range "rand_i BETWEEN 500 AND 600")"
bad="$bad $(diff_count neg_range "neg_i BETWEEN -20001 AND -19999")"
bad="$bad $(diff_count null_range "nullable_i BETWEEN 4999 AND 5001")"
bad="$bad $(diff_count null_isnull "nullable_i IS NULL")"
bad="$bad $(diff_count text_eq "t = 't5000'")"

check "every column returns the same rows as heap under range predicates" \
"$(echo $bad | tr -s ' ')" ""

# --- 2. the stored bounds are the true bounds ----------------------------------

# min/max read straight back out, which is what the zone maps hold. An aggregate
# over a column with no deletes is answered from those zone maps, so this reads
# the stored bounds rather than the data.
bad=""
for col in i2 i4 i8 d ts tz asc_i desc_i rand_i neg_i nullable_i t; do
c="$(q "SELECT min($col)::text || '|' || max($col)::text FROM mm_c;")"
h="$(q "SELECT min($col)::text || '|' || max($col)::text FROM mm_h;")"
[ "$c" = "$h" ] || bad="$bad $col(columnar=$c heap=$h)"
done
check "min and max match heap on every column" "${bad:-same}" "same"

# --- 3. skipping still happens -------------------------------------------------

# A fast path that quietly widened every bound would pass checks 1 and 2 while
# giving up the skipping the zone maps exist for, so assert groups are still
# ruled out.
skipped="$(q "EXPLAIN (ANALYZE, COSTS off, TIMING off, SUMMARY off)
SELECT count(*) FROM mm_c WHERE asc_i BETWEEN 19999 AND 20001;" \
| grep -oE 'Columnar Chunk Groups Removed by Filter: [0-9]+' \
| grep -oE '[0-9]+$' | head -1)"
echo "-- chunk groups skipped for a narrow range: ${skipped:-none reported}"

check "a narrow range still skips row groups" \
"$( [ -n "${skipped:-}" ] && [ "$skipped" -gt 0 ] && echo yes || echo "no (${skipped:-unreported})")" \
"yes"

# --- 4. floats keep their btree ordering, NaN included -------------------------

psql_run "DROP TABLE IF EXISTS mm_f; DROP TABLE IF EXISTS mm_fh;
SET pgcolumnar.stripe_row_limit = 20000;
SET pgcolumnar.chunk_group_row_limit = 20000;
CREATE TABLE mm_f (f4 real, f8 double precision) USING pgcolumnar;
INSERT INTO mm_f SELECT
CASE WHEN g = 5001 THEN 'NaN'::real
WHEN g = 5002 THEN 'Infinity'::real
WHEN g = 5003 THEN '-Infinity'::real
ELSE g::real END,
CASE WHEN g = 5001 THEN 'NaN'::float8
WHEN g = 5002 THEN 'Infinity'::float8
WHEN g = 5003 THEN '-Infinity'::float8
ELSE g::float8 END
FROM generate_series(1, 5003) g;
CREATE TABLE mm_fh (LIKE mm_f);
INSERT INTO mm_fh SELECT * FROM mm_f;" >/dev/null 2>&1

# The special values sit at the END of a single chunk group, on purpose. NaN
# arriving first would become that chunk's min and max before any comparison ran,
# and every check below would pass whatever the comparison did -- which is what an
# earlier version of this fixture did, and it let a deliberately broken build
# through.
check "the float fixture is one chunk group" \
"$(q "SELECT count(*) FROM pgcolumnar.row_group r
JOIN pgcolumnar.storage s ON s.storage_id = r.storage_id
WHERE s.relation_oid = 'mm_f'::regclass;")" "1"

# btree orders NaN above everything, so it is the maximum of both columns. A C
# comparison would never make it one, and then a query for it can be skipped.
check "max of a float column with NaN matches heap" \
"$(q "SELECT max(f8)::text FROM mm_f;")" "$(q "SELECT max(f8)::text FROM mm_fh;")"

check "max of a real column with NaN matches heap" \
"$(q "SELECT max(f4)::text FROM mm_f;")" "$(q "SELECT max(f4)::text FROM mm_fh;")"

check "a row holding NaN is still found by an equality predicate" \
"$(q "SELECT count(*) FROM mm_f WHERE f8 = 'NaN';")" \
"$(q "SELECT count(*) FROM mm_fh WHERE f8 = 'NaN';")"

check "infinities are found too" \
"$(q "SELECT count(*) FROM mm_f WHERE f8 = 'Infinity' OR f8 = '-Infinity';")" \
"$(q "SELECT count(*) FROM mm_fh WHERE f8 = 'Infinity' OR f8 = '-Infinity';")"

# --- 5. the fast path is restricted to the types that can take it --------------

SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src"

check "float types are not on the direct comparison list" \
"$(grep -cE 'COLUMNAR_FASTCMP_F(32|64)' "$SRC/columnar_write_state.c")" "0"

pgc_summary