feat(duckdb): report per-file column statistics from the vortex COPY writer - #9471
feat(duckdb): report per-file column statistics from the vortex COPY writer#9471moshap-firebolt wants to merge 3 commits into
Conversation
Merging this PR will degrade performance by 11.59%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | cold_misaligned[(16, 64)] |
380.2 µs | 430.1 µs | -11.59% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing feat/rtdl-written-statistics-upstream (522eba4) with develop (e4b3421)
Footnotes
-
54 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
fbcce30 to
6fd42b6
Compare
6fd42b6 to
56d106f
Compare
myrrc
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The changes mostly look good but let's remove some comments and add some others :)
…writer Implement DuckDB's copy_to_get_written_statistics hook for the vortex COPY function, mirroring the parquet writer, so callers that request WRITTEN_FILE_STATISTICS (e.g. DuckLake) receive per-file, per-column stats instead of only a changed-row count. The stats are read from the WriteSummary that copy_to_finalize previously dropped - no file is re-opened. Per column we report min/max (from the footer FileStatistics, converted via the existing column_statistics bridge), null_count, num_values, and column_size_bytes (the on-disk compressed size via WriteSummary::compressed_column_sizes, the same quantity parquet reports). The hook is opt-in: when the caller does not request statistics the finalize path is unchanged. Includes a unit test that writes an int/varchar/nullable-double struct and asserts the derived row/column counts, null counts, min/max presence, and a non-zero on-disk column size. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Mosha (RTDL) <moshap@firebolt.io>
Remove the call-order narration comments, assert the statistics target and the statistics/column-count invariant instead of silently truncating the loop, and turn the two unreachable branches in the statistics getter into errors, covered by new unit tests. Move the RETURN_STATS end-to-end coverage from a Rust e2e test into a sqllogictest, which asserts the per-column statistics themselves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mosha (RTDL) <moshap@firebolt.io>
56d106f to
b56818c
Compare
|
Let's fix the rest comments and the tests, and the PR is good to go |
Move column_names into VortexCopyBindData's constructor, and document why copy_to_get_written_statistics may keep the statistics pointer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mosha (RTDL) <moshap@firebolt.io>
|
Good! Let's fix the test and merge this. |
Rationale for this change
DuckDB's COPY hook
copy_to_get_written_statisticslets a writer returnWRITTEN_FILE_STATISTICS. The vortex COPY function didn't implement it, soCOPY … (FORMAT vortex, RETURN_STATS)failed at bind (RETURN_STATS is not supported for the "vortex" copy format) and callers such as DuckLake could not record per-column statistics or enforceNOT NULLon vortex columns.What changes are included in this PR?
copy_to_get_written_statisticsfor the vortex COPY function, following the parquet writer's store-pointer-then-fill-at-finalize pattern. This makesCOPY … (FORMAT vortex, RETURN_STATS)work.row_count,file_size_bytes. Per column:min/max,null_count,num_values,has_nan(float columns), andcolumn_size_bytes(on-disk compressed size; excludes bytes not attributable to a column, so per-column sizes do not sum to the file size). Only top-level columns are reported — the footer exposes one statistics set per top-level field.WriteSummarythatcopy_to_finalizepreviously dropped; no file is re-opened. A scalar-conversion failure is surfaced through the copy function's error channel rather than swallowed as "no statistics".COPY … RETURN_STATSthrough DuckDB and assert the returned file statistics (including that nested struct/list columns do not crash the hook); a plainCOPYwithoutRETURN_STATSis unchanged.What APIs are changed? Are there any user-facing changes?
Yes —
COPY … (FORMAT vortex, RETURN_STATS)starts working (previously a bind-time error). Internally, two new C FFI entry points and two FFI structs;vortex.his regenerated. No public Rust API changes.