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 1 commit into
Conversation
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | slice_dict_tight_loop[10000] |
687 µs | 805.3 µs | -14.69% |
| ❌ | Simulation | slice_primitive_tight_loop[10000] |
421.4 µs | 476.4 µs | -11.56% |
| ⚡ | Simulation | take[small_m/shuffled/primitive/nonnull/chunks=16384/indices=16] |
1.2 ms | 1 ms | +15.72% |
| ⚡ | Simulation | cold_misaligned[(64, 256)] |
5 ms | 4.4 ms | +14.8% |
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 (56d106f) with develop (82cfea7)
Footnotes
-
89 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
…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>
6fd42b6 to
56d106f
Compare
myrrc
left a comment
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 :)
| } | ||
|
|
||
| unique_ptr<CData> ffi_data; | ||
| // Non-owning; set in copy_to_get_written_statistics (before the write) and filled in |
There was a problem hiding this comment.
Can we remove the "how it's used" part (set in ... and filled in ...) from this and other places? I see this as a common patterns LLM do, and it clutters the overall code. Removing it would also make the diff smaller
| // that is an internal inconsistency, not a silently empty result. | ||
| throw InternalException("vortex COPY: written statistics were requested but not produced"); | ||
| } | ||
| global.written_stats->row_count = file_stats.row_count; |
There was a problem hiding this comment.
Please add D_ASSERT(global.written_stats != nullptr)
| // Per-column statistics of a written Vortex file. `min`/`max` are owned | ||
| // duckdb_value handles (null if absent) that the caller must destroy. | ||
| typedef struct { | ||
| duckdb_value min; |
There was a problem hiding this comment.
Nit: can we move struct-wide comment about min-max being owned directly to these fields?
I.e.
// Owned value
duckdb_value max;
| // Keyed by top-level column name only. The vortex footer reports one statistics set per | ||
| // top-level field, so nested struct/list leaf columns get no statistics here (unlike parquet, | ||
| // which recurses to leaf paths). Flat tables are fully covered. | ||
| for (idx_t i = 0; i < file_stats.num_columns && i < names.size(); i++) { |
There was a problem hiding this comment.
Is there a situation when file_stats.num_columns != names.size()? If no, can we remove this part, if yes, can we clarify, when?
| /// Without `RETURN_STATS` the statistics hook is never invoked; a plain vortex COPY must still | ||
| /// succeed unchanged. | ||
| #[test] | ||
| fn copy_without_return_stats_still_works() { |
There was a problem hiding this comment.
This is already covered by sqllogic tests. Can you remove this test please?
On RETURN_STATS, on the other hand, can you add a sqllogic test with the same query?
| return Ok(None); | ||
| }; | ||
| let stats_sets = file_stats.stats_sets(); | ||
| if column_index >= stats_sets.len() { |
There was a problem hiding this comment.
When can this situation happen? If this is a virtual column, let's filter it via is_virtual_column function. Otherwise I this we can return an error or panic since this seems like a logical bug to me.
| try_or(error_out, || copy_to_finalize(global_data)) | ||
| } | ||
|
|
||
| /// Fill file-level statistics of the just-written Vortex file. Returns `false` if the |
There was a problem hiding this comment.
ffi functions shouldn't have comments, please remove them
2 participants
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.