Skip to content

feat(duckdb): report per-file column statistics from the vortex COPY writer - #9471

Open
moshap-firebolt wants to merge 3 commits into
developfrom
feat/rtdl-written-statistics-upstream
Open

feat(duckdb): report per-file column statistics from the vortex COPY writer#9471
moshap-firebolt wants to merge 3 commits into
developfrom
feat/rtdl-written-statistics-upstream

Conversation

@moshap-firebolt

Copy link
Copy Markdown

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.

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

codspeed-hq Bot commented Aug 18, 2026

Copy link
Copy Markdown

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 benchmarks1

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)

Open in CodSpeed

Footnotes

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

@moshap-firebolt moshap-firebolt added the changelog/feature A new feature label Aug 18, 2026
@moshap-firebolt
moshap-firebolt force-pushed the feat/rtdl-written-statistics-upstream branch from fbcce30 to 6fd42b6 Compare August 18, 2026 22:26
@moshap-firebolt
moshap-firebolt requested a review from myrrc August 18, 2026 23:53
@moshap-firebolt
moshap-firebolt marked this pull request as ready for review August 18, 2026 23:53
@moshap-firebolt
moshap-firebolt force-pushed the feat/rtdl-written-statistics-upstream branch from 6fd42b6 to 56d106f Compare August 18, 2026 23:53

@myrrc myrrc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread vortex-duckdb/cpp/copy_function.cpp Outdated
Comment thread vortex-duckdb/cpp/copy_function.cpp
Comment thread vortex-duckdb/cpp/include/table_function.h
Comment thread vortex-duckdb/cpp/copy_function.cpp Outdated
Comment thread vortex-duckdb/src/e2e_test/copy_statistics_test.rs Outdated
Comment thread vortex-duckdb/src/copy.rs
Comment thread vortex-duckdb/src/ffi.rs Outdated
@myrrc myrrc added the ext/duckdb Relates to the DuckDB integration label Aug 19, 2026
@myrrc myrrc self-assigned this Aug 20, 2026
moshap-firebolt and others added 2 commits August 21, 2026 13:57
…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>
@moshap-firebolt
moshap-firebolt force-pushed the feat/rtdl-written-statistics-upstream branch from 56d106f to b56818c Compare August 22, 2026 04:10
Comment thread vortex-duckdb/cpp/copy_function.cpp Outdated
@myrrc

myrrc commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

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

Comment thread vortex-duckdb/cpp/copy_function.cpp
@myrrc
myrrc self-requested a review August 24, 2026 14:44
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>
@myrrc
myrrc enabled auto-merge (squash) August 25, 2026 09:06
@myrrc

myrrc commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Good! Let's fix the test and merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/feature A new feature ext/duckdb Relates to the DuckDB integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement copy_to_get_written_statistics for DuckDB COPY function

2 participants