zhuqi-lucas commented on PR #25688: URL: https://github.com/apache/datafusion/pull/25688#issuecomment-6017106094
Thank you @alamb and @2010YOUY01. One observation from reading #26069 against the code: its "only shape 1 is possible" precondition holds exactly because `LimitedDistinctAggregation` runs before `EnsureRequirements` and `CombinePartialFinalAggregate`, the only producers of the other two shapes. This PR makes that precondition explicit, since the rule now sits in the analyzer list ahead of enforcement. So I think the two PRs agree with each other. On the general mechanism, I agree with @alamb's framing: the invariants a plan can check about itself (schema, declared input properties satisfied) are what this boundary encodes, and `SanityCheckPlan` already verifies them. Shape facts like #26069's are a different kind: they describe what earlier rules produced, and nothing in the plan can verify them. For those I think the cheapest honest tool is what #26069 already does, a precondition stated in the rule's docs plus a debug assertion inside the rule, rather than a pipeline-level boundary that every rule has to declare against. I would land this PR as the base, and if a second concrete boundary that the plan itself can check shows up, build the general mechanism on top of it. Happy to review the PoC if @2010YOUY01 wants to explore it in parallel. -- 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]
