alamb commented on issue #25572: URL: https://github.com/apache/datafusion/issues/25572#issuecomment-5766183132
> ### Design questions to settle > * **Phase topology.** Is the shape a fixed `optimize-loop → enforce`, or can a chain declare an alternation? Some downstream rules consume the operators enforcement materializes (for example, removing an enforced sort by reorganizing what runs beneath it) rather than the abstract requirement, and today they are deliberately scheduled after an enforcement pass. Either the split sanctions an `enforce → optimize → enforce` alternation, or it establishes the convention that optimizer rules read requirements, never materialized operators. > * **Classification of the built-in rules** into the two traits, and the compatibility story for downstream chains that currently splice enforcement rules in by position. I think this is saying that some downstream crates might rely on the fact they can add EnforceDistribution into their chain of operations and it would run. I think we can handle this a few ways: 1. Leave a (deprecated) EnforceDistributon `OptimizerRule` in place that calls into the new EnforceRequirementsAnalyzer rule so they can keep using it for a while 1. Document what each pass should be doing / what rules it needs to follow to get the ExecutionPlans to work. For example, if downstream users currently splice in EnforceRequirements to make sure the output of their passes is valid, we should document that any pass that makes an invalid plan should run EnforceRequirements again itself (or not make the invalid plan) @wiedld and I tried to add something like this with the Invariant API https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html#method.check_invariants that could tell you if some invariant isn't being followed. However I don't think that idea got very far and is still only used a little > * **Cycle detection, not just "unchanged since last pass".** We have a measured case where an enforcement rule oscillates: two adjacent passes each undo the other's change, so the plan alternates between two forms without stabilizing. The logical loop already handles this by keeping a `HashSet` of every prior pass signature and stopping on first revisit; the physical loop should copy that, not compare only against the previous pass. I agree this sounds like a goodplan -- 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]
