apache / apache/datafusion

StatisticsContext: make the pointer-keyed cache robust against node reuse when shared across a plan-rewriting pass

Open
#25,141 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

Follow-up to #25098 (cc @asolimando).

## Background

`StatisticsContext`'s memoization cache is keyed by the raw `ExecutionPlan` node pointer address. This is safe for the common usage — a fresh context computing statistics over a single, stable plan tree.

It becomes subtle when one context is **shared across a physical-optimizer pass that rewrites the plan** (e.g. `EnsureRequirements` in #25098 reuses one context across its `ensure_distribution` `transform_up`). During such a pass a node can be freed when it is replaced, and a later allocation could reuse that address, so a stale cache entry could be returned for a different node (an ABA hazard).

## Current mitigation (in #25098)

The consumer resets the cache after any node whose plan pointer actually changed (`Arc::ptr_eq` before/after). This is safe, but it puts the burden on every consumer that shares a context across a rewrite.

## Proposal

Make the cache robust so consumers do not have to think about this, per @asolimando's suggestions:

- **(a)** Give each constructed `ExecutionPlan` a unique id and key the cache on it instead of the pointer. Cleanest long-term, but touches every plan node.
- **(b)** Have the cache keep the nodes alive for its lifetime (hold an `Arc` clone of each cached node) so an address cannot be reused. Smaller in spirit, but `compute` currently takes `&dyn ExecutionPlan` and is called from ~10 sites, so threading an `Arc` through would be a wider API change.

Either removes the consumer-side reset. Filing so the robustness work is tracked separately from #25098 (which keeps the safe reset).

Contributor guide

Open the contributing guide

Research direction

Start with StatisticsContext's compute entry point and trace its use from EnsureRequirements through ensure_distribution and transform_up. Compare the proposed unique-id and Arc-retention approaches, including the roughly 10 compute call sites; done means the cache remains safe across plan rewrites without consumer-side resets.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
databases
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.