adriangb opened a new pull request, #10880:
URL: https://github.com/apache/arrow-rs/pull/10880
# Which issue does this PR close?
- Contributes to #9722.
- Builds on #10878, which adds the `repeated_batches` benchmark cases used
below.
# Rationale for this change
`WriterProperties` keeps per-column overrides in a
`HashMap<ColumnPath, ColumnProperties>`. Every accessor that takes a
`&ColumnPath` — `compression`, `encoding`, `dictionary_enabled`,
`statistics_enabled`, `write_page_header_statistics`,
`column_data_page_size_limit`, `column_dictionary_page_size_limit`,
`column_data_page_v2_compression_ratio_threshold` and
`bloom_filter_properties` — hashes that path and searches the map.
`ColumnPath`
is a `Vec<String>`, so each search hashes every part of the path and then
compares strings on a hit.
The column writers call those accessors repeatedly for the same leaf column:
- 6 times while a column writer is constructed (`GenericColumnWriter::new`
plus
`ColumnValueEncoder::try_new`);
- 2 more on every `write`, in `ByteBudgetChunker::new`;
- 1 per mini-batch in `should_add_data_page`, and another in
`should_dict_fallback` when a dictionary encoder is active;
- 1 per page in `add_data_page`, plus one more for Data Page v2.
`WriterProperties` is immutable once built, so all of that recomputes an
answer
that was already available when the column writer was created.
Counting the searches directly (temporary instrumentation on the map access)
over
the `writer_overhead` shapes:
| benchmark case | searches before | searches after |
| --- | --- | --- |
| `writer_overhead/1000_cols` | 10,000 | 1,000 |
| `writer_overhead/5000_cols` | 50,000 | 5,000 |
| `writer_overhead/10000_cols` | 100,000 | 10,000 |
| `writer_overhead/1000_cols/repeated_batches` | 103,000 | 1,000 |
| `writer_overhead/5000_cols/repeated_batches` | 515,000 | 5,000 |
After the change it is exactly one search per leaf column per row group.
# What changes are included in this PR?
- Adds `WriterProperties::resolve_column_properties`, which searches the
per-column map once and returns a `ResolvedColumnProperties` holding every
per-column setting already resolved against the file-wide defaults. The
struct
is `pub(crate)`; no public API is added.
- `GenericColumnWriter::new` calls it once and stores the result. The
per-batch
and per-page paths, `ByteBudgetChunker::new`, and both `ColumnValueEncoder`
implementations read from it instead of searching the map again.
`ColumnValueEncoder::try_new` and `create_bloom_filter` now take the
resolved
settings; that trait is not nameable outside the crate.
- The existing per-column accessors keep their behaviour and are
reimplemented on
top of the same resolution helpers, so the "column override, else file
default,
else constant" rule has one definition rather than one per accessor.
# Are these changes tested?
Yes.
- The existing suite passes unchanged, with and without `encryption`:
`cargo test -p parquet --features "arrow async encryption test_common
experimental"`.
- New `test_resolve_column_properties_matches_individual_accessors` asserts
the
one-pass resolution agrees with every individual accessor, for a column
with
overrides, a column that inherits the file defaults, and properties left
entirely at their defaults.
- To check the writer is byte-for-byte unchanged I wrote the same file from a
build before and after this change, with a workload covering the settings
this
PR touches: per-column compression, encoding, dictionary on and off, page
and
dictionary page size limits, page-header statistics, a bloom filter,
dictionary
fallback on multi-KB values, and both writer versions. The two files are
identical.
Benchmark:
cargo bench -p parquet --bench writer_overhead
I did not have an idle machine, and criterion's wall-clock intervals were too
wide there to be usable. I measured CPU time (user + sys) per iteration
instead,
which is far less sensitive to competing load: the minimum over 40
alternating
runs of the two builds, with a separately measured ~13.3 ms of fixed
benchmark
setup subtracted.
| benchmark case | before | after | |
| --- | --- | --- | --- |
| `writer_overhead/1000_cols` | 2.7 ms | 2.1 ms | see note |
| `writer_overhead/5000_cols` | 20.2 ms | 18.5 ms | −8 % |
| `writer_overhead/10000_cols` | 38.8 ms | 34.2 ms | −12 % |
| `writer_overhead/1000_cols/repeated_batches` | 12.9 ms | 8.6 ms | −34 % |
| `writer_overhead/5000_cols/repeated_batches` | 88.0 ms | 70.1 ms | −20 % |
Note: at 1,000 columns with a single batch the remaining figure is close
enough
to the spread of the setup subtraction that I would not read a percentage
into
it. The other four imply about 40 ns per map search, consistently across all
four
shapes, which matches the search counts above. A run on an idle machine
would be
worth having, and the benchmark from #10878 makes that easy.
`ColumnWriterImpl<Int32Type>` grows from 1312 to 1376 bytes to hold the
resolved
settings. Peak RSS on the 10,000 column benchmark was unchanged.
# Are there any user-facing changes?
No. No public API is added, removed or changed, and the bytes written are
unchanged.
# AI usage
This PR was written with Claude Code and reviewed by a human. The
byte-for-byte
output comparison and the map-search counts above are the checks run to
confirm
the behaviour is unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
