Repository navigation
Conversation
This was referenced Jun 8, 2026
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Building an `ArrowReaderMetadata` walks every leaf of the parquet schema to produce the arrow `Schema` + dremel field levels. For wide-schema files this O(N_columns) walk runs on every file open, once per query, even though the result is identical across queries for the same file. Cache it on `CachedParquetMetaData` (which already lives in the file metadata cache): - `arrow_reader_metadata()` lazily builds the base `ArrowReaderMetadata` via `parquet_to_arrow_schema_and_field_levels` + `ArrowReaderMetadata::from_field_levels` and memoises it in a `OnceLock`; warm hits are a cheap `Arc`-bump clone. - `coerced_arrow_reader_metadata()` memoises a single post-coercion build keyed by the supplied schema's `Arc` identity (the common case: every file coerces to the same table schema). - `CachedParquetFileReader` overrides `AsyncFileReader::get_arrow_reader_metadata` to serve both from cache, falling back to a fetch + `try_new` on miss. Depends on the arrow-rs primitives in apache/arrow-rs#9882 (`from_field_levels`, `parquet_to_arrow_schema_and_field_levels`, `ArrowReaderOptions` accessors, `AsyncFileReader::get_arrow_reader_metadata`). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This PR uses parquet API that only exists on apache/arrow-rs#9882, which has not merged and so is in no parquet release. Without a patch the workspace does not compile and CI cannot say anything useful about the change. The patch points at a throwaway branch that is the parquet 59.2.0 release tag plus apache#9882's four commits — deliberately not arrow-rs `main`, which has post-release changes (notably `arrow_schema::Metadata` replacing `HashMap<String, String>`) that DataFusion has not absorbed and which break `datafusion-common` before parquet is even reached. Revert this commit once apache#9882 has merged and DataFusion has bumped to a parquet release containing it. Green CI on this PR means "the change works against that API", not "this is mergeable". Co-Authored-By: Claude Opus 5 <[email protected]>
adriangb
force-pushed
the
adrian/wide-schema-arrow-metadata-cache
branch
from
August 9, 2026 12:42
277613a to
0bf8ca6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22830 +/- ##
==========================================
- Coverage 81.06% 81.05% -0.02%
==========================================
Files 1106 1106
Lines 381891 381987 +96
Branches 381891 381987 +96
==========================================
+ Hits 309578 309612 +34
- Misses 54034 54090 +56
- Partials 18279 18285 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
run benchmark wide_schema |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adrian/wide-schema-arrow-metadata-cache (0bf8ca6) to 918013e (merge-base) diff Run configurationrun benchmark wide_schemaResults will be posted here when complete File an issue against this benchmark runner |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
adriangb
added a commit
to pydantic/datafusion
that referenced
this pull request
Oct 16, 2026
…on (apache#26355) ## Which issue does this PR close? - Part of apache#21968. This revives apache#21987 by @adriangb, which was closed while stale and never reviewed. The two commits are his, rebased onto current `main`. The fix was never carried into apache#22829 or apache#22830, and the quadratic lookup is still on `main`. ## Rationale for this change Planning a query against a wide-schema Parquet table takes time quadratic in the number of columns, even when the query reads only one or two of them. `DFParquetMetadata::statistics_from_parquet_metadata` resolves every field in the logical file schema to a Parquet leaf column. Both branches resolve each field with an O(N) lookup: - When the file has statistics, it calls `StatisticsConverter::try_new`. That does a linear `Schema::column_with_name`, then calls `parquet_column`, which does a linear `Fields::find` and a linear scan for the first leaf under the matched root. - When the file has no statistics, it calls `parquet_column` directly. This happens once per field, so the cost is O(N²) per file, on every query plan, for every file in the table. ## What changes are included in this PR? - A new crate-private `LeafResolver` (`datafusion/datasource-parquet/src/leaf_resolver.rs`). It builds two maps once per file: Arrow field name to root index, and Parquet root index to first leaf index. Both call sites use it in place of the per-field linear lookups, which makes the statistics pass O(N) per file. - `LeafResolver` gives the same results as `parquet_column`, including its tie-breaks: the first Arrow field with a given name wins, and the lowest leaf index for a root wins. When a leaf exists, it builds the converter with `StatisticsConverter::from_column_index`, which yields the same converter `try_new` would (same leaf index, same Arrow field, same physical type). - A field with no Parquet leaf (a nested type, or a name missing from the file schema) falls back to `StatisticsConverter::try_new`. This reproduces its exact error for an unknown column, and it is needed because `from_column_index` cannot express "this column has no Parquet leaf". - The `parquet_metadata_statistics` benchmark now also covers 1024 columns, because the quadratic term only shows up past a few hundred columns. Rebasing onto `main` conflicted in `statistics_from_parquet_metadata`. The resolution keeps every change made on `main` since apache#21987, including the omitted-null-count handling and the untrusted min/max order checks, and changes only how each field's converter is built. **Known limitation:** schemas with many nested fields still take the O(N) fallback for those fields. apache/arrow-rs#9882 proposes a constructor that takes an `Option<usize>` leaf, which would remove the fallback. Flat wide schemas never take it. ## What is the testing strategy for this PR? - Seven unit tests in `leaf_resolver.rs`. Five of them check, for every field name plus some absent names, that `LeafResolver::leaf_index` returns exactly what `parquet_column` returns and that `LeafResolver::converter` matches `StatisticsConverter::try_new` (same leaf index, same Arrow field, same error text). The tie-break rules are the risky part: getting either backwards would silently attribute one column's statistics to another, and nothing in the existing suite covers duplicate names. The other two check specific leaf indices, and that a duplicated name uses the first field's type. The schemas cover: - primitives, a nested struct, a nested list, and a duplicate field name with a different type from the first - a primitive Arrow field over a multi-leaf Parquet group (lowest leaf wins) - a Parquet root with no leaves (an empty group) - more Arrow fields than Parquet roots - an empty schema - No `.slt` test: this changes how columns are looked up, not the statistics produced, so the unit tests compare directly against `parquet_column` and `try_new`. - Existing suites: `-p datafusion-datasource-parquet --lib`, `-p datafusion --test parquet_integration`, and the `parquet_statistics`, `listing_table_statistics`, `parquet_sorted_statistics` and `parquet_nested_schema_pruning` sqllogictest files. ### Benchmarks `wide_schema` (1024 vs 8 columns × 256 files), run locally on an AMD Ryzen 9 9950X3D. These are Criterion medians, `main` (e732212) against this branch: | Query | main | this PR | change | |---|---:|---:|---:| | Q01 wide | 694.5 ms | 522.9 ms | −24.7% | | Q02 wide | 528.0 ms | 350.3 ms | −33.7% | | Q03 wide | 529.2 ms | 353.4 ms | −33.2% | | Q04 wide | 617.2 ms | 445.5 ms | −27.8% | | Q01–Q04 narrow | — | — | within ±0.5% | For every wide query the confidence intervals do not overlap. The two sides ran one after the other on the same machine, not interleaved. ## Are there any user-facing changes? No. `LeafResolver` is crate-private and the statistics produced are unchanged. Planning against wide-schema Parquet tables is faster. --- This PR was prepared with AI assistance (Claude Code). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Adrian Garcia Badaracco <[email protected]> Co-authored-by: Claude Opus 5 <[email protected]>
alamb
pushed a commit
to rxdrccq/datafusion
that referenced
this pull request
Oct 16, 2026
…on (apache#26355) ## Which issue does this PR close? - Part of apache#21968. This revives apache#21987 by @adriangb, which was closed while stale and never reviewed. The two commits are his, rebased onto current `main`. The fix was never carried into apache#22829 or apache#22830, and the quadratic lookup is still on `main`. ## Rationale for this change Planning a query against a wide-schema Parquet table takes time quadratic in the number of columns, even when the query reads only one or two of them. `DFParquetMetadata::statistics_from_parquet_metadata` resolves every field in the logical file schema to a Parquet leaf column. Both branches resolve each field with an O(N) lookup: - When the file has statistics, it calls `StatisticsConverter::try_new`. That does a linear `Schema::column_with_name`, then calls `parquet_column`, which does a linear `Fields::find` and a linear scan for the first leaf under the matched root. - When the file has no statistics, it calls `parquet_column` directly. This happens once per field, so the cost is O(N²) per file, on every query plan, for every file in the table. ## What changes are included in this PR? - A new crate-private `LeafResolver` (`datafusion/datasource-parquet/src/leaf_resolver.rs`). It builds two maps once per file: Arrow field name to root index, and Parquet root index to first leaf index. Both call sites use it in place of the per-field linear lookups, which makes the statistics pass O(N) per file. - `LeafResolver` gives the same results as `parquet_column`, including its tie-breaks: the first Arrow field with a given name wins, and the lowest leaf index for a root wins. When a leaf exists, it builds the converter with `StatisticsConverter::from_column_index`, which yields the same converter `try_new` would (same leaf index, same Arrow field, same physical type). - A field with no Parquet leaf (a nested type, or a name missing from the file schema) falls back to `StatisticsConverter::try_new`. This reproduces its exact error for an unknown column, and it is needed because `from_column_index` cannot express "this column has no Parquet leaf". - The `parquet_metadata_statistics` benchmark now also covers 1024 columns, because the quadratic term only shows up past a few hundred columns. Rebasing onto `main` conflicted in `statistics_from_parquet_metadata`. The resolution keeps every change made on `main` since apache#21987, including the omitted-null-count handling and the untrusted min/max order checks, and changes only how each field's converter is built. **Known limitation:** schemas with many nested fields still take the O(N) fallback for those fields. apache/arrow-rs#9882 proposes a constructor that takes an `Option<usize>` leaf, which would remove the fallback. Flat wide schemas never take it. ## What is the testing strategy for this PR? - Seven unit tests in `leaf_resolver.rs`. Five of them check, for every field name plus some absent names, that `LeafResolver::leaf_index` returns exactly what `parquet_column` returns and that `LeafResolver::converter` matches `StatisticsConverter::try_new` (same leaf index, same Arrow field, same error text). The tie-break rules are the risky part: getting either backwards would silently attribute one column's statistics to another, and nothing in the existing suite covers duplicate names. The other two check specific leaf indices, and that a duplicated name uses the first field's type. The schemas cover: - primitives, a nested struct, a nested list, and a duplicate field name with a different type from the first - a primitive Arrow field over a multi-leaf Parquet group (lowest leaf wins) - a Parquet root with no leaves (an empty group) - more Arrow fields than Parquet roots - an empty schema - No `.slt` test: this changes how columns are looked up, not the statistics produced, so the unit tests compare directly against `parquet_column` and `try_new`. - Existing suites: `-p datafusion-datasource-parquet --lib`, `-p datafusion --test parquet_integration`, and the `parquet_statistics`, `listing_table_statistics`, `parquet_sorted_statistics` and `parquet_nested_schema_pruning` sqllogictest files. ### Benchmarks `wide_schema` (1024 vs 8 columns × 256 files), run locally on an AMD Ryzen 9 9950X3D. These are Criterion medians, `main` (e732212) against this branch: | Query | main | this PR | change | |---|---:|---:|---:| | Q01 wide | 694.5 ms | 522.9 ms | −24.7% | | Q02 wide | 528.0 ms | 350.3 ms | −33.7% | | Q03 wide | 529.2 ms | 353.4 ms | −33.2% | | Q04 wide | 617.2 ms | 445.5 ms | −27.8% | | Q01–Q04 narrow | — | — | within ±0.5% | For every wide query the confidence intervals do not overlap. The two sides ran one after the other on the same machine, not interleaved. ## Are there any user-facing changes? No. `LeafResolver` is crate-private and the statistics produced are unchanged. Planning against wide-schema Parquet tables is faster. --- This PR was prepared with AI assistance (Claude Code). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Adrian Garcia Badaracco <[email protected]> Co-authored-by: Claude Opus 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Building an
ArrowReaderMetadatawalks every leaf of the parquet schema to produce the arrowSchemaand dremel field levels. For wide-schema files thisO(N_columns)walk runs on every file open, once per query, even though the result is identical across queries for the same file.For scale: at 1024 columns the footer decode is ~385 µs/file, of which ~147 µs (38%) is the schema walk this PR caches.
What changes are included in this PR?
Cache the built
ArrowReaderMetadataonCachedParquetMetaData, which already lives in the file metadata cache:arrow_reader_metadata()lazily builds the base metadata viaparquet_to_arrow_schema_and_field_levels+from_field_levelsand memoises it in aOnceLock; warm hits are a cheapArc-bump clone.coerced_arrow_reader_metadata()memoises a single post-coercion build, keyed by the supplied schema'sArcidentity.CachedParquetFileReaderoverridesAsyncFileReader::get_arrow_reader_metadatato serve both from cache.Dependencies, and why the patch points where it does
This needs the parquet primitives in apache/arrow-rs#9882 (
from_field_levels,parquet_to_arrow_schema_and_field_levels, theArrowReaderOptionsaccessors, andAsyncFileReader::get_arrow_reader_metadata). That PR is open and unmerged, so the API is in no parquet release and this PR cannot compile against crates.io.The temporary patch therefore points at a fork branch that is the parquet 59.2.0 release tag plus #9882's four commits. Deliberately not arrow-rs
main:maincarries post-release changes — notablyarrow_schema::MetadatareplacingHashMap<String, String>, plus a newarrow-cmpcrate — that DataFusion has not absorbed, and which fail to compile indatafusion-commonbefore parquet is even reached.To merge: revert the
TEMPORARY:commit once #9882 has merged and DataFusion has bumped to a parquet release containing it.The arrow-rs-independent pieces of the #21968 investigation are split out separately: #22829 (
memory_sizecaching, coercion early-return) and #21987 (O(1) parquet leaf lookup in statistics collection, which needs no arrow-rs change).Are these changes tested?
Yes, now that it builds. Existing suites pass against the patched dependency:
-p datafusion-datasource-parquet --lib(194),-p datafusion --test parquet_integration(218),-p datafusion --test core_integration(1064)../ci/scripts/rust_clippy.shandcargo fmt --checkare clean.Rebasing onto current
mainsurfaced a genuine bug in the original commit, now fixed:get_arrow_reader_metadatafetched metadata without propagating the page-index policy fromoptions, so a caller that asked to skip the page index got it fetched anyway and the cached entry was polluted with it.test_page_index_skipped_with_cached_reader_factorycatches this. It matters more than it sounds on the target workload — at 1024 columns the page index is 424 KiB/file and ~37% of total metadata decode.No new tests are added for the caching itself; that gap is worth closing before this leaves draft.
Are there any user-facing changes?
No public API changes —
CachedParquetMetaDatagains internal caching fields and methods.🤖 Generated with Claude Code