fix(vortex): compute per-file column statistics in-stream for vortex writes (mixed parquet+vortex corruption) - #6
moshap-firebolt wants to merge 1 commit into
Conversation
17e81f8 to
49f2a21
Compare
49f2a21 to
0faa88c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0faa88c. Configure here.
| } | ||
| vector<string> name_path {bind_data.names[c]}; | ||
| statistics.column_statistics[DuckLakeUtil::ToQuotedList(name_path)] = std::move(column_stats); | ||
| } |
There was a problem hiding this comment.
Variant columns not skipped in stats
Medium Severity
StatsCopyFinalize writes null_count/num_values for every sunk column, including VARIANT. AddWrittenFiles then rejects top-level variant stats, so a vortex insert into a table that has a VARIANT column fails after the file is written. The intended skip of variant columns is not implemented.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0faa88c. Configure here.
…writes The vortex COPY reports only a file list, so vortex data files were registered without per-file column statistics and the global table stats were never updated. Readers trust those stats, which turned the stale bounds into wrong results on mixed parquet+vortex tables: - filters on values outside the stale bounds folded to empty (WHERE id = 300 -> no rows, count(*) WHERE id >= 250 -> 0) - DuckDB's sort-key compression packed ORDER BY columns into the stale range's width, returning values truncated mod 256 for ORDER BY..LIMIT Fix: DuckLakeStatsCopy wraps any sink-based COPY function that lacks a copy_to_get_written_statistics hook. The wrapper accumulates per-column min/max (arg-min/max raw scan per chunk, one Value materialization per chunk), null/value counts, and NaN detection for FLOAT/DOUBLE (NaN is excluded from bounds and flagged, parquet-style) as the chunks stream through the sink - the same point where the parquet writer computes its statistics - and reports them through WRITTEN_FILE_STATISTICS. No extra pass over the written file; no extension change needed. The insert path then consumes one unified WRITTEN_FILE_STATISTICS chunk shape for every format (the non-parquet file-list special case is gone; footer_size tolerates NULL since only parquet reports one). Vortex files therefore get real ducklake_file_column_stats rows (file pruning now works on them), the global bounds stay correct, and NOT NULL constraints are enforced from the written null counts - the previous NOT NULL rejection for non-parquet formats is lifted. Bounds are recorded for types whose physical order matches logical order and whose values round-trip through the stats VARCHAR form; other columns (nested, blob, uuid, interval, oversized strings) record counts only. This also unblocks requesting WRITTEN_FILE_STATISTICS for vortex, which previously crashed on the missing extension hook (feat/rtdl-vortex-written-stats); if the duckdb-vortex extension later implements the hook natively, the wrapper steps aside automatically (NeedsWrapping). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Added adversarial coverage in Also pins three write-time type limitations discovered while testing (clean bind errors, so a silent behaviour change gets noticed): vortex rejects HUGEINT/UUID/INTERVAL; ducklake rejects ENUM. Full suite: 397/399, same two pre-existing env-dependent failures as unpatched main. |
0faa88c to
8c47c26
Compare


Problem
Vortex data files were registered without per-file column statistics (the vortex COPY reports only a file list), so
ducklake_file_column_statsgot no rows andducklake_table_column_statswas never updated past the parquet-only bounds. Readers trust those stats. On a table with a parquet file (ids 0–249) and a vortex file (ids 250–499):SELECT id, s FROM m WHERE id = 300→ no rows (filter folded to empty against stale max=249)SELECT count(*) FROM m WHERE id >= 250→ 0SELECT id, s FROM m ORDER BY id DESC LIMIT 2→ 243, 242 — DuckDB's sort-key compression packs the sort column into the stale range's width, truncating values mod 256Silent wrong results, not errors.
Fix — compute the statistics while writing, like parquet
New
DuckLakeStatsCopywraps any sink-based COPY function that lacks acopy_to_get_written_statisticshook. The wrapper accumulates per-column min/max (arg-min/max raw comparison scan per chunk; oneValuematerialization per chunk per bound), null/value counts, and NaN detection for FLOAT/DOUBLE (NaN excluded from bounds and flagged, parquet-style) as the chunks stream through the sink — the same point where the parquet writer computes its statistics — and reports them throughWRITTEN_FILE_STATISTICS. No extra pass over the written file. No extension change needed.The insert path now consumes one unified
WRITTEN_FILE_STATISTICSchunk shape for every format (the non-parquet file-list special case is gone;footer_sizetolerates NULL since only parquet reports one). Consequences:ducklake_file_column_statsrows → file pruning now works on vortex datavortex_write.testupdated)Bounds are recorded for types whose physical order matches logical order and whose values round-trip through the stats VARCHAR form; other columns (nested, blob, uuid, interval, oversized strings >1KB) record counts only — counts are what NOT NULL and
AnyValidneed.Relation to feat/rtdl-vortex-written-stats (PR #4)
That branch flipped the return type to
WRITTEN_FILE_STATISTICSassuming the vortex extension implements the hook — it doesn't (the pinned duckdb-vortex has nocopy_to_get_written_statistics), so duckdb calls a null function pointer at file open and segfaults. This PR adopts that branch's insert-side unification and supplies the missing statistics via the wrapper. If the extension later implements the hook natively,DuckLakeStatsCopy::NeedsWrappingmakes the wrapper step aside automatically.Validation
test/sql/vortex/vortex_mixed_format_stats.test: real per-file stats for the vortex file (bounds, null/value counts), correct global bounds spanning both files, all three previously-wrong query shapes, NOT NULL enforcement. Manually verified NaN handling (bounds exclude NaN,contains_nanrecorded on file and global stats).test/sql/*: 396/398 — the 2 failures (settings/max_retry_count.test,metadata/ducklake_settings_sqlite.test) fail identically on unpatchedmainin this environment (env-dependent, unrelated).🤖 Generated with Claude Code
Note
Medium Risk
Changes core insert/statistics registration for non-parquet writes; incorrect min/max or merge logic could reintroduce filter pruning or constraint bugs, though scope is limited to single-file vortex paths with extensive test coverage.
Overview
Fixes silent wrong results on mixed parquet+vortex tables where vortex files were registered without per-file column statistics, leaving stale parquet-only bounds in catalog metadata (broken filters,
ORDER BY ... LIMITtruncation).Adds
DuckLakeStatsCopy, a wrapper around sink-based COPY functions that lackcopy_to_get_written_statistics(e.g. vortex). It accumulates min/max, null/value counts, and NaN flags while chunks stream through the sink, then reportsWRITTEN_FILE_STATISTICS(file size from the filesystem;footer_sizeleft unset).The DuckLake insert path drops the vortex file-list special case and always consumes the parquet-shaped statistics chunk (nullable
footer_size). Non-parquet writers get the wrapper inGetCopyOptionswhenNeedsWrappingis true. NOT NULL enforcement for vortex is enabled from computed null counts (the previous blanket rejection is removed).New regression tests cover mixed-format stats, type edge cases, and updated
vortex_write.testexpectations.Reviewed by Cursor Bugbot for commit 8c47c26. Bugbot is set up for automated code reviews on this repo. Configure here.