feat: maintain inlined-data column stats in the direct-insert writer - #238
Draft
GavinPizza wants to merge 1 commit into
Draft
feat: maintain inlined-data column stats in the direct-insert writer#238GavinPizza wants to merge 1 commit into
GavinPizza wants to merge 1 commit into
Conversation
GavinPizza
marked this pull request as draft
July 29, 2026 15:19
The direct-insert path (CreateSnapshotForDirectInsert) did not maintain ducklake_table_column_stats min/max or contains_null for inlined rows, so stale zone-map bounds could mis-prune scans, a stale contains_null could drop rows outright, and the persisted stats are wrong for any DuckLake reader of the same catalog that has no reader-side guard. This was masked in our own reads by patch 006; the guard is now retired and the bug fixed in the writer where it originates. relytcloud#217 fixed the same function on v1.0 by degrading the stats to NULL and named this precise version as a refactor targeting main. - Widen (never shrink, never seed) per-column min/max from each inserted batch via a vectorized PG-Datum accumulator (inline_col_stats), wired into the UNNEST/VALUES and COPY inline writers. Never-seed is per bound, not per row: neither a stats row for a column that has none (e.g. an ALTER-added column whose back-fill has not flushed), nor a missing min or max on a row that already carries the other one. A bound derived from one batch is not a bound over the table. - Maintain contains_null the same way, through the same MergeStats call (as null_count), in all three inline writers. Left stale at false it is worse than a bad bound: the optimizer folds IS NULL to false and the row vanishes, while IS NOT NULL counts it as non-NULL. Counting is not gated on the min/max eligibility test -- null-ness needs no comparison proc or canonical encoding, so unlike a bound it costs nothing to match the normal path for every column type. - Decode the persisted row through DuckLakeColumnStats::FromGlobalStats and apply the widen through MergeStats, rather than reimplementing either locally, so the comparison semantics and the any_valid rule cannot drift from the engine's. - Fold the per-column widen into a single SELECT plus one UPDATE ... FROM (VALUES ...) per snapshot, so insert cost no longer scales with column count, and skip the UPDATE entirely when no bound moved -- the steady state, where a batch falls inside the persisted bounds, now costs one round trip and writes no dead tuples. - Advance next_file_id on the inline commit, forward-porting relytcloud#217's fix to main. It rotates the stats-cache key so a scan taken between two inline inserts cannot serve stale bounds, and it retires patch 006 with no replacement vendor patch. It also fixes data loss that main has today: the stale cached stats carry next_row_id, so a normal-path insert following direct inserts in one backend reuses row ids, and deleting one of those rows destroys an unrelated one. - Create the accumulator's memory context under CurTransactionContext rather than TopMemoryContext, so a PG elog(ERROR) that longjmps past the destructor does not leak it once per failed insert. - Log a DEBUG1 when a persisted bound cannot be cast to the column's current type -- reachable after a plain ALTER COLUMN TYPE -- rather than skipping that column's widen silently. The accumulator folds native PG Datums instead of building a duckdb::Value per cell. That divergence from the reuse-the-engine rule is deliberate and measured: a Value-per-cell implementation costs ~6x more (+161ms vs +25ms per 100k-row COPY, 7 columns) for byte-identical bounds. Net write-path cost is +7% at batch=1000 and +20% at batch=100, flat in column count. Retires third_party/ducklake-006-inlined-scan-stats-guard.patch, reworks inlined_scan_stats_guard to assert maintained bounds, and forward-ports relytcloud#217's direct_insert_metadata test, which main never received. That test's third section asserted that the fast path degrades stats to NULL because it cannot maintain them; it now asserts the widened bounds and contains_null. Three further sections pin one column per inline writer, the min/max-ineligible types, and the inline-only table where contains_null correctly stays unknown.
GavinPizza
force-pushed
the
feat/maintain-inline-column-stats
branch
from
July 29, 2026 15:55
c5c84bd to
7f3be7e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The direct-insert writer (
CreateSnapshotForDirectInsert) does not maintainducklake_table_column_statsmin/max orcontains_nullfor the rows it inlines. Reads stay correct only because the vendored guardducklake-006-inlined-scan-stats-guard.patchsuppresses statistics for the whole table, permanently, for anything that has ever inlined. Details in the issue.#217 fixed the same function on
v1.0by advancingnext_file_idand degrading the column stats to NULL, and named the precise-stats version as a refactor targetingmain. This is that refactor.mainhas neither half today, so in this function it is behind the release branch.Widening instead of degrading
Each inserted batch widens the persisted bounds: never shrink, never blank, never seed. "Never seed" is per bound rather than per row, because a bound derived from one batch is not a bound over the table. So the writer never creates a stats row for a column that has none, and never fills a missing
minormaxon a row that already carries the other one. A NULL bound only costs pruning; a too-narrow one is silently wrong, and not only for pruning, since DuckDB's compressed materialization uses a table-level min as an exact re-base constant.Decoding goes through
FromGlobalStatsand merging throughMergeStats, so the comparison semantics cannot drift from the engine's. Per snapshot that is one SELECT plus one UPDATE, skipped when no bound moved.Write-path cost, measured on this branch: +7% at batch=1000 and +20% at batch=100. The residual is flat per-statement overhead rather than a width effect (it shows identically at one column, 143 vs 114 ms), so cost no longer scales with column count: width sensitivity is 5.7 ms/col against a 5.4 baseline, where a naive per-column implementation cost 26.7. The accumulator folds PG Datums instead of building a
duckdb::Valueper cell, which is a deliberate departure from reusing the engine's types and worth about 6x on this path for byte-identical bounds.This supersedes v1.0's degrade-to-NULL rather than porting it: blanking wipes bounds for already-flushed parquet data too, which bakes in wrong results after a later flush.
contains_null
Retiring 006 means the reader trusts the whole stats row, not just the bounds, and
contains_nullis part of it. Left stale atfalsewhile an inlined NULL exists, DuckDB foldsIS NULLto false and the row disappears from the result, whileIS NOT NULLreports it as non-NULL. That is a wrong answer rather than lost pruning, so retiring the guard has to cover it for the same reason it has to cover the bounds.It rides the same
MergeStatscall, asnull_count, which the catalog persists as the boolean. The OR, the monotone false->true and the sticky unknown-degrade are all the engine's rather than a second implementation here.MergeStatsfoldsnull_countbefore its!AnyValid()early return, so an all-NULL batch updates null-ness without touching bounds.Counting is deliberately not gated on the min/max eligibility test. That test gates comparability, which has no bearing on null-ness, and native's inline writer (
TemplatedUpdateStats) counts NULLs for every column whatever its type. Gating it would make the fast path disagree with the normal path on the sameINSERT. All three inline writers do it --VALUES,UNNESTandCOPY FROM STDIN-- each having its own null branch.On a table that has only ever held inlined rows,
contains_nullstays SQL NULL rather than becoming true:MergeStats'has_null_countdegrade is sticky and no widen can recover it. That is native behavior and it is safe, since unknown makes noHasNoNullclaim and nothing folds.next_file_id
Forward-ports #217's first fix, load-bearing here for a second reason: DuckLake keys its table-stats cache on
next_file_id, which an inline insert does not advance, so a scan taken between two inline inserts in one session serves the pre-widening cached zone map however truthful the catalog is. With both halves in place, 006 retires with no replacement vendored patch: net -1.It also fixes #217's Drift 1 corruption on
main, which is data loss rather than lost pruning. The stale cached stats carrynext_row_id, so a normal-path insert after direct inserts in the same backend reuses row ids: onmaintoday such a table holds 4 rows with 3 distinctrow_ids, and deleting one row destroys an unrelated one. Verified by A/B on both binaries.Disclosures
Retiring 006 changes behavior on tables whose persisted bound can no longer be cast to the column's current type: four query shapes that work today start throwing at plan time. Those tables are already broken at the data level (
SELECT *throws either way), and the throw is upstream's, confirmed on stock DuckDB and stock ducklake with no vendored patches. 006 only moved it from planning to execution. The 11-shape matrix is in the issue. If you would rather land the upstream read-path fix first, re-adding the guard is a one-hunk revert and I can split it out.The accumulator forces ISO
DateStyleinsideFinalizeand restores it from a destructor, which a PGelog(ERROR)longjmps past, so an error there can leave the session pinned to ISO/YMD; neitherROLLBACKnorRESET ALLundoes it.direct_insert.cppalready does the same thing twice unguarded across a much wider window (:1526-1531restored at:1604-1607), spanning the conversion loop where errors actually happen, so fixing only the new and narrower instance would leave the one users hit. I would rather fix the pattern in a follow-up, either with theNewGUCNestLevel()idiom or by encoding temporal bounds without touching the global, but can fold it in here if you would prefer not to merge another instance.Tests
inlined_scan_stats_guardreworked to assert maintained bounds: fully-inlined widening, multi-file seed/flush/insert/flush, a half-populated stats row reached through supported SQL, DateStyle independence, and the between-inserts stale-cache case. Both properties have negative controls.Also forward-ports #217's
direct_insert_metadata, whichmainnever received. Itsnext_file_idand row-id-uniqueness sections pass byte-for-byte against v1.0's expected output. Its third section asserted that the fast path degrades stats to NULL because it cannot maintain them, which this change makes false, so it now asserts the widened bounds andcontains_nullinstead.Three further sections cover
contains_null: one column per inline writer, so a fix reachingVALUESandCOPYbut missingUNNESTstill fails; the min/max-ineligible types (DOUBLE,BLOB), which only this change makes reach the widen at all; and the inline-only table, asserting the value stays unknown and thatIS NULLstill returns the row across a flush.Regression 54/54 and isolation 3/3 on PG 18.4.
If this is backported to
v1.0: that branch degrades the stats to NULL in the same function (pgducklake_metadata_manager.cpp:833), and the backport must delete that statement. Left in, it runs after the widen and blanks the bounds this change just computed, silently and without failing any test.On the follow-up refactor #217 also promised, replacing this hand-written SQL with
UpdateGlobalTableStatsSql: deliberately not attempted here. That builder also writescontains_nan,extra_statsandducklake_table_stats, and its INSERT branch would create the stats rows this change is careful never to create. It is worth doing, but as its own PR rather than inside the one making these bounds correct.Fixes #237