feat(duckdb): report per-file column statistics from the vortex COPY writer by moshap-firebolt · Pull Request #9471 · vortex-data/vortex

moshap-firebolt · 2026-08-18T21:05:50Z

Rationale for this change

DuckDB's COPY hook copy_to_get_written_statistics lets a writer return WRITTEN_FILE_STATISTICS. The vortex COPY function didn't implement it, so COPY … (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 enforce NOT NULL on vortex columns.

Closes Implement copy_to_get_written_statistics for DuckDB COPY function #7819

What changes are included in this PR?

Implement copy_to_get_written_statistics for the vortex COPY function, following the parquet writer's store-pointer-then-fill-at-finalize pattern. This makes COPY … (FORMAT vortex, RETURN_STATS) work.
Per file: row_count, file_size_bytes. Per column: min/max, null_count, num_values, has_nan (float columns), and column_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.
Statistics are read from the WriteSummary that copy_to_finalize previously 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".
e2e tests drive COPY … RETURN_STATS through DuckDB and assert the returned file statistics (including that nested struct/list columns do not crash the hook); a plain COPY without RETURN_STATS is 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.h is regenerated. No public Rust API changes.

codspeed-hq · 2026-08-18T21:26:13Z

Merging this PR will degrade performance by 11.59%

⚠️

Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

❌ 1 regressed benchmark
✅ 1980 untouched benchmarks
⏩ 54 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	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)}

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. ↩

myrrc

Thanks for the contribution. The changes mostly look good but let's remove some comments and add some others :)

myrrc · 2026-08-19T09:10:56Z

    }

    unique_ptr<CData> ffi_data;
+    // Non-owning; set in copy_to_get_written_statistics (before the write) and filled in


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

myrrc · 2026-08-19T09:12:16Z

+        // 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;


Please add D_ASSERT(global.written_stats != nullptr)

myrrc · 2026-08-19T09:16:34Z

+// 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;


Nit: can we move struct-wide comment about min-max being owned directly to these fields?

I.e.

// Owned value duckdb_value max;

myrrc · 2026-08-19T09:17:14Z

+    // 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++) {


Is there a situation when file_stats.num_columns != names.size()? If no, can we remove this part, if yes, can we clarify, when?

myrrc · 2026-08-19T09:19:33Z

+/// 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() {


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?

myrrc · 2026-08-19T09:21:21Z

+        return Ok(None);
+    };
+    let stats_sets = file_stats.stats_sets();
+    if column_index >= stats_sets.len() {


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.

right, it cannot happen

myrrc · 2026-08-19T09:22:42Z

    try_or(error_out, || copy_to_finalize(global_data))
 }
+
+/// Fill file-level statistics of the just-written Vortex file. Returns `false` if the


ffi functions shouldn't have comments, please remove them

…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>

myrrc · 2026-08-24T14:24:06Z

    }
    auto cdata = unique_ptr<CData>(reinterpret_cast<CData *>(ffi_bind_data));
-    return make_uniq<VortexCopyBindData>(std::move(cdata));
+    auto bind = make_uniq<VortexCopyBindData>(std::move(cdata));


Since there's no other way to construct VortexCopyBindData, let's move "column_names" to be part of the constructor.

myrrc · 2026-08-24T14:42:49Z

Let's fix the rest comments and the tests, and the PR is good to go

myrrc · 2026-08-24T14:44:18Z

+                                    FunctionData &,
+                                    GlobalFunctionData &gstate,
+                                    CopyFunctionFileStatistics &statistics) {
+    gstate.Cast<VortexCopyGlobalState>().written_stats = &statistics;


Let's add a comment about lifetimes here. This code is sane because "statistics" is an optional_ptr on the caller, and Parquet reader does the same, but this isn't obvious (and rather looks as UB) if looking at the function alone

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>

moshap-firebolt added the changelog/feature A new feature label Aug 18, 2026

moshap-firebolt force-pushed the feat/rtdl-written-statistics-upstream branch from fbcce30 to 6fd42b6 Compare August 18, 2026 22:26

moshap-firebolt requested a review from myrrc August 18, 2026 23:53

moshap-firebolt marked this pull request as ready for review August 18, 2026 23:53

moshap-firebolt force-pushed the feat/rtdl-written-statistics-upstream branch from 6fd42b6 to 56d106f Compare August 18, 2026 23:53

myrrc reviewed Aug 19, 2026

View reviewed changes

myrrc added the ext/duckdb Relates to the DuckDB integration label Aug 19, 2026

myrrc self-assigned this Aug 20, 2026

moshap-firebolt and others added 2 commits August 21, 2026 13:57

moshap-firebolt force-pushed the feat/rtdl-written-statistics-upstream branch from 56d106f to b56818c Compare August 22, 2026 04:10

myrrc approved these changes Aug 24, 2026

View reviewed changes

myrrc reviewed Aug 24, 2026

View reviewed changes

myrrc self-requested a review August 24, 2026 14:44

Uh oh!

Conversation

moshap-firebolt commented Aug 18, 2026

Rationale for this change

What changes are included in this PR?

What APIs are changed? Are there any user-facing changes?

Uh oh!

codspeed-hq Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will degrade performance by 11.59%

Performance Changes

Footnotes

Uh oh!

myrrc left a comment

Choose a reason for hiding this comment

Uh oh!

myrrc Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myrrc commented Aug 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codspeed-hq Bot commented Aug 18, 2026 •

edited

Loading

myrrc Aug 19, 2026 •

edited

Loading