Skip to content

Speed up snapshot_by_id with an index, ~4.5x faster inspect.partitions() - #3725

Open
MatheusFreitas25 wants to merge 2 commits into
apache:mainfrom
MatheusFreitas25:index-snapshot-by-id
Open

Speed up snapshot_by_id with an index, ~4.5x faster inspect.partitions()#3725
MatheusFreitas25 wants to merge 2 commits into
apache:mainfrom
MatheusFreitas25:index-snapshot-by-id

Conversation

@MatheusFreitas25

Copy link
Copy Markdown

Rationale for this change

snapshot_by_id does a linear scan over self.snapshots. It is called once per
manifest entry by InspectTable._process_manifest, so inspect.partitions() ends
up being O(data_files × snapshots) — the cost grows with the snapshot count, which
means every commit makes all later reads slower.

On a table with ~317k data files and ~14.5k snapshots, reading the partitions of a
single manifest went from 1.70s to 0.38s (~4.5x) with this change. A lookup-only
benchmark of 20k calls shows where the time goes:

This replaces the scan with a memoized id-to-snapshot index.

A plain cached_property is not usable here: model_copy carries __dict__ over
to the new instance, so a copy that replaces snapshots would inherit a stale
index and fail to find newly added snapshots. Since the commit path does exactly
that (snapshot_by_id on the base metadata, then model_copy(update={"snapshots": ...})), the index is tied to the list it was built from and recomputed whenever
snapshots is a different list.

This follows the same pattern already used for Schema._lazy_id_to_field.

Are these changes tested?

Three tests added to tests/table/test_metadata.py, using the existing
example_table_metadata_v2 fixture

Are there any user-facing changes?

No. snapshot_by_id returns the same Snapshot instance and None for unknown
ids, exactly as before. No signature or API change.

snapshot_by_id did a linear scan over the snapshots list, and is called once
per manifest entry, making inspect.partitions() O(data_files x snapshots).

Memoize an id-to-snapshot index instead. A cached_property is not usable
here: model_copy carries __dict__ over, so a copy replacing the snapshots
would inherit a stale index. The index is tied to the list it was built from
and recomputed whenever snapshots is a different list.
@ebyhr

ebyhr commented Aug 1, 2026

Copy link
Copy Markdown
Member

Could you confirm the CI failure?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants