[Data] [11/11] Enable footer-based Parquet reads by default - #65821
Conversation
Flips Parquet V2 to the footer read path and removes the old one. The release-test tuning and CI env pinning landed in earlier steps, so this is the small, revertible commit that actually changes behavior. Removes `RAY_DATA_PARQUET_ENABLE_FOOTER_INDEXER` from both call sites -- `_get_file_indexer` now always returns `FooterFileIndexer`, and `get_file_partitioner` always returns `OnlineBinPacker`. Deletes the blind size-based path: `ParquetFileChunker`, `ParquetFileChunkMetadata`, `_calculate_row_group_range`, `_fragments_from_chunk_metadata`, and the tests covering only it. Also removes the config that path left behind, which the deletion strands: `DataContext.parquet_chunker_target_chunk_size` and `DEFAULT_PARQUET_CHUNKER_TARGET_CHUNK_SIZE` were read only by `ParquetFileChunker.__init__`, so they would otherwise stay declared and documented while doing nothing. Behavior change worth calling out for review: read-task sizing no longer comes from `override_num_blocks` / `read_op_min_num_blocks` for Parquet. Bins are sized by `RAY_DATA_PARQUET_BIN_PACKING_BYTES`, which is also not derived from `DataContext.target_max_block_size`. Signed-off-by: Goutam <goutam@anyscale.com>
Tunes the Data release tests for the footer path, so this PR can be validated by an actual release run rather than by unit tests alone. Carried over from the tuning measured on the original branch (ray-project#64985), with the stale parts dropped: 7 of the 33 originally-pinned entries no longer exist (the autoscaling variants removed in ray-project#65506, plus tpch_q20 / tpch_q22), and the `{{scaling}}` de-templating is unnecessary because ray-project#65506 already left those entries spelled out. 26 entries get a pin. Most hold the previous 64 MiB behaviour on tests that regressed when the fallback default moved to 128 MiB; a few go the other way (1 GiB / 1.25 GiB) for datasets whose row groups are large enough that the default produces one read task per row group. Three entries are split out of their matrices because a matrix cannot vary `byod.runtime_env` per value and their variants want different budgets: `wide_schema_pipeline_{{data_type}}` into four, and one variant each out of `iceberg_benchmark_{{mode}}` and `iter_batches_{{format}}`. Rendered test names are unchanged -- 121 before and after -- so release baselines still line up. Any entry that gains its own `runtime_env` re-lists the four DEFAULTS guards verbatim: `deep_update` overwrites lists rather than merging them, so declaring one silently drops the OOM / dead-node / object-store checks. Also drops `map_benchmark.py`'s `use_datasource_v2 = False` pin, which was there because V2 was spilling; the footer path is the read path now. These budgets were measured against the pre-decoupling design. Packing moved between stages in ray-project#65596 but the packer and its inputs did not, so the bins should be unchanged -- treat these as a starting point to confirm or correct with a release run, not as settled values. Signed-off-by: Goutam <goutam@anyscale.com>
The pin carried over as `{{bin_packing_bytes}}`, but master's
`aggregate_groups` matrix defines only `shuffle_strategy` and `columns` --
the budget dimension was part of the older matrix shape. Left as-is the
variable never resolves and the test would run with a literal
`{{bin_packing_bytes}}` as its env value.
The `shuffle_v2` entry measured uniformly at 64 MiB, so it takes a plain
literal. The other entry needs three of its four combinations at 32 MiB and
one at 256 MiB, so the budget rides as a matrix dimension with explicit
adjustments. All six rendered combinations now resolve to a number.
Signed-off-by: Goutam <goutam@anyscale.com>
There was a problem hiding this comment.
Code Review
This pull request removes the legacy size-based Parquet chunking path (including ParquetFileChunker, ParquetFileChunkMetadata, and the NonSamplingFileIndexer fallback) in favor of the footer-based chunking path (FooterFileIndexer and OnlineBinPacker), making it the default and only path for Parquet V2 reads. This cleanup involves removing obsolete configuration options, unit tests, and environment variables, as well as tuning the release benchmarks for the new footer-based bin packing. A review comment suggests adding defensive checks in parquet_file_reader.py to prevent potential KeyError exceptions when accessing chunk_metadata keys.
| else: | ||
| fragments.extend( | ||
| _fragments_from_row_group_ids( | ||
| fragment, | ||
| chunk_metadata["row_group_ids"], | ||
| per_row_group_offsets=self._include_row_hash, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Defensive programming: Accessing 'chunk_metadata["row_group_ids"]' directly without checking if the key exists can raise a 'KeyError' if 'chunk_metadata' is not 'None' but does not contain 'row_group_ids' (e.g., if a custom or legacy chunk metadata format is encountered). Checking for 'row_group_ids' in 'chunk_metadata' and falling back to reading the whole fragment is safer and more robust.
| else: | |
| fragments.extend( | |
| _fragments_from_row_group_ids( | |
| fragment, | |
| chunk_metadata["row_group_ids"], | |
| per_row_group_offsets=self._include_row_hash, | |
| ) | |
| ) | |
| elif "row_group_ids" in chunk_metadata: | |
| fragments.extend( | |
| _fragments_from_row_group_ids( | |
| fragment, | |
| chunk_metadata["row_group_ids"], | |
| per_row_group_offsets=self._include_row_hash, | |
| ) | |
| ) | |
| else: | |
| fragments.append((fragment, 0)) |
| assert list(manifest.file_chunk_metadatas) == [None] | ||
| assert list(manifest.file_sizes) == [100] | ||
|
|
||
| def test_parquet_chunker_splits_large_file_into_many_chunks(self, tmp_path): |
There was a problem hiding this comment.
Do we test integration of the FileIndexer with our online bin packer as the file chunker? I wasn't able to find a test for this, and I think we should test for this end to end
We already unit test online bin packer
There was a problem hiding this comment.
Added test_footer_indexer_feeds_online_bin_packer
Signed-off-by: Goutam <goutam@anyscale.com>
Signed-off-by: Goutam <goutam@anyscale.com>
Why
The last step of the split of #64985: flip Parquet V2 to the footer read path, delete the blind one, and tune the release tests so this can be validated by a real release run.
Everything it depends on has landed — footer types and the bin packer (#65210), the
FooterReaderactor pool (#65273), the indexer pluscount()fix and packer-as-partitioner (#65596), reader IO tuning (#65806), and test env pinning (#65169). The path has been reachable behindRAY_DATA_PARQUET_ENABLE_FOOTER_INDEXERsince #65596; this makes it the only path.Flow
What changed
The flip —
RAY_DATA_PARQUET_ENABLE_FOOTER_INDEXERis gone from both call sites._get_file_indexer()always returnsFooterFileIndexer;get_file_partitioner()always returnsOnlineBinPacker.The deletions —
ParquetFileChunker,ParquetFileChunkMetadata,_calculate_row_group_range,_fragments_from_chunk_metadata, and the tests covering only that path.The config it left behind —
DataContext.parquet_chunker_target_chunk_sizeandDEFAULT_PARQUET_CHUNKER_TARGET_CHUNK_SIZEwere read only byParquetFileChunker.__init__, so they would otherwise stay declared and documented while doing nothing. Verified dead by grep across.py/.yaml/.rst/.md.Reader pool defaults —
RAY_DATA_PARQUET_READER_IO_THREAD_COUNT128,RAY_DATA_PARQUET_READER_CPU_COUNT32.Release tests tuned
Budgets are per-test overrides of
RAY_DATA_PARQUET_BIN_PACKING_BYTES(uncompressed bytes of row-group data per read task; default 128 MiB). The shape of the tuning: reads over few large groups want large bins, because small bins shatter the read into short tasks the executor cannot dispatch fast enough and the cluster idles; shuffle-bound reads over many groups want small bins.Rows whose variants all share the same tuning are collapsed and marked.
aggregate_groups_fixed_size_hash_shuffle_column02 column14aggregate_groups_fixed_size_hash_shuffle_column08 column13 column14aggregate_groups_fixed_size_shuffle_v2_column02 column14aggregate_groups_fixed_size_shuffle_v2_column08 column13 column14aggregate_groups_fixed_size_sort_shuffle_pull_based_column02 column14aggregate_groups_fixed_size_sort_shuffle_pull_based_column08 column13 column14count_parquet_fixed_sizecross_az_map_batches_autoscaling_iptable_failure_injectiondistributed_training20flat_mapiceberg_benchmark_overwriteiter_batches_pyarrowiter_torch_batchesjoins_{{dataset}}_{{join_type}} *(all 4 variants)*mapmap_batches_fixed_size_{{compute}}_{{format}}_{{repeat_map_batches}} *(all 7 variants)*0.6map_groups_fixed_size_{{shuffle_strategy}}_{{columns}} *(all 6 variants)*mix4random_shuffle_fixed_sizeread_large_parquet_fixed_sizeread_parquet_fixed_sizeread_tfrecordsstreaming_split50, result batch50to_tftpch_q1_fixed_size_{{shuffle_strategy}} *(all 2 variants)*training_ingest_benchmarkwide_schema_pipeline_nested_structs1, footer actors21, io threads5000wide_schema_pipeline_objects1, footer actors9, io threads5000wide_schema_pipeline_primitives1, footer actors27, io threads5000wide_schema_pipeline_tensors1, footer actors22, io threads5000write_parquetThree matrices are split, because a matrix cannot vary
byod.runtime_envper value and their variants want different budgets:wide_schema_pipeline_{{data_type}}into four entries, and one variant each out oficeberg_benchmark_{{mode}}anditer_batches_{{format}}.aggregate_groupskeeps its matrix and carries the budget as a matrix dimension with explicit adjustments. Rendered test names are unchanged — 121 before and after — so release baselines still line up.Not carried over from the original branch: 7 previously-pinned entries no longer exist — the autoscaling variants removed in #65506, plus
tpch_q20andtpch_q22.Any entry gaining its own
runtime_envre-lists the fourDEFAULTSguards verbatim:deep_updateoverwrites lists rather than merging them, so declaring one silently drops the OOM / dead-node / object-store checks. (17 entries on master are already missing guards for this reason; pre-existing, not touched here.)map_benchmark.pyalso drops itsuse_datasource_v2 = Falsepin, which existed because V2 was spilling.Behavior change worth reviewing
Read-task sizing no longer comes from
override_num_blocks/read_op_min_num_blocksfor Parquet. Bins are sized byRAY_DATA_PARQUET_BIN_PACKING_BYTES, which is also not derived fromDataContext.target_max_block_size. That decoupling is deliberate — read units are row-group-accurate rather than size-estimated — but it means the two knobs users reach for first no longer affect Parquet read parallelism. Deriving the budget fromtarget_max_block_sizeis worth considering as a follow-up; the per-test overrides above are the evidence that a single global default does not fit every workload.Testing
The 286-test parquet suite is the one that matters — it runs the full read surface with the footer path as the default rather than behind a flag. The release YAML is additionally checked for parse validity, unresolved
{{...}}in any pin, duplicatebyod:keys, and rendered-name stability.Stack
Final step of an 11-step split of #64985 (43 files / +3234 −719, not reviewable as one unit).
[1]· [Data] [2/11] Push limit into ReadFiles when Limit sits directly on it #65168[2]· [Data] [3/11] Derive ListFiles pushdown state from the ReadFiles scanner #65214[3]· [Data] [4/11] Add Parquet footer types and the online bin packer #65210[4]· [Data] [5/11] Add the Parquet FooterReader actor pool #65273[5]· [Data] [6/11] Add FooterFileIndexer behind an opt-in flag, with bin packing as a partitioner #65596[6]· [Data] [8/11] Tune Parquet reader IO threads and Arrow pool sizing #65806[8]· [Data] [9/11] Pin the Parquet footer actor pool to 1 in tests #65169[9]— all merged[7](count push-down fix) was absorbed into [Data] [6/11] Add FooterFileIndexer behind an opt-in flag, with bin packing as a partitioner #65596[10](release tuning) is folded into this PR, so a release run exercises the flip and its tuning together[11/11]Not a duplicate. File-level overlap check: #63158 none; #65142 overlaps
file_indexer.py/context.py; #65406 / #65407 (arrow-rs reader A/B) overlap broadly but are explicitly[DO NOT MERGE]benchmark branches carrying the footer work out-of-tree.AI assistance was used; every line reviewed by me and tests run locally.