iceberg: bring back in-manifest stats - #31332
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the datalake → Iceberg pipeline to propagate Parquet per-column statistics (bounds/counts/sizes) into Iceberg manifest entries, and introduces byte-based bounding/backpressure to keep coordinator/commit memory usage under control.
Changes:
- Extract aggregated file-level Parquet column statistics and plumb them through
local_file_metadata→ coordinatordata_file→ Iceberg manifest metrics. - Add byte-based caps for commit chunking and coordinator pending backlog (new config tunables + derived-state accounting in coordinator STM).
- Add/extend unit + e2e tests covering parquet stats correctness, manifest serialization, and snapshot/backpressure behavior.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rptest/tests/datalake/datalake_e2e_test.py | Adds Spark-based e2e assertion that manifest .files readable_metrics are populated correctly. |
| src/v/serde/parquet/writer.h | Exposes file-level aggregated column stats from Parquet writer. |
| src/v/serde/parquet/writer.cc | Accumulates file-level value counts/sizes and returns per-column file stats. |
| src/v/serde/parquet/column_writer.h | Adds API to return aggregated per-column stats across row groups. |
| src/v/serde/parquet/column_writer.cc | Implements file-level stats aggregation/merge across flushes/row groups. |
| src/v/serde/parquet/BUILD | Makes parquet metadata library publicly visible for downstream use. |
| src/v/datalake/translation_task.cc | Plumbs local parquet column stats into coordinator data_file upload metadata. |
| src/v/datalake/tests/test_data_writer.h | Fixes test writer to move results out (avoid copies). |
| src/v/datalake/tests/serde_parquet_writer_test.cc | Adds unit tests validating stats encoding and cross-row-group aggregation. |
| src/v/datalake/tests/BUILD | Adds deps needed by new parquet stats unit tests. |
| src/v/datalake/serde_parquet_writer.h | Stores/returns extracted per-column stats post-finish. |
| src/v/datalake/serde_parquet_writer.cc | Extracts Parquet footer stats into per_column_stats for datalake usage. |
| src/v/datalake/local_parquet_file_writer.cc | Stores per-column stats into local_file_metadata after finish. |
| src/v/datalake/data_writer_interface.h | Extends parquet writer interface with optional column_stats() API. |
| src/v/datalake/coordinator/translated_offset_range.h | Adds estimated-memory helper for pending range accounting. |
| src/v/datalake/coordinator/tests/state_update_test.cc | Updates tests for new copy_bounded(max_files, max_bytes, ...) signature + adds pending totals test. |
| src/v/datalake/coordinator/tests/state_machine_test.cc | Adds snapshot hydration test ensuring derived pending totals are recomputed. |
| src/v/datalake/coordinator/tests/iceberg_file_committer_test.cc | Adds tests for manifest column stats + byte-based commit chunking; updates committer ctor usage. |
| src/v/datalake/coordinator/tests/data_file_test.cc | New tests for estimated-memory sizing behavior with/without column stats. |
| src/v/datalake/coordinator/tests/coordinator_test.cc | Adds coordinator backpressure test driven by byte limit. |
| src/v/datalake/coordinator/tests/BUILD | Adds new gtest target and dependencies for added coordinator tests. |
| src/v/datalake/coordinator/state.h | Adds byte-based bounded copy + derived pending totals API. |
| src/v/datalake/coordinator/state.cc | Implements byte-bounded copy + running totals + recompute after snapshot install. |
| src/v/datalake/coordinator/state_update.cc | Maintains running pending totals on add/commit/purge/reset updates. |
| src/v/datalake/coordinator/state_machine.cc | Recomputes derived pending totals when applying snapshots. |
| src/v/datalake/coordinator/iceberg_file_committer.h | Adds max_bytes_per_commit binding to committer. |
| src/v/datalake/coordinator/iceberg_file_committer.cc | Writes column metrics into Iceberg manifests + uses byte-bounded copy_bounded. |
| src/v/datalake/coordinator/data_file.h | Adds serialized column_stats to coordinator data_file + memory estimate helper. |
| src/v/datalake/coordinator/coordinator.h | Adds byte-based pending backlog limit binding to coordinator. |
| src/v/datalake/coordinator/coordinator.cc | Uses derived pending totals (files + bytes) for backpressure decision. |
| src/v/datalake/coordinator/coordinator_manager.cc | Wires new config bindings into coordinator and committer constructors. |
| src/v/datalake/coordinator/BUILD | Updates deps for coordinator build targets due to new types/usage. |
| src/v/datalake/BUILD | Adds base_types + parquet metadata deps for parquet stats plumbing. |
| src/v/datalake/base_types.h | Introduces per_column_stats + adds column stats to local_file_metadata. |
| src/v/config/configuration.h | Declares new tunables for byte-based commit/pending caps. |
| src/v/config/configuration.cc | Defines defaults/help strings for new datalake byte-based tunables. |
| _buffered_bytes = _flushed_bytes = 0; | ||
| co_return writer_error::ok; | ||
| } |
| if (cs.bounds.min) { | ||
| ps.lower_bound = iobuf_to_bytes(cs.bounds.min->value.copy()); | ||
| } | ||
| if (cs.bounds.max) { | ||
| ps.upper_bound = iobuf_to_bytes(cs.bounds.max->value.copy()); | ||
| } |
| coordinator::data_file uploaded{ | ||
| .remote_path = r.value()().string(), | ||
| .row_count = file.local_file.row_count, | ||
| .file_size_bytes = file.local_file.size_bytes, | ||
| .table_schema_id = file.schema_id, | ||
| .partition_spec_id = file.partition_spec_id, | ||
| .partition_key = std::move(pk_fields), | ||
| .column_stats = file.local_file.column_stats.copy(), | ||
| }; |
CI test resultstest results on build#87868test results on build#87960
test results on build#88390
|
1c96301 to
9c0c7c6
Compare
|
/ci-repeat 1 |
|
/ci-repeat 10 |
wdberkeley
left a comment
There was a problem hiding this comment.
Nice. This is a big improvement.
| state.pending_files() >= max_pending_files_() | ||
| || state.pending_bytes() >= max_pending_bytes_()) { | ||
| backpressured_as_of_ = now; | ||
| return true; |
There was a problem hiding this comment.
has_too_many_pending_files is technically a misnomer as we may have few files but their pending state takes too much memory. If you can come up with a good name it's worth renaming, but it's also ~fitting.
Also is it worth distinguishing (for an error message, metric) the cause of the backpressure, returning an enum or error code instead of a boolean?
There was a problem hiding this comment.
Only did a rename, opted out of an enum/error code. Seemed like kind of overkill but can add it if you feel strongly about it
There was a problem hiding this comment.
Don't feel strongly. Fine as is.
| // The estimate models seastar-allocator resident bytes, which is what the | ||
| // translator's memory semaphore admits writers against, so the measurement is | ||
| // only meaningful in builds that link that allocator. | ||
| TEST(ParquetWriter, ColumnMemoryEstimateCoversActual) { |
There was a problem hiding this comment.
This test is very Claude. It's a little sus but if it doesn't flake or act weird it's nice to have.
There was a problem hiding this comment.
Yea kinda agree, but it's pretty consistent it seems. And it's useful for picking a reasonable estimate (I think)
| // run that test (it prints the measured per-leaf on failure) and set this | ||
| // so the estimate falls back in range. Absolute bytes are allocator | ||
| // dependent, so the test compares a large-N slope, not an absolute figure. | ||
| constexpr size_t largest_column_object = std::max({ |
There was a problem hiding this comment.
Funny to see the AI go to such lengths with the test and the comment to guard the code against drift of the estimate, but this list would silently go stale if we add a physical type. 🤷♂️
There was a problem hiding this comment.
Yea, fair. The whole memory estimation is empirical anyway so I removed this.
9c0c7c6 to
09aa654
Compare
29b9ad4 to
4fbc545
Compare
Thread per-column stats (min/max bounds, null counts, value counts, column sizes) through to the Iceberg data_file manifest entry, where query engines use them for column-level predicate pushdown. Maintain a file-level column_stats_collector in buffered_column_writer that accumulates by merging after each flush_pages(), then use the result after the file is done to get file-level stats. (cherry picked from commit 7f83353)
(cherry picked from commit b4e8232)
Adds estimated_memory_bytes() for data_file and translated_offset_range. For use by a subsequent commit that bounds the coordinator's pending state by memory rather than file count.
has_too_many_pending_files() scans every pending entry on each request. Rather than keep scanning as we start to also account memory, maintain running totals on topics_state, updated as entries enter and leave pending state. The totals are derived, so they're left out of serde_fields() and rebuilt by recompute_pending() on snapshot install. Underflow is clamped rather than left to wrap: a wrapped total would sit above every backpressure limit and reject adds for the whole coordinator until the next recompute. For use by subsequent commits that shed load and chunk commits by memory.
A pending file's footprint varies widely with its column stats, so a file count is a poor proxy for the memory has_too_many_pending_files() is meant to bound. Adds datalake_coordinator_max_pending_bytes (soft, 32 MiB) and sheds when either total is over its limit. Reads the running totals, so the check is O(1) rather than a scan. The ducktape case puts the file count out of reach so nothing but the byte limit can reject a request, which makes the backpressure metrics on their own proof that the path works.
copy_bounded() caps the files copied into a single commit pass to bound its transient memory, but the same file count can hold far more memory once files carry column stats. Adds datalake_coordinator_max_bytes_per_commit (soft, 32 MiB) and stops the copy once either limit is hit.
Adds writer::estimated_memory(num_leaf_columns): sizeof(impl) plus a
per-column term. That term is empirical, covering the column writer object,
the chunks its containers take on the first row, the schema element and
allocator rounding.
ColumnMemoryEstimateCoversActual reports what has to be covered:
estimated_column_memory() needs at least 2920 to cover a byte_array
column; it is 4096
The slope holds near 2900 whether the second measurement is 2000 or 10000
columns, so 4KiB leaves about 40% over it. It only measures where seastar's
allocator is linked, and skips elsewhere.
For use by the datalake writer memory reservation in a subsequent commit.
Each open parquet writer costs the output stream buffer, held until close, plus a column writer per leaf column. A high-cardinality partition spec opens a writer per partition value, which can be far more than the datalake memory pool holds. The reservation is released on flush, so writers that stay open past one still hold memory the pool no longer accounts for.
column_writer::memory_usage() drives the translator's memory reservation but counted only buffered page data. Each leaf column also retains the row group's min/max bounds untruncated, which for byte-array columns grows with the value width.
iobuf_to_bytes() takes its argument by const ref and copies into a fresh bytes, so copying the bound first was an extra allocation and memcpy per column on every file finish.
4fbc545 to
5e64e28
Compare
|
Latest push was a rebase. Diff from right before then since the last review: https://github.com/redpanda-data/redpanda/compare/9c0c7c65f98846843c75c33f1b28eda526262a4f..4fbc54521534870c4d276a655e36f9320898871c |
|
/backport v26.2.x |
|
/backport v26.1.x |
|
/backport v25.3.x |
|
Failed to create a backport PR to v25.3.x branch. I tried: |
|
Failed to create a backport PR to v26.1.x branch. I tried: |
Reverts the revert and adds a few more fixes to make it slightly safer in certain pathological cases:
Backports Required
Release Notes