[Data] [4/11] Add Parquet footer types and the online bin packer - #65210
Conversation
Groundwork for footer-based Parquet chunking: the data types describing a file's row groups, and the packer that groups them into read units. `OnlineBinPacker` is a streaming, locality-preserving bin packer. Light colours (files whose total size fits the bin budget) share First-Fit bins; heavy colours get dedicated monochromatic Next-Fit bins, so a large file's row groups stay contiguous instead of being scattered across every open bin. Bins are sealed and evicted as they fill, which bounds memory over an unbounded file stream. `coalesce_row_groups` merges runs of consecutive row groups into ~target-byte chunks, breaking on a change in match class, an index gap, or once the accumulator is full. It gets its own module rather than living beside the footer-reading actor, since it is pure and worth testing alone. All of it is unused so far and needs no Ray cluster to test. `ParquetRowGroupChunkMetadata` is added alongside the existing `ParquetFileChunkMetadata`; the size-based chunker still needs its own metadata type until the read path switches over. Signed-off-by: Goutam <goutam@anyscale.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a footer-based Parquet chunking and online bin-packing implementation, including row-group coalescing logic, streaming bin-packing algorithms, and corresponding unit tests. Feedback on the changes highlights an issue in the best-fit bin selection logic (_best_open_bin), where the packer should prioritize maximizing the number of units packed before minimizing the leftover gap to align with its intended behavior.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit dc134d7. Configure here.
| def _place(self, item: BinItem) -> None: | ||
| item_bytes = item.uncompressed_size | ||
| seen_bytes = self._seen_bytes_by_path.get(item.path, 0) | ||
| self._seen_bytes_by_path[item.path] = seen_bytes + item_bytes |
There was a problem hiding this comment.
why do we add uncompressed size and compressed size for a path?
There was a problem hiding this comment.
no it should be adding only uncompressed sizes for a path
| # file was light -- otherwise a splittable oversized run would scatter | ||
| # across shared bins. | ||
| self._heavy.switch_to(item.path) | ||
| self._pack(item, self._heavy) |
There was a problem hiding this comment.
do we have a test case covering the last else condition?
There was a problem hiding this comment.
I did not fully understand this one
There was a problem hiding this comment.
I clarified it in the comments.
There was a problem hiding this comment.
And yes there are a few tests that test this else clause.
abhishekverma-ray
left a comment
There was a problem hiding this comment.
Left a few comments, please check. Otherwise looks good to me.
Signed-off-by: Goutam <goutam@anyscale.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a streaming, footer-based Parquet chunking and bin-packing mechanism, including row-group coalescing and an online bin packer to group chunks into partitions. The reviewer suggests adding validation checks across several new dataclasses and classes, specifically ensuring that row-group metadata sizes match counts for coalesced runs, validating that configuration parameters like bin bytes and open bin limits are positive, and verifying that the coalescing target size is non-negative.
| rg_sizes: Tuple[int, ...] = () | ||
| rg_rows: Tuple[int, ...] = () |
There was a problem hiding this comment.
For defensive programming and to prevent malformed metadata from causing silent issues downstream, we should validate that rg_sizes and rg_rows are populated and match rg_count when rg_count > 1.
| rg_sizes: Tuple[int, ...] = () | |
| rg_rows: Tuple[int, ...] = () | |
| rg_sizes: Tuple[int, ...] = () | |
| rg_rows: Tuple[int, ...] = () | |
| def __post_init__(self) -> None: | |
| if self.rg_count > 1: | |
| if not self.rg_sizes or len(self.rg_sizes) != self.rg_count: | |
| raise ValueError(f"rg_sizes must have length {self.rg_count} for coalesced runs") | |
| if not self.rg_rows or len(self.rg_rows) != self.rg_count: | |
| raise ValueError(f"rg_rows must have length {self.rg_count} for coalesced runs") |
| rg_sizes: Tuple[int, ...] = () | ||
| rg_rows: Tuple[int, ...] = () |
There was a problem hiding this comment.
Similar to RowGroupInfo, we should validate that rg_sizes and rg_rows are populated and match rg_count when rg_count > 1 in BinItem to ensure metadata integrity.
| rg_sizes: Tuple[int, ...] = () | |
| rg_rows: Tuple[int, ...] = () | |
| rg_sizes: Tuple[int, ...] = () | |
| rg_rows: Tuple[int, ...] = () | |
| def __post_init__(self) -> None: | |
| if self.rg_count > 1: | |
| if not self.rg_sizes or len(self.rg_sizes) != self.rg_count: | |
| raise ValueError(f"rg_sizes must have length {self.rg_count} for coalesced runs") | |
| if not self.rg_rows or len(self.rg_rows) != self.rg_count: | |
| raise ValueError(f"rg_rows must have length {self.rg_count} for coalesced runs") |
| def __init__( | ||
| self, | ||
| max_bin_bytes: int, | ||
| *, | ||
| max_shared_open_bins: int = 16, | ||
| split_coalesced: bool = False, | ||
| ): |
There was a problem hiding this comment.
Add validation for max_bin_bytes and max_shared_open_bins to prevent division by zero, infinite loops, or ValueError when calling max() on an empty sequence of bins.
| def __init__( | |
| self, | |
| max_bin_bytes: int, | |
| *, | |
| max_shared_open_bins: int = 16, | |
| split_coalesced: bool = False, | |
| ): | |
| def __init__( | |
| self, | |
| max_bin_bytes: int, | |
| *, | |
| max_shared_open_bins: int = 16, | |
| split_coalesced: bool = False, | |
| ): | |
| if max_bin_bytes <= 0: | |
| raise ValueError("max_bin_bytes must be positive") | |
| if max_shared_open_bins <= 0: | |
| raise ValueError("max_shared_open_bins must be at least 1") |
| if not target: | ||
| return tuple(per_rg) |
There was a problem hiding this comment.
|
@abhishekverma-ray in a follow up PR, I'll decouple the indexer from partitioner that way I can reuse the |
…ner (#65214) ## Why A metadata-aware indexer wants the pushed-down predicate, projection, and limit at **listing** time — to prune files by their statistics, size only the projected columns, and stop listing early once enough rows are found. (The footer-based Parquet indexer, later in this split, is the first such consumer.) ## Flow ```mermaid flowchart TD R["rule loop: projection → predicate → limit → count<br/>(runs to a fixed point)"] --> S["ReadFiles.scanner<br/>final accepted state"] S -->|"pushed_predicate / pruned_column_names / pushed_limit"| D["DeriveListFilesPushdown<br/>_post_optimize, runs last"] D -->|"overwrite, unconditionally, both directions"| L["ListFiles.predicate / projected_columns / limit"] L --> P[plan_list_files_op] --> I["FileIndexer.list_files(...)<br/>plain indexer ignores them"] ``` `DeriveListFilesPushdown` recomputes every `ListFiles`' constraints from the scanner of the `ReadFiles` that consumes it. Unconditional in both directions: a `ListFiles` whose consumer is *not* a `ReadFiles` — e.g. after `PushdownCountFiles` rewrites the plan — has its constraints cleared. So a rule that weakens a scanner's predicate automatically weakens listing too, and the worst a future rule can cause is listing *more* than it needs to. It runs in `LogicalOptimizer._post_optimize`, after the rule loop reaches a fixed point, rather than as a `_LOGICAL_RULESET` entry. `get_logical_ruleset().add()` is a `@DeveloperAPI`, so a caller-registered rule could otherwise be topologically ordered *after* it and strand a stale predicate. ## What changed **The rule and its inputs** (the substance — start here): | File | Change | | --- | --- | | `logical/rules/derive_list_files_pushdown.py` | **new** — the rule | | `datasource_v2/logical_optimizers.py` | read-back accessors `pushed_predicate()` / `pushed_limit()`, plus the `derive_list_files_pushdown(scanner)` helper that `isinstance`-guards each mixin | | `datasource_v2/scanners/arrow_file_scanner.py` | implements both accessors | | `logical/operators/read_operator.py` | `ListFiles` gains `predicate` / `projected_columns` / `limit` | | `logical/optimizers.py`, `logical/rules/__init__.py` | register and export the rule | **Plumbing so an indexer can consume it** (no consumer yet): | File | Change | | --- | --- | | `datasource_v2/listing/file_indexer.py` | `list_files()` accepts the three as kwargs (the plain indexer documents that it ignores them); new `produces_partitioned_manifests` property, default `False` | | `datasource_v2/listing/listing_utils.py` | forwards the kwargs | | `planner/plan_list_files_op.py`, `read_api.py` | forward the constraints, and honor `produces_partitioned_manifests` by skipping the partitioner and listing in a single task | **Tests:** `tests/unit/datasource_v2/test_derive_list_files_pushdown.py` (new, 9 tests). Both new accessors are **concrete, defaulting to `None`**, not abstract — these are `@DeveloperAPI` mixins, and an out-of-tree scanner should keep working (just without listing-time pruning) rather than fail to instantiate on upgrade. `None` is always the safe answer. **Inert on its own** — nothing reads the derived state yet. It is separated precisely because it is the soundness argument for the feature, and much easier to review on its own than buried in a read-path rewrite. ## Stack Step 3 of an 11-step split of #64985, which was 43 files / +3234 −719 and not reviewable as one unit. - #65167 — `[1/11]` Extract `FileIndexer.list_file_infos` ✅ merged - #65168 — `[2/11]` Push limit into `ReadFiles` when `Limit` sits directly on it ✅ merged - **this PR** — `[3/11]`, unblocked by the two above (it extends the same files) - #65210 — `[4/11]` Footer types + online bin packer (open, independent) - #65169 — `[9/11]` Pin the Parquet footer actor pool to 1 in tests (open, independent) --------- Signed-off-by: Goutam <goutam@anyscale.com>

Why
Groundwork for footer-based Parquet chunking (the rest of the split of #64985). Everything here is pure logic with no Ray dependency, landed on its own so it can be reviewed as an algorithm rather than as part of a read-path rewrite.
Today
ParquetFileChunkersplits each file byceil(file_size / 1 GiB)without reading the footer, and the partitioner bin-packs those guesses using a fixed encoding-ratio estimate. Bin sizes are inaccurate because the compression ratio is unknown until the footer is read. Replacing that needs a packer that works from real row-group metadata, arrives as a stream, and bounds its own memory.Nothing calls this yet — it is unused until the footer indexer lands.
Flow
The default path: pack the row groups themselves
Out of the box both coalescing and splitting are off (
coalesce_bytes = 0,split_coalesced = False). One bin item is one physical row group, andOnlineBinPackerFirst-Fits those whole items into big bins ofmax_bin_bytes.One big bin becomes one read task. Nothing is ever cut.
Optional: a second, smaller bin size underneath
Setting
coalesce_bytes > 0inserts a first packing pass upstream, in the footerreader:
coalesce_row_groupspacks consecutive row groups into contiguous runs of~
coalesce_bytes. Each run is a small bin, and the packer then packs small bins intobig bins instead of packing row groups directly. Fewer, larger items means fewer
descriptors crossing to the driver.
That raises one question the default path never has to answer: a small bin is a
pre-packed bundle, so what happens when it doesn't fit in the space left in a big bin?
split_coalescedis that answer, and it is also opt-in:flowchart TD RG["physical row groups"] -->|"optional level 1: coalesce_bytes"| SB["small bins<br/>contiguous row-group runs"] SB --> FIT{"fits in an open big bin?"} FIT -->|yes| PUT["drop it in whole"] FIT -->|"no — split_coalesced = false (default)"| NEW["keep it intact,<br/>start a new big bin"] FIT -->|"no — split_coalesced = true"| CUT["break it back into its row groups:<br/>fill the leftover space,<br/>carry the remainder onward"] PUT --> SEAL["level 2: seal + evict<br/>full big bins (max_bin_bytes)"] NEW --> SEAL CUT --> SEAL SEAL --> M["one big bin = one read task"]So the three configurations are:
coalesce_bytessplit_coalesced0(default)False(default)> 0False> 0TrueTwo properties worth checking during review:
split_coalescedcannot change the default path. Without coalescing there are nosmall bins to break open — a bin item already is one row group — so the flag is inert
and packing is plain First-Fit either way. Guarded by
test_split_coalesced_is_noop_without_coalescing.every piece stays a contiguous run with exact sizes and row counts. A run larger than a
whole big bin is simply cut across several of them.
Orthogonal to all of the above, big bins come from two pools. A file's chunks go to a
shared pool (mixed files) until that file's cumulative bytes exceed the cap, after
which it switches to a single-file pool — so a large file's row groups stay contiguous
instead of scattering across every open bin. Bins seal and evict as they fill, which
bounds memory over an unbounded stream and lets read tasks start before every footer has
landed.
What changed
chunkers/parquet_footer_types.pyRowGroupInfo,FileChunks,BinItem,Bin. Frozen dataclasses, no imports beyondtyping.chunkers/parquet_row_group_coalescing.pycoalesce_row_groups(). A run breaks on a change infully_matched(never merge across the match-class boundary, or limit push-down miscounts), a gap inrg_idx(filter-pruned groups), or once the accumulator is full.target == 0disables it.partitioners/online_bin_packer.pyOnlineBinPackerplus the_BinPool/_SharedBinPool/_SingleFileBinPoolhierarchy. A file stays in the shared pool until its cumulative bytes exceed the cap, then switches to a dedicated pool so a large file's row groups stay contiguous instead of scattering across every open bin. Bins seal and evict as they fill, which bounds memory over an unbounded stream and lets read tasks start before every footer has landed.chunkers/file_chunker.pyParquetRowGroupChunkMetadataalongside the existingParquetFileChunkMetadata. Additive on purpose — the size-based chunker still needs its own metadata type until the read path switches over.tests/unit/datasource_v2/test_online_bin_packer.py864 insertions, 0 deletions.
Testing
Coverage is aimed at the invariants rather than the happy path: that every row group is covered exactly once across all emitted bins (parametrized over
split_coalesced), that splitting is a no-op without coalescing, that an oversize coalesced run is cut at boundaries and can fill residual shared-bin space, and that a full single-file bin is sealed immediately.Stack
Step 4 of an 11-step split of #64985, which was 43 files / +3234 −719 and not reviewable as one unit.
[1/11]ExtractFileIndexer.list_file_infos✅ merged[2/11]Push limit intoReadFileswhenLimitsits directly on it ✅ merged[3/11]DeriveListFilespushdown state from theReadFilesscanner (open)[4/11], no merge-order dependency: it touches a file set disjoint from every other step and applies cleanly to master on its own.[9/11]Pin the Parquet footer actor pool to 1 in tests (open, independent)Step 5 (
FooterReader) importsparquet_footer_typesandparquet_row_group_coalescingfrom this PR, so it follows this one.Not a duplicate of any open PR (checked
gh pr list --repo ray-project/ray --state openplus a file-level overlap check; only #64985, which this replaces).AI assistance was used; every line reviewed by me and tests run locally.