Repository navigation
fix: SanityCheckPlan error when a per-partition constant leads an ordering - #27191
jayzhan211 wants to merge 19 commits into
Conversation
A normal ordering keeps a constant whose value differs between partitions (`AcrossPartitions::Heterogeneous`, since apache#23528), so such a constant can lead a normal ordering. `SortKeyWalk` assumed every ordering's head was at index 0 until the first key was pinned, so it treated the constant as the head and did not see the expression after it. A window ordered by `(total DESC, k)` over an input ordered by `[k, total]`, with `k` constant within each partition, then failed `SanityCheckPlan`. Compute the initial heads past leading fixed expressions, as the walk already does after each pin. Closes apache#26762 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions
Bot
added
physical-expr
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/per-partition-constant-ordering (6e6a8ae) to 4bf32cb (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/per-partition-constant-ordering (6e6a8ae) to 4bf32cb (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/per-partition-constant-ordering (c777864) to 4bf32cb (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @jayzhan211 for taking care of this
Left few comments please take a look
| if normal | ||
| .iter() | ||
| .any(|ordering| walk.is_fixed(&ordering[0].expr)) |
There was a problem hiding this comment.
Cache initial head offsets instead of rescanning each walk
There was a problem hiding this comment.
Same answer as on the comment about the scan in SortKeyWalk::new, a few lines up: the planning benchmarks are flat, so I'd rather not add a cache.
| let implied: Vec<_> = normal_orderings | ||
| .iter() | ||
| .filter_map(|ordering| { | ||
| self.implied_ordering(&ordering[0], ordering[1..].iter()) |
There was a problem hiding this comment.
add_orderings looks for an implied ordering only through ordering[0]. Normal orderings keep heterogeneous (per-partition) constants, so when such a constant leads the ordering the equality behind it is never used, and the order-dependence this commit removes is still there.
| .iter() | ||
| .any(|ordering| walk.is_fixed(&ordering[0].expr)) | ||
| { | ||
| walk.head_idxs = normal |
There was a problem hiding this comment.
SortKeyWalk::new moves heads past leading fixed expressions but never runs the implied-ordering discovery that pin runs for moved heads. A head exposed this way implies nothing until some key has been pinned.
There was a problem hiding this comment.
I closed this in discovery rather than in the walk: add_orderings and discover_new_orderings now look past leading constants. SortKeyWalk::new runs on every ordering check and its result is dropped when the walk ends, so a ProjectionExec above couldn't use it. With the ordering stored, a window on c over a TopK on (k, a, c) with a not selected no longer needs a sort at one partition.
| query DPI | ||
| SELECT a, c, ROW_NUMBER() OVER (ORDER BY c) AS r | ||
| FROM (SELECT a, c FROM ordered_through_cast WHERE a = CAST(c AS DATE) ORDER BY a, c LIMIT 10) | ||
| ORDER BY r; |
There was a problem hiding this comment.
The new test "The same with a per-partition constant leading the sort" (and the merge variant) checks only result rows, not the plan, so it cannot tell whether [c] was derived or whether a sort was just added.
| if projection_exec | ||
| .input() | ||
| .equivalence_properties() | ||
| .ordering_satisfy_requirement(requirements.first().clone())? |
There was a problem hiding this comment.
The new guard compares requirements.first() with parent_required.first(), but these can be different alternatives: remap_requirement_through_projection uses filter_map, which silently drops alternatives it cannot remap.
| // may not be able to do: the proof can depend on a column that it drops, | ||
| // such as a leading per-partition constant. Keep the sort above the | ||
| // projection in that case. | ||
| if projection_exec |
There was a problem hiding this comment.
Altitude: the "input has the ordering but the node cannot prove it" guard is added only for ProjectionExec. HashJoinExec with an embedded projection (handle_hash_join) also drops columns before pushing a remapped requirement to its right child, so it can fail the same #26747 way.
There was a problem hiding this comment.
This can't happen with the default optimizer rules: the physical planner builds HashJoinExec without a projection, and ProjectionPushdown, which embeds one, runs after EnsureRequirements. I ran the shape, a window on (g, c) over a join whose right side is a TopK on (g, a, c) with a not selected, and it plans at 1 and 4 partitions. A custom rule order that embeds the projection earlier could hit this. If anyone has one, I'm happy to open an issue for it.
| // a constant may lead it. Within a partition it has one value, so it | ||
| // is fixed and the head is the first expression after it. | ||
| let normal = &props.oeq_cache.normal_cls; | ||
| if normal |
There was a problem hiding this comment.
Every SortKeyWalk::new (i.e. every ordering_satisfy / ordering_satisfy_requirement call) now scans all normal orderings and runs is_fixed (hash lookups, equivalence-class lookups, recursion into children) to find head offsets. Those offsets depend only on props.
There was a problem hiding this comment.
When the heads are columns, this is two lookups per ordering, and the sql_planner bot run on c777864f7c is flat: physical_plan_tpch_all unchanged, physical_plan_clickbench_all +1%, physical_plan_tpcds_all -3%, worst single query +3%. A cached list of heads would have to be kept correct by every path that changes constants or orderings (add_constants, add_orderings, update_oeq_cache, clear_orderings, clear_per_partition_constants). That's more state for no visible gain, so I'd leave it out unless a benchmark shows a cost.
| .filter_map(|ordering| { | ||
| self.implied_ordering(&ordering[0], ordering[1..].iter()) | ||
| .ok() | ||
| .flatten() |
There was a problem hiding this comment.
.ok().flatten() silently discards errors from implied_ordering (e.g. data_type or Interval::make_unbounded failures). The equivalent call in discover_new_orderings propagates them with ?.
There was a problem hiding this comment.
I agree it would be good not to drop the errors
There was a problem hiding this comment.
add_ordering and add_orderings now return Result, so this error reaches the caller. SortPreservingMergeExec::new and RepartitionExec::with_preserve_order can't return one without changing their signatures (57 and 20 call sites), so they panic on it, as SortExec::new already does. In practice it takes an expression that doesn't match the schema. @alamb let me know if you'd rather change those two signatures as well.
| .input() | ||
| .equivalence_properties() | ||
| .ordering_satisfy_requirement(requirements.first().clone())? | ||
| && !projection_exec |
There was a problem hiding this comment.
The guard recomputes projection_exec.equivalence_properties().ordering_satisfy_requirement(parent_required.first()). The caller pushdown_sorts_helper has already computed this as satisfy_parent (line 264) for the same plan and requirement.
There was a problem hiding this comment.
After the change for your comment on alternatives, this checks the alternative that gets pushed down, which isn't always the first one that satisfy_parent checked. Reusing it would also add a parameter to pushdown_requirement_to_children that only this branch reads. The check only runs when the input already has the ordering, so I'd keep it.
There was a problem hiding this comment.
discover_new_orderings now duplicates work: it computes implied_ordering for each leading ordering and passes the results to add_orderings, which immediately recomputes implied_ordering on those results and recurses again, each level re-normalizing and calling update_map.
There was a problem hiding this comment.
These aren't the same computation: discover_new_orderings derives from an ordering, and add_orderings then derives from what that ordering implied. The recursion is what finds chains. With d = CAST(a), a = CAST(c) and ordering [d, a, c], [c] is found with it and missed with one level, and implied_orderings_chain covers that. It only runs when something was implied.
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/per-partition-constant-ordering (c777864) to 4bf32cb (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
github-actions
Bot
added
documentation
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/per-partition-constant-ordering (434a127) to 4bf32cb (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/per-partition-constant-ordering (434a127) to 4bf32cb (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I think this one is ok, thank you for working on it @jayzhan211
I am still not very happy with the current state of the EquivalenceProperties and SortWalkKey (I realize you did not cause this). Specifically the fact that there are now explicit "implied" orderings in addition to equivalence properties seems like they partially duplicate significant functionality.
This PR seems to be adding more (complicated) code to add implied ordering in another path, rather than unifying the behavior. I realize unifying the logic is too much for a bug fix. It would be really nice to try and unify all this spaghetti as a follow on, otherwise we are going to be chasing bugs down in this code indefinitely
I also spent a while writing up what I think SortKeyWalk actually does
| /// For each ordering (the normal orderings of `props`, then `implied`), | ||
| /// the index of its head. Empty until the first pin, so that a walk with | ||
| /// nothing pinned allocates nothing; until then every head is at index 0. | ||
| /// the index of its head. Empty until a head moves, so that a walk that |
There was a problem hiding this comment.
I found all this talk about head super confusing so I tried to document it better
| if class.is_some_and(|class| class.constant.is_some()) { | ||
| return true; | ||
| } | ||
| // A column is only fixed if it is pinned or constant: |
There was a problem hiding this comment.
isn't it more like a varying column can't be fixed (by definition?)
| /// input is sorted by `c`, and the projection proves `score` (`c` renamed), | ||
| /// but not `score % 2`, so the requirement can be pushed down. | ||
| #[test] | ||
| fn projection_guard_compares_the_pushed_alternative() -> Result<()> { |
There was a problem hiding this comment.
what is this tet comparing? I am pretty confused
| Ok(()) | ||
| } | ||
|
|
||
| /// Returns the orderings that the normal ordering `ordering` implies. If a |
There was a problem hiding this comment.
It would help me here to write out the examples more clearly: add whitespace between input and outputs, and explain them in more detail -- for example how I have done on #27266
| let mut implied = vec![]; | ||
| for (idx, key) in keys.iter().enumerate() { | ||
| if let Some(rest) = | ||
| self.implied_ordering(key, keys[idx + 1..].iter().copied())? |
There was a problem hiding this comment.
I also vote to return ok() and leave a comment explaining why the error is ignored
This will make it less disruptive to backport
Then we can make a PR to main perhaps to propagate the error
| /// constants that a normal ordering can keep (those that differ between | ||
| /// partitions) are left out, because within a partition they have one | ||
| /// value. | ||
| fn implied_by(&self, ordering: &LexOrdering) -> Result<Vec<Vec<PhysicalSortExpr>>> { |
There was a problem hiding this comment.
I wonder why this doesn't return Result<Vec<LexicalOrdering>> which I think is what the PhysicaSortExpr represent
I've also found equivalence properties overly complicated, and the "walk" PR is my first step toward simplifying them. I'd like to revisit them and come up with a version that's easier to understand! |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
@jayzhan211, thank you for addressing the window ordering regressions. The changes make ordering inference consistent across property registration paths and preserve valid plans that previously failed sanity checks. The approach looks appropriate, and I found no blocking correctness issues during static inspection.
The architectural boundaries are sound. Implied orderings belong in equivalence-property discovery, while the projection pushdown guard appropriately retains an explicit sort when the projection cannot establish the required ordering. This avoids relying on ordering information that does not survive the projection boundary.
The regression coverage is substantial, and the reported generated-query results support the fix. I have one optional suggestion to strengthen sort-direction coverage. I am approving the change. I did not independently run the Rust or SQL tests.
| equality_first.add_equal_conditions(Arc::clone(&cast_c), Arc::clone(&a))?; | ||
| equality_first.add_ordering(ordering.clone()); | ||
|
|
||
| let mut ordering_first = EquivalenceProperties::new(Arc::clone(&schema)); |
There was a problem hiding this comment.
Could we parameterize implied_ordering_does_not_depend_on_call_order over sort options? It would be useful to cover matching descending directions and verify that [a DESC, c ASC] does not imply global [c ASC] across distinct dates, with both equality/ordering insertion orders. The reported generated sweep already covers all four directions, so this would provide focused regression coverage.
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
Thanks for the updates. Storing [k, c] fixed the extra sort, and returning to .ok() keeps this easy to backport. One more thing before merging: an existing caller now depends on call order (see the inline comment on add_orderings).
| /// Adds new orderings into the existing ordering equivalence class. | ||
| /// Adds new orderings into the existing ordering equivalence class, along | ||
| /// with the orderings they imply through known equalities. | ||
| pub fn add_orderings( |
There was a problem hiding this comment.
Now that add_orderings derives implied orderings, a caller that adds an ordering before clearing per-partition constants keeps orderings that no longer hold. RepartitionExec::eq_properties_helper does this: it calls add_ordering(merge key) and then clear_per_partition_constants(). SortPreservingMergeExec::compute_properties clears first.
With k a Heterogeneous constant, CAST(c AS DATE) = a, and merge key [k, a, c], the repartition order leaves ordering_satisfy([c]) true, although [c] doesn't hold once partitions with different k are merged. The SPM order gives false. Could eq_properties_helper clear the constants before add_ordering, the way SPM does? I don't see a SQL path to this today, because UnionExec drops equalities, so it mainly affects custom plans and any future source of per-partition constants.
That would be amazing thank you @jayzhan211 -- I agree SortWalkkey is a great first step |
## Which issue does this PR close? - split out of apache#27234. ## Rationale for this change While reviewing apache#27191 I was researching how `EquivalenceProperties` tracks orderings, equal expressions, constants and groupings, and wanted to write down what I learned so others do not have to repeat it. The concepts were documented in pieces across several structs, with no single place that explained them together. ## What changes are included in this PR? Rustdoc only. The `EquivalenceProperties` doc in `datafusion/physical-expr/src/equivalence/properties/mod.rs` becomes the hub that the other struct docs link to. It keeps the existing examples and adds: ## What is the testing strategy for this PR? CI ## Are there any user-facing changes? No. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Jay Zhan <jayzhan211@gmail.com>
| let mut implied = vec![]; | ||
| for ordering in &normal_orderings { | ||
| implied.extend(self.implied_by(ordering).unwrap_or_default()); |
There was a problem hiding this comment.
Should we clear repartition constants before deriving merge orderings?
| // Each implied ordering is shorter than the one implying it, so this | ||
| // terminates: | ||
| self.add_orderings(implied); |
There was a problem hiding this comment.
can we use a deduplicated worklist and avoid expanding already-known candidates?
There was a problem hiding this comment.
Each round's implied orderings are now deduplicated before they are expanded. I didn't skip candidates that are already in the class: remove_redundant_entries drops the repeats anyway, and skipping them would rely on every earlier call having derived their implied orderings.
| if let Some(rest) = | ||
| self.implied_ordering(key, keys[idx + 1..].iter().copied())? | ||
| { | ||
| let prefix = keys[..idx].iter().copied().cloned(); |
There was a problem hiding this comment.
IMO the Simpler fix would be: keep the constants in the stored implied ordering (store [k, c], not [c]). SortKeyWalk::new already skips a leading fixed head, so [c] still holds within a partition, and clearing k then removes it correctly.
wdyt?
There was a problem hiding this comment.
This handles a leading constant, but not one between a key and its arguments, as in implied_ordering_past_a_constant_between_keys: [a, k, c] with a = CAST(c AS DATE) would store [k, c]. That holds within each partition, but not after RepartitionExec merges on [a, k, c]. Rows from two partitions on the same date come out as k = 1, 2, and the next date starts again at k = 1. Since the merge adds the ordering before clearing k, [k, c] would still be kept.
There was a problem hiding this comment.
I don't understand the implications of this comment
Specifically, if we didn't do this change, would we get wrong plans / errors? Or is it just a missed optimization opportunity?
There was a problem hiding this comment.
I'd keep [c] in this PR. As viirya says, the RepartitionExec call order needs fixing either way, and that fix is now in the PR. With it, SortPreservingMergeExec, RepartitionExec and CoalescePartitionsExec all drop [c] when they merge partitions.
There was a problem hiding this comment.
I think the issue is not critical but the change is quite small
There was a problem hiding this comment.
It was a wrong-result risk, not a missed optimization: before the reorder, the merged output could claim [c] although it doesn't hold, so a sort above it could be removed and rows come out in the wrong order. We don't know a SQL query that reaches it, which is why it wasn't critical. 2ceaba5 fixes it, and test_preserve_order_drops_per_partition_implied_ordering covers it.
| } | ||
| if !new_orderings.is_empty() { | ||
| self.add_orderings(new_orderings); | ||
| new_orderings.extend(self.implied_by(ordering)?); |
There was a problem hiding this comment.
discover_new_orderings still passes implied_by errors up with ?, while add_orderings ignores them. implied_by now checks every key, not just ordering[0], so whether the call fails still depends on the order of calls.
| // such as a leading per-partition constant. Keep the sort above the | ||
| // projection in that case. Alternatives that cannot be remapped are not | ||
| // pushed down, so compare against the first one that can. | ||
| let pushed = parent_required.iter().find(|required| { |
There was a problem hiding this comment.
should we walk the alternatives once and keep the first (original, remapped) pair, or have the remap helper return it.
| self.oeq_cache.update_map(); | ||
| // Each implied ordering is shorter than the one implying it, so this | ||
| // terminates: | ||
| self.add_orderings(implied); |
There was a problem hiding this comment.
add_orderings passes implied back to itself without removing duplicates. Each level also runs remove_redundant_entries (O(n²)) and update_map over the whole class, so the work can grow with the number of key subsets.
| statement ok | ||
| set datafusion.execution.target_partitions = 4; | ||
|
|
||
| query IIII |
There was a problem hiding this comment.
The new 4-partition case (SELECT SUM(v) AS k, COUNT(*) AS c ...) only checks result rows, with no EXPLAIN. It cannot show whether a sort was avoided or added.
| .iter() | ||
| .any(|ordering| walk.is_fixed(&ordering[0].expr)) | ||
| { | ||
| walk.head_idxs = normal |
There was a problem hiding this comment.
SortKeyWalk::new repeats the head-moving logic in pin (lines 177-180: move head_idx forward while is_fixed) as a separate position scan, so the rule for picking a head now lives in two places.
There was a problem hiding this comment.
The walk does not derive it because nothing is pinned. This is the order dependence the new comment says must not happen.
There was a problem hiding this comment.
I couldn't get an order dependence from new_with_orderings. It starts with no equalities, and add_equal_conditions and add_constants both re-derive the implied orderings of every stored ordering. With k a per-partition constant and a = CAST(c AS DATE), ordering_satisfy([c]) is true for [a, c] and for [k, a, c], whichever of the equality and the constant is added first, and the same as with add_ordering. Which sequence of calls do you have in mind?
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
The RepartitionExec call order from my last comment is still open. It's the only one of the new comments I'd hold the merge for. The others can be follow-ups. Keeping the constants in the implied ordering doesn't remove the need for the call-order fix (see the reply inline).
…the projection guard
|
FWIW I think it is important for this particular to fix the regression so we can safely backport to branch_56 and get the release out; I think we should try to minimize the impact / need for any other changes I think the number of comments on this PR is indicative of how complex this code is and there are many ways we can improve it, but I would like to suggest we leave improvements to future follow on PR |
|
Address all! |
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @jayzhan211 for all the iterations.
I think we are in a good enough position to move forward with this. Other improvements will be in further follow ups.
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
The RepartitionExec fix and its test cover what I raised. Thanks for all the iterations.
…he#27237) ## Which issue does this PR close? - N/A, documentation only. ## Rationale for this change While reviewing apache#27191 I was researching how `EquivalenceProperties` tracks orderings and groupings, and wanted to write down what I learned so others do not have to repeat it. The `GroupingEquivalenceClass` docs described the property in prose but had no example, and did not say how it relates to `OrderingEquivalenceClass`, which it parallels. ## What changes are included in this PR? Rustdoc only, in `datafusion/physical-expr/src/equivalence/grouping.rs`: Split out of a larger documentation PR (apache#27234) for easier review. ## What is the testing strategy for this PR? CI ## Are there any user-facing changes? No. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Which issue does this PR close?
Rationale for this change
Queries that planned in earlier releases fail with
SanityCheckPlanwhen a window's input ordering starts with a per-partition constant or is proven through an equality such asa = CAST(c AS DATE). Across 7,440 generated query runs,mainhas 62 failures on queries that plan on 55.2.0, and this PR has none, with the same rows wherever both plan.There are three causes. In each one, window planning and
SanityCheckPlanreach different answers about whether the input is sorted:add_constantsdrops them, so window planning sees[total]andSanityCheckPlansees[k, total]branch-56)cover a TopK on(k, a, c), withka per-partition constant anda = CAST(c AS DATE)add_constantsdiscovers that[a, c]implies[c], but an ordering added after the equality doesn'tbranch-56)(k, c)over a TopK on(k, a, c), whereaisn't selectedThis PR replaces #27140 and keeps its commit.
What changes are included in this PR?
Start reading at
add_orderings. It is the decision I most want reviewed, because it makes the optimizer claim more orderings, and a wrong claim means wrong results.SortKeyWalk::newadd_orderingsadd_constantsandadd_equal_conditionsalready do, for any key and past the constants an ordering keeps:[k, a, c]witha = CAST(c AS DATE)implies[k, c]handle_projection_pushdownRepartitionExecSortPreservingMergeExecdoes[c]from[k, a, c]after a merge, where it holds only within each partitionSortKeyWalk::is_fixedWho is affected. Plans with an ordering that starts with a per-partition constant, or that is proven through an equality or a column a projection drops. It can't stop halfway.
No API change. Discovery in
add_orderingscan fail only on an expression that doesn't match the schema. That implied ordering is then left out, which can cost a sort but not change results.Worst case. 4 of the 7,440 generated runs plan a sort that
maindoesn't: aGROUP BY k, cover a TopK on(a, c)witha = CAST(c AS DATE), at 4 partitions. Knowing[c], the aggregate streams oncand sorts oncafter its hash repartition.mainpicks the same aggregate when the ordering is known before the equality. 392 runs have fewer sorts.What is the testing strategy for this PR?
window.sltcovers each cause, including a merge of partitions above the sort, and checks the plans. Removing any one of the first three changes, limiting discovery to the first key, or not skipping constants makes tests fail, inwindow.sltor in the unit tests for the walk and for implied orderings. ARepartitionExecunit test covers the merge.physical_optimizerintegration tests and the equivalence fuzz tests.Generated query runs, benchmarks, rejected alternatives, and queries that still fail
Generated runs. Each sweep runs at
target_partitions1 and 4. A regression plans on the reference build and fails on the other build.main, vs 55.2.0main, vs before #23528a = CAST(c AS DATE)head = f(arg)forCAST(x AS VARCHAR),ABS, negation, modulo,upperand order-preserving functions, in all 4 sort directionsThe results are the same with this patch on
branch-56, where all SLT files pass as well. Compared withmainon the runs that plan on both, the rows are the same, 4 runs have more sorts (see the worst case above) and 392 have fewer.Benchmarks.
equivalence_properties, best of 10 alternating runs on a busy laptop, so approximate: single-key checks are 6-12% slower (about 20-40 ns), and window-heavy orderings are 24-33% faster.mainordering_satisfy/1_key/8ordering_satisfy_requirement/1_key/8find_longest_permutation/1_expr/32window/4window/32Rejected.
add_constantskeep per-partition constants failstest_eliminate_redundant_monotonic_sorts.lower(g)as a uniform constant isn't enough, because a constant from an aggregate withoutGROUP BYreally is per-partition.handle_projection_pushdown, makes 2 generated queries failSanityCheckPlan: a window over aGROUP BYwhose input ordering leads with such a constant.pushdown_sorts_helperonly fixes one more query, and that query already fails on every release, so I kept the narrower check.Still failing, not regressions. A window over a
UNION ALLof two TopKs that havev AS wfails on every build I tried, including before #23528:Are there any user-facing changes?
Affected queries plan again. This is a candidate for
branch-56, where it applies without conflicts.