kosiew commented on code in PR #24716:
URL: https://github.com/apache/datafusion/pull/24716#discussion_r3886068562
##########
datafusion/physical-optimizer/src/join_selection.rs:
##########
@@ -152,19 +151,15 @@ impl PhysicalOptimizerRule for JoinSelection {
plan: Arc<dyn ExecutionPlan>,
context: &dyn PhysicalOptimizerContext,
) -> Result<Arc<dyn ExecutionPlan>> {
- let config = context.config_options();
- let registry = context.statistics_registry();
let subrules: Vec<Box<PipelineFixerSubrule>> = vec![
Box::new(hash_join_convert_symmetric_subrule),
Box::new(hash_join_swap_subrule),
];
let new_plan = plan
- .transform_up(|p| apply_subrules(p, &subrules, config))
+ .transform_up(|p| apply_subrules(p, &subrules,
context.config_options()))
.data()?;
new_plan
- .transform_up(|plan| {
- statistical_join_selection_subrule(plan, config, registry)
- })
+ .transform_up(|plan| statistical_join_selection_subrule(plan,
context))
Review Comment:
Could we add a focused test that calls
`JoinSelection::optimize_with_context` with a custom context and statistics
registry where the estimates actually change the join decision? The existing
registry SLT covers this through the `SessionState` path, but a test here would
directly exercise the context-threading boundary introduced by this refactor.
Not blocking, but I think it would be useful coverage.
--
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]
