zhuqi-lucas opened a new issue, #26152: URL: https://github.com/apache/datafusion/issues/26152
### Is your feature request related to a problem or challenge? Proposed by @2010YOUY01 in https://github.com/apache/datafusion/pull/23599#issuecomment-6072789069. `sqllogictest` runs under exactly one physical optimizer configuration: the default rule list. That makes a whole class of plan shapes untestable end-to-end, because the shape only arises under a different rule order. The concrete case that surfaced this: `WindowTopN` sits at position 117 in the default list and `ProjectionPushdown` at 142. A `FilterExec` carrying an embedded projection therefore never reaches `WindowTopN` from a stock pipeline — but it does in downstream pipelines that re-run projection pushdown earlier. #21596 asks `WindowTopN` to handle that shape, and its premise ("this happens when `ProjectionPushdown` runs before `WindowTopN`") is not true of the default list. This leaves us with a bad choice. Either a rule grows logic for shapes no `slt` can produce — untested in practice, and an open-ended maintenance burden as "handle every possible shape" is not a realistic bar for an early-phase rule — or downstream engines carry local patches for orderings that upstream never exercises. The underlying tension, in @2010YOUY01's words: is the public API the *ordered default rule list*, or is it *each individual rule*? Today we rely on an ordering nobody wrote down. ### Describe the solution you'd like Run the `slt` corpus under a second, named optimizer pipeline, so an alternative rule order is a first-class thing upstream exercises rather than an assumption downstream engines rely on. Most of the machinery already exists. `# configMatrix:` (#24493) re-runs a single `slt` file across a config sweep and attributes failures to the combination that produced them: ``` # configMatrix: datafusion.optimizer.prefer_hash_join=true,false # configMatrix: datafusion.execution.batch_size=1,2,100,8192 ``` Five files use it today (`sort_merge_join_matrix.slt`, `piecewise_merge_join_matrix.slt`, `mark_join_matrix.slt`, ...). What it sweeps is `ConfigOptions` key/value pairs, not rule lists. So the missing piece is a config option that selects a pipeline variant, e.g. ``` datafusion.optimizer.pipeline = default | reoptimize ``` where `reoptimize` is the default list plus a few re-optimization passes (a second `ProjectionPushdown`, etc.) — chosen to be closer to what downstream engines actually run. `prefer_hash_join` is precedent for a config option that changes the physical plan. Files then opt in with: ``` # configMatrix: datafusion.optimizer.pipeline=default,reoptimize ``` and the existing matrix machinery handles the rest. Two things this buys, which are worth keeping separate: 1. **Results agree.** Sweeping the corpus proves the alternative pipeline computes the same answers everywhere. This is the part that scales, and the part that makes rule-level shape handling maintainable instead of speculative. 2. **What capability does it actually buy?** This needs its own plan assertions — a sweep can't show it. Note the existing matrix files carry almost no `EXPLAIN` (`sort_merge_join_matrix.slt` has one), because a different rule list produces different plans and `slt` has no per-configuration expected output. So plan-shape coverage stays in targeted tests, and the sweep covers correctness. ### Describe alternatives you've considered - **Per-rule unit tests that construct the shape by hand.** This is what's done today. It's a circular dependency — upstream UTs added to guard downstream use cases — and it can't catch interactions between rules. - **A custom optimizer pipeline running a single query in a Rust test.** Weaker than `slt`: it covers one query, and nothing keeps it in sync as the rule list evolves. - **Enumerating all orderings of the rule list.** Combinatorially hopeless, and most orderings are not shapes anyone runs. ### Additional context - #21596 — `WindowTopN` + embedded projections, the case that surfaced this. Blocked on having a way to test it. - #23599 — the PR where this came up; now cut down to a bail-out guard, with the projection handling dropped for exactly this reason. - #24493 — the `configMatrix` runner this would build on. -- 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]
