zhuqi-lucas commented on PR #23599: URL: https://github.com/apache/datafusion/pull/23599#issuecomment-6060919136
Thanks @2010YOUY01 — you're right, and I confirmed it: I dropped the new `slt` onto `main` unchanged and the whole file passes. It isn't a regression test, so I've removed it. The trap was that `EXPLAIN` shows the final plan, not what `WindowTopN` saw — `ProjectionPushdown` embeds the projection *after* the rule has already run. One more data point for your "hidden convention" framing: no other rule in `physical-optimizer` reads `filter.projection()` at all. So this PR wouldn't just rely on the convention — it'd be the first rule in core to step outside it. On your actual question. DataFusion is a library for building engines, so I don't think "a downstream pipeline produces this shape" is automatically out of scope. But the bar I'd want is that core could *plausibly* produce the shape itself, and today it can't — the only thing keeping it from happening is an ordering nobody wrote down. So I'd rather make that explicit than quietly depend on it. Concretely, I'd split this: - The `fetch` guard is independent of all this and holds on its own — happy to keep it in a separate PR. - For the projection support, I'll follow your call. If it's useful to core, I have an e2e test that does discriminate (it fails on `main` and passes here) by inserting an extra `ProjectionPushdown` before `WindowTopN` and planning real SQL — though that still needs a non-default order, which is exactly your point. If you'd rather not take it, I'm fine carrying it downstream. Either way the convention deserves to be written down — a rule silently ceasing to fire has no failing test anywhere. Happy to help with whatever systematic fix you have in mind. -- 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]
