zhuqi-lucas commented on code in PR #26094:
URL: https://github.com/apache/datafusion/pull/26094#discussion_r4219786894
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -3123,9 +3138,7 @@ impl DefaultPhysicalPlanner {
InvariantChecker(InvariantLevel::Always).check(&plan)?;
let mut new_plan = Arc::clone(&plan);
- let optimizer_context = SessionOptimizerContext {
- session: session_state,
- };
+ let optimizer_context = SessionOptimizerContext::new(session_state);
Review Comment:
One more thing this widens, worth stating somewhere: the cache is now
exposed to in-place mutation for the whole planning run, not just one rule.
A pointer-keyed hit is only valid because a plan node never changes behind
its `Arc`. That held trivially when each rule had its own context — anything a
rule did produced new nodes. Now an entry computed by the first rule is still
served to the last one, so a node that mutated its own statistics through
interior mutability without changing identity would be read stale.
Nothing does that today (planning-time rules all rebuild nodes, and dynamic
filters are updated at execution time, after this context is dropped), so this
isn't a bug — but it's an invariant the design now leans on much harder. A line
on `StatisticsContext` saying cached nodes must be immutable for the context's
lifetime would make it checkable in review.
--
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]
