zhuqi-lucas commented on issue #25572: URL: https://github.com/apache/datafusion/issues/25572#issuecomment-5770261452
Thanks @alamb! **On the compatibility options**: both work for the producer direction, and the deprecated shim (option 1) is what would keep a downstream chain like ours running unmodified through the migration. One clarification though, because the case I was describing is the opposite direction: not "my pass leaves the plan invalid, who cleans up", but **"my pass only finds work on an enforced plan"**. Think of a rule that eliminates enforced sorts by reorganizing what runs beneath them: it pattern-matches the very operators enforcement materializes, produces nothing invalid itself, and simply finds no work at all if it runs before enforcement. "Any pass that makes an invalid plan should run EnforceRequirements itself" places the duty on producers, and consumers have no invalid plan to answer for. For that class I only see three shapes: a sanctioned `enforce → optimize → enforce` alternation, a convention that optimizer rules read requirements rather than materialized operators (which is a real rewrite of such rules, we have one and it is not small), or the consumer running enforcement itself, which re-derives requirements once per consumer and is the cost this whole effort wants to remove. **On `check_invariants`**: I think this is the piece that makes the enforcement phase cheap, and maybe its second life. An enforcement pass that finds nothing to do still pays a full-tree requirement re-derivation today, which is most of what our six passes spend (4 of 6 are byte-identical no-ops). If the enforcement phase is gated on `check_invariants` (invariants hold, skip the pass entirely), the no-op case drops from "re-derive everything" to one linear walk, and it is a semantic check rather than the textual fingerprint #25356 had to use for the same purpose. That would also give the Invariant API the adoption it did not get the first time: the phase boundary is a natural, always-on call site. It likely needs the invariant coverage audited first (distribution and ordering requirements would have to be fully expressible as invariants), which could be its own preparatory issue. **On sequencing**: since the cycle-detection shape is agreed, I would propose implementing in three steps so each lands reviewable: 1. `PhysicalPlanSignature` as a type plus its boundary tests (what identity sees and deliberately does not see: partitioning/ordering/equivalences in, statistics out; the tests from #25356 port over wholesale); 2. the convergence loop over the existing optimizer list behind a config, using the signature `HashSet`; 3. the trait split with the deprecated shims from your option 1, plus the pass-contract documentation from your option 2. Happy to start with step 1 unless you would rather see the trait split first. -- 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]
