Phoenix500526 opened a new pull request, #23201:
URL: https://github.com/apache/datafusion/pull/23201
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases. You can
link an issue to this PR using the GitHub syntax. For example `Closes #123`
indicates that this PR will close issue #123.
-->
- Closes #23072.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in
the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your
changes and offer better suggestions for fixes.
-->
File statistics are computed against a specific `file_schema` (their
`column_statistics` are positional, one per column), but the file-statistics
cache was keyed only by table and path. Reading the same path under a
different
schema could therefore reuse statistics whose columns no longer line up,
panicking during statistics projection.
#22950 worked around this by **bypassing** the file-statistics cache
entirely for
anonymous explicit-schema reads — correct, but it gave up cache reuse for
them
(every such read recomputes statistics). #23072 asks to make the cache itself
schema-aware so those reads can reuse the cache safely instead of skipping
it.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is
sometimes worth providing a summary of the individual changes in this PR.
-->
- Add `SchemaFingerprint` — the per-column `(name, data_type, nullable)` of a
`file_schema`, in order — and `FileStatisticsCacheKey { table, path,
schema }`,
and key the file-statistics cache on it (`FileStatisticsCache` is now
`dyn Cache<FileStatisticsCacheKey, CachedFileMetadata>`).
- `ListingTable::do_collect_statistics_and_ordering` builds the key with the
`file_schema` fingerprint and uses the shared cache directly. The #22950
bypass (`statistics_cache` helper / `schema_source`-based skip) is removed:
different schemas now land in distinct entries (no stale cross-schema
reuse),
while a repeated read of the same schema reuses its entry.
- The fingerprint deliberately **excludes** field/schema metadata (it cannot
affect statistics, and including it would needlessly fragment the cache)
and
partition columns (partition statistics are computed separately, outside
this
cache).
- Table-drop invalidation is unchanged: `drop_table_entries` matches on
`CacheKey::table_ref()`, which still returns the table, so all schema
variants
for a dropped table are removed together.
- The list-files cache continues to key on `TableScopedPath`.
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are
they covered by existing tests)?
-->
Yes.
- Updated the #22950 regression test
(`anonymous_parquet_stats_cache_with_explicit_wider_schema`): the wider
explicit-schema read now lands in its own cache entry (2 entries, was 1
under
the bypass) with correct statistics and no panic, and a repeated read of
that
schema is served from the cache (a cache hit, no new entry).
- Added unit tests for `SchemaFingerprint`: it distinguishes nullability and
field order, and ignores field/schema metadata.
- `cargo test` for the `file_statistics` integration module and the
`datafusion-execution` cache tests (including `drop_table_entries`) pass,
along
with `cargo fmt --all` and `cargo clippy --all-targets --all-features
-- -D warnings` for the touched crates.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
No change to query results, physical plans, or the serialized (proto) wire
format; file statistics are computed exactly as before.
One public API change (please add the `api change` label): the
`FileStatisticsCache` type alias now uses `FileStatisticsCacheKey` instead of
`TableScopedPath` as its key. Code that constructed keys for this cache
directly
must switch to `FileStatisticsCacheKey`. `SchemaFingerprint` and
`FileStatisticsCacheKey` are newly public; `TableScopedPath` remains (still
used
by the list-files cache). `cargo-semver-checks` will flag the key-type
change,
which is expected.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
