Skip to content

[Enhancement] Add Window skew optimization for multiple column partition expressions #73592

Open
martinr0x wants to merge 2 commits into
StarRocks:mainfrom
martinr0x:mbogusz/force-distributed-sort-on-hint-os
Open

[Enhancement] Add Window skew optimization for multiple column partition expressions #73592
martinr0x wants to merge 2 commits into
StarRocks:mainfrom
martinr0x:mbogusz/force-distributed-sort-on-hint-os

Conversation

@martinr0x

@martinr0x martinr0x commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Why I'm doing:

Similar to SplitWindowSkewToUnionRule, this PR adds another optimization for skewed window partitions.

SplitWindowSkewToUnionRule needs the optimizer to identify the exact skewed value, because it splits the input into skewed and non-skewed branches based on that value. This is feasible for a single partition column, but does not work for multi-column partition expressions without multi-column statistics.

How it works:

This optimization avoids that requirement by forcing a sort-merge path before partitioning the data by the window partition expressions. After the merge sort, local_ordered_partition_exchange distributes partitions across multiple pipeline drivers on one node while preserving the required order and partition boundaries.

  ANALYTIC        ← OrderedPartitionExchanger (intra-node)
  ↑
  MERGING-EXCHANGE(GATHER)    ← single global stream
  ↑
   SORT           ← inter-node gather, all data converges here

How is it different from skewed optimization:

The skewed optimization is a similar option that is less aggressive. Instead of having a distributed sort of all data, data is first partitioned by p and then sorted by each backend. Once the data is partitioned each node adds a sort + gather and then uses the local_ordered_partition_exchange to distribute partitions across pipelines.
This optimization is useful if some partitions are slightly skewed but for large skew the new optimization is preferred.

What I'm doing:

Im adding the rule WindowSkewToMergeSortRule.
This rule can be enabled by the enable_window_skew_merge_sort session variable or by the [merge_sort] hint.

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
    • This pr needs auto generate documentation
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 4.1
    • 4.0
    • 3.5
    • 3.4

@martinr0x martinr0x requested review from a team as code owners May 20, 2026 15:36
@wanpengfei-git wanpengfei-git requested a review from a team May 20, 2026 15:37
@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from b3b9512 to d765390 Compare May 20, 2026 15:58
@CelerData-Reviewer

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch 2 times, most recently from ae0bfb5 to a63168f Compare May 20, 2026 16:11
@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from a63168f to 82f4031 Compare May 26, 2026 13:28
@CelerData-Reviewer

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from 91b618a to b1a0cae Compare May 28, 2026 10:27
@CelerData-Reviewer

Copy link
Copy Markdown

@codex review

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from b1a0cae to 21ae88a Compare May 28, 2026 10:31
@martinr0x martinr0x requested a review from Copilot May 28, 2026 10:31

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1a0cae53d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a “merge_sort” execution path for window functions to mitigate skew on composite partition keys, controllable via a new analytic hint and new session variables, with FE/BE propagation and tests.

Changes:

  • Introduce [merge_sort] analytic hint and propagate force_merge_sort through AST → optimizer operators → plan nodes → thrift.
  • Add WindowSkewToMergeSortRule plus new session variables enable_window_skew_merge_sort / force_window_merge_sort.
  • Add FE/BE and SQL regression tests for explain/pipeline behavior.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/sql/test_window_function/T/test_window_merge_sort Adds SQL test cases covering default, hinted, forced, and skew-enabled modes
test/sql/test_window_function/R/test_window_merge_sort Captures expected results and explain assertions for the new SQL tests
gensrc/thrift/PlanNodes.thrift Adds force_merge_sort to TAnalyticNode for FE→BE propagation
fe/fe-parser/src/main/java/com/starrocks/sql/ast/HintNode.java Adds merge_sort constant for analytic hint parsing
fe/fe-core/src/test/java/com/starrocks/sql/plan/WindowSkewToMergeSortRuleTest.java Adds optimizer plan tests for merge-sort skew rule, force mode, and hint behavior
fe/fe-core/src/main/java/com/starrocks/sql/plan/PlanFragmentBuilder.java Passes forceMergeSort into AnalyticEvalNode construction
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/transformer/WindowTransformer.java Preserves forceMergeSort when reordering window operators
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/transformer/QueryTransformer.java Propagates forceMergeSort between rewritten window operators
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/rule/tree/lowcardinality/DecodeRewriter.java Propagates forceMergeSort through decode rewrite of physical window ops
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/rule/transformation/WindowSkewToMergeSortRule.java Introduces transformation rule that sets forceMergeSort based on skew detection or force var
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/rule/transformation/SplitWindowSkewToUnionRule.java Prevents union-splitting when merge-sort is forced
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/rule/implementation/WindowImplementationRule.java Propagates forceMergeSort into physical window operator
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/rule/RuleType.java Registers new rule type TF_WINDOW_SKEW_TO_MERGE_SORT
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/operator/physical/PhysicalWindowOperator.java Adds forceMergeSort field and includes it in equals/hashCode
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/operator/logical/LogicalWindowOperator.java Adds forceMergeSort field and builder support
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/RequiredPropertyDeriver.java Forces gather distribution when forceMergeSort is set
fe/fe-core/src/main/java/com/starrocks/sql/optimizer/QueryOptimizer.java Schedules new rule under session-variable control
fe/fe-core/src/main/java/com/starrocks/sql/ast/expression/AnalyticExpr.java Parses [merge_sort] hint and stores forceMergeSort
fe/fe-core/src/main/java/com/starrocks/qe/SessionVariable.java Adds new invisible session variables to control feature
fe/fe-core/src/main/java/com/starrocks/planner/AnalyticEvalNode.java Persists/writes force_merge_sort into thrift and normal form
be/test/exec/pipeline/analytic_node_pipeline_test.cpp Adds pipeline tests verifying ordered partition exchange with force/skew
be/test/CMakeLists.txt Registers the new BE pipeline unit test
be/src/exec/analytic_node.cpp Applies ordered partition exchange when force_merge_sort is set

Comment thread be/src/exec/analytic_node.cpp
Comment thread be/src/exec/analytic_node.cpp Outdated
@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch 3 times, most recently from 496e1d1 to d19a07b Compare May 28, 2026 12:23
@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from d19a07b to db87504 Compare June 8, 2026 09:59
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

No new undocumented parameters detected by the param-drift check.

@CelerData-Reviewer

Copy link
Copy Markdown

@codex review

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from 1af5565 to a83db62 Compare June 17, 2026 07:53
@CelerData-Reviewer

Copy link
Copy Markdown

@codex review

@github-actions

Copy link
Copy Markdown
Contributor

Module-risk briefing for PR #73592sql/optimizer module


sql/optimizer — 2 diff-aligned findings (from 20 merged bugfixes)

Pitfall · Low-cardinality decode rewriter field propagation

Changed hunk: DecodeRewriter.java:visitPhysicalAnalytic reconstructs a PhysicalWindowOperator and this PR adds isForceMergeSort() to the constructor call.

Verify:

  • The new forceMergeSort field is correctly threaded through every reconstruction path in DecodeRewriter/DecodeContext, not just visitPhysicalAnalytic. If any path silently drops the flag, the low-cardinality pass rebuilds a window node without its merge-sort requirement — potentially causing incorrect execution or a wrong plan shape.
  • CTEConsume + window function combinations are covered by tests. The dictification path for CTEConsume projections has its own visitor and has caused incorrect plans in the past (see [[e:pr/StarRocks_starrocks/72418]]).
  • UNNEST-adjacent window expressions are tested; element-type vs. container-type confusion in the low-card path has caused BE crashes ([[e:pr/StarRocks_starrocks/72027]], [[e:pr/StarRocks_starrocks/74159]]).

Recurring bug class · Statistics propagation in window-skew rules

Changed files: SplitWindowSkewToUnionRule.java and new WindowSkewToMergeSortRule.java.

PR [[e:pr/StarRocks_starrocks/71622]] fixed SplitWindowSkewToUnionRule's OptExpressionDuplicator not propagating statistics to duplicated OptExpression nodes. This PR modifies SplitWindowSkewToUnionRule and adds the analogous WindowSkewToMergeSortRule.

In WindowSkewToMergeSortRule.transform(), the buildResult(window, input) call constructs new OptExpression nodes. Verify that child statistics are propagated to the newly constructed expressions — downstream rules that check child.getStatistics() == null will silently skip them if statistics are absent, which could suppress further optimizations or produce incorrect cost estimates.


History-backed heuristic from 20 merged bugfixes in sql/optimizer — verify each point against the actual change before acting on it.

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from a83db62 to 554e7e2 Compare June 17, 2026 12:46
@CelerData-Reviewer

Copy link
Copy Markdown

@codex review

@github-actions

Copy link
Copy Markdown
Contributor

Module-risk briefing for PR #73592 — history-backed review focus for the window/analytic skew optimization area (from 2 merged bugfixes).


Window skew optimization (fe/sql/optimizer/rule/transformation)

Review pitfalls

  1. Statistics freshness before new ruleSplitWindowSkewToUnionRule previously required Utils.calculateStatistics() to be called before it; without this, duplicated subtrees had no statistics and downstream rule invocations made incorrect plan choices. This PR correctly calls calculateStatistics before WindowSkewToMergeSortRule in QueryOptimizer.java. Verify the placement is still correct when both enableSplitWindowSkewToUnion and enableWindowSkewMergeSort are enabled: SplitWindowSkewToUnionRule fires first (pre-predicate-pushdown), modifying the tree, and then WindowSkewToMergeSortRule fires with a fresh calculateStatistics call — confirm the statistics at that later point correctly reflect the post-split plan and that no intermediate transformations between the two calculate calls invalidate skew estimates used by the second rule.
    Aligned to: QueryOptimizer.java lines adding Utils.calculateStatistics(tree, ...) + WindowSkewToMergeSortRule.getInstance().
    Source: [[e:pr/StarRocks_starrocks/71622]]

  2. forceMergeSort flag propagation completenessWindowSkewHint was previously missing from OptExpressionDuplicator (fixed in [BugFix] Handle WindowSkewHint in OptExpressionDuplicator  #70341), causing incorrect plan rewrites after MV-rewrite-style duplication. This PR propagates forceMergeSort through withOperator(), WindowTransformer.reorderWindowOperator(), QueryTransformer.window(), WindowImplementationRule, and DecodeRewriter — all the paths visible in the diff. Check whether OptExpressionDuplicator (used by MV-transparent-rewrite and other duplication paths) also handles the new forceMergeSort field on LogicalWindowOperator; the SplitWindowSkewToUnionRule guard (&& !lwo.isForceMergeSort()) prevents the main skew-union duplication path from reaching these windows, but MV rewrite or other future duplication callers may still invoke the duplicator on a plan subtree that contains a window with forceMergeSort=true.
    Aligned to: LogicalWindowOperator.java withOperator() additions; WindowTransformer.java, DecodeRewriter.java propagation hunks.
    Source: [[e:pr/StarRocks_starrocks/70341]]

  3. Mutual-exclusion guard between the two skew rulesSplitWindowSkewToUnionRule.check() now adds && !lwo.isForceMergeSort() to prevent both skew strategies from applying to the same window. Verify the hint path is symmetric: when [merge_sort] hint is set in SQL, AnalyticExpr sets skewHint = HINT_ANALYTIC_MERGE_SORT and forceMergeSort = true; confirm that this hint value does not also satisfy SplitWindowSkewToUnionRule's isSkewed() check (which reads from LogicalWindowOperator.isSkewed() set from HINT_ANALYTIC_SKEW), so the guard is not the only line of defence.
    Aligned to: SplitWindowSkewToUnionRule.java new guard; AnalyticExpr.java hint parsing.
    Source: [[e:pr/StarRocks_starrocks/71622]], [[e:pr/StarRocks_starrocks/70341]]


from 2 merged bugfixes (#71622, #70341) — history-backed heuristic; verify each pitfall against the actual change before acting on it.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 554e7e2d24

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread be/test/exec/pipeline/analytic_node_pipeline_test.cpp Outdated
@CelerData-Reviewer

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b8737842e2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread fe/fe-core/src/main/java/com/starrocks/qe/SessionVariable.java
@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from b873784 to 8d87674 Compare June 29, 2026 15:42
@CelerData-Reviewer

Copy link
Copy Markdown

@codex review

@github-actions

Copy link
Copy Markdown
Contributor

Module-risk briefing for PR #73592 (window skew merge-sort optimization) — from 20 merged bugfixes in sql/optimizer


sql/optimizer — 2 diff-aligned pitfalls

Pitfall: Verify all fields are threaded through DecodeRewriter reconstruction

DecodeRewriter.java:474 adds windowOp.isForceMergeSort() to the PhysicalWindowOperator constructor call inside visitPhysicalAnalytic. This is the exact field-threading pattern that caused [[e:pr/StarRocks_starrocks/74977]]: when SplitWindowSkewToUnion produced new window operators, the dict-decode pipeline reconstructed PhysicalWindowOperator from scratch, and any field not explicitly passed through the constructor was silently zeroed — causing column-id mismatches and BE crashes. The fix there added missing column tracking; this PR adds isForceMergeSort() to the same call site.

Review focus: confirm that DecodeRewriter.visitPhysicalAnalytic now passes every field of PhysicalWindowOperator, and that isForceMergeSort() is read from the original operator (not accidentally hard-coded false elsewhere in the decode pipeline). Any future field added to PhysicalWindowOperator will need the same treatment.

(from [[e:pr/StarRocks_starrocks/74977]] — dict decode column-id mismatch under SplitWindowSkewToUnion)


Pitfall: Empty analytic operators after column pruning

QueryOptimizer.java:628–634 inserts WindowSkewToMergeSortRule after predicate pushdown but before column pruning (OutputColumnsPrune). Historical pattern [[e:pr/StarRocks_starrocks/74810]]: after PushDownDistinctAggregateRule, column pruning left a LogicalWindowOperator with no window functions, which caused the ANALYTIC node's prepare() to segfault the BE. WindowSkewToMergeSortRule only annotates (it does not restructure the plan), but the new forceMergeSort flag changes which downstream pruning decisions are valid — verify that the pruning phase cannot produce a window operator with zero window calls when forceMergeSort=true, and that the existing empty-analytic guard (if any) covers this code path.

(from [[e:pr/StarRocks_starrocks/74810]] — empty ANALYTIC node segfault after column prune)


This is a history-grounded heuristic drawn from past bugfixes in touched modules — verify each point against the actual change before acting on it.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d87674879

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +3042 to +3043
@VariableMgr.VarAttr(name = ENABLE_WINDOW_SKEW_MERGE_SORT, flag = VariableMgr.INVISIBLE)
private boolean enableWindowSkewMergeSort = false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Document the new window merge-sort session knob

The repo-wide AGENTS.md requires user-facing config changes to update the matching docs in docs/en/ and docs/zh/ when applicable. This adds a settable session variable, and the same change also introduces the [merge_sort] window hint, but no documentation was added, so users and operators have no supported reference for when to use it or what plan behavior it changes.

Useful? React with 👍 / 👎.

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from 8d87674 to a95d5f3 Compare June 30, 2026 07:21
@github-actions

Copy link
Copy Markdown
Contributor

History-backed review focus for PR #73592module: sql/optimizer (from 20 merged bugfixes)


Review pitfall: Verify column references are fully remapped in DecodeRewriter

Why it applies here: DecodeRewriter.java is modified to thread forceMergeSort through when reconstructing a PhysicalWindowOperator during the low-cardinality dict decode pass. The historical pattern shows this is a fragile seam: when SplitWindowSkewToUnion rewrites changed the plan structure for window operators, the DecodeRewriter / UnionDictionaryManager lost track of output column ids, causing BE crashes.

What to check: In DecodeRewriter.visitPhysicalAnalytic(), confirm that the output column id mapping is still valid when forceMergeSort=true — specifically, the new plan shape (Gather → OrderedPartitionExchanger) produces column ids that the decode dict collector and rewriter both see consistently. A single off-by-one in output column tracking between the dict encode side and the analytic node's tuple output would silently corrupt results.

Cited fix: [[e:pr/StarRocks_starrocks/74977]] — SplitWindowSkewToUnion rewrites produce PhysicalUnionOperator with mismatched output column ids in dict decode pipeline.


Review pitfall: Empty analytic operators after column pruning

Why it applies here: WindowSkewToMergeSortRule fires after predicate pushdown and annotates a LogicalWindowOperator with forceMergeSort=true. A subsequent column-prune pass (e.g., triggered by a downstream projection) could remove all window call functions from that annotated operator. The BE's AnalyticNode::prepare() segfaults when its function list is empty, and the force_merge_sort branch in analytic_node.cpp does not guard against this.

What to check: Verify there is a guard — either in WindowSkewToMergeSortRule.check(), in the column prune rule, or in AnalyticNode::decompose_to_pipeline() — that prevents a force_merge_sort=true analytic node with zero analytic functions from reaching the BE. The SplitWindowSkewToUnionRule exclusion added in this PR (&& !lwo.isForceMergeSort()) only prevents the two rules from conflicting; it does not prevent empty-function pruning.

Cited fix: [[e:pr/StarRocks_starrocks/74810]] — Column-prune after PushDownDistinctAggregateRule produces empty ANALYTIC operator whose prepare() segfaults the BE.


This is a history-backed heuristic derived from 20 merged bugfixes in the sql/optimizer module — verify each point against the actual change before acting on it.

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from a95d5f3 to e3ffa89 Compare June 30, 2026 11:13
@github-actions

Copy link
Copy Markdown
Contributor

Module-risk briefing for PR #73592 — history-backed review focus from sql/optimizer (20 merged bugfixes)


Pitfall: Verify forceMergeSort is propagated through all PhysicalWindowOperator reconstructions in DecodeRewriter

Aligned hunk: DecodeRewriter.javawindowOp.isForceMergeSort() added to visitPhysicalAnalytic.

The recurring bug pattern ([[e:pr/StarRocks_starrocks/74977]]) is that window-skew rewrites confuse DecodeRewriter with mismatched output column ids, not just a missing constructor parameter. The fix in that PR required explicit column re-mapping in UnionDictionaryManager. The forceMergeSort path (gather→merge-sort→OrderedPartitionExchanger) changes how column batches arrive at the analytic node; if any other visit* method in DecodeRewriter reconstructs PhysicalWindowOperator without carrying forceMergeSort, or if the gather path produces a column layout the dict-decode pipeline doesn't expect, a similar column-id mismatch can surface at runtime.

Verify: every site that rebuilds PhysicalWindowOperator inside DecodeRewriter passes the new flag, and that the dict-decode pipeline's column tracking assumptions hold under the gather/merge-sort execution shape.

Fix PRs: [[e:pr/StarRocks_starrocks/74977]], [[e:pr/StarRocks_starrocks/74159]]


Pitfall: Empty analytic operator after column pruning can segfault the BE

Aligned hunk: QueryOptimizer.javaWindowSkewToMergeSortRule inserted late in logicalRuleRewrite (after prepareMetaOnlyOnce).

The prior fix ([[e:pr/StarRocks_starrocks/74810]]) guarded specifically against PushDownDistinctAggregateRule + column pruning producing an empty ANALYTIC node whose prepare() segfaults the BE. WindowSkewToMergeSortRule annotates window operators after predicate pushdown; if any subsequent pruning step zeros out the windowCall map of an annotated LogicalWindowOperator, the same segfault path is reachable. Confirm that the prune-empty-analytic guard inserted by #74810 runs after this new rule in the optimizer pipeline, or that the rule cannot produce a window operator whose calls are later fully pruned.

Fix PR: [[e:pr/StarRocks_starrocks/74810]]


This is a history-backed heuristic to focus review attention — verify each pitfall against the actual change before acting.

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from e3ffa89 to e6b12c3 Compare June 30, 2026 12:56
@github-actions

Copy link
Copy Markdown
Contributor

Module-risk briefing for PR #73592 — history-backed review focus for sql/optimizer (20 merged bugfixes)


sql/optimizer — diff-aligned review pitfalls

1. Low-cardinality dict pipeline: verify DecodeCollector handles forceMergeSort windows

Changed hunk: DecodeRewriter.java correctly propagates forceMergeSort into the reconstructed PhysicalWindowOperator. However, DecodeCollector (not in this diff) is the upstream pass that decides dict-optimization eligibility for window operators. Historically, when SplitWindowSkewToUnion introduced a new window-skew execution variant, DecodeCollector/UnionDictionaryManager produced mismatched output column ids [[e:pr/StarRocks_starrocks/74977]].

WindowSkewToMergeSortRule sets forceMergeSort=true, isSkewed=false, useHashBasedPartition=false — a new combination. Confirm that DecodeCollector's dict-eligibility logic (which previously keyed off isSkewed/useHashBasedPartition) is still correct for this new execution path. In particular, does applying dict optimization to a forceMergeSort window remain valid when it routes through GatherDistribution + OrderedPartitionExchanger instead of hash shuffle?

From [[e:pr/StarRocks_starrocks/74977]] and [[e:pr/StarRocks_starrocks/74159]]: two recent dict-decode bugs were triggered by window-skew rewrites that the DecodeCollector/DecodeRewriter pipeline was not updated for.


2. inputIsBinary was silently dropped in withOperator() — confirm no latent exposure

Changed hunk: LogicalWindowOperator.Builder.withOperator() now copies inputIsBinary (previously missing). Any rule that called withOperator() on a window with inputIsBinary=true would have silently reset the flag to false. The fix is correct, but: (a) is there an existing test that exercises inputIsBinary=true through a rule that calls withOperator? (b) the new WindowSkewToMergeSortRule.buildResult uses withOperator — did binary-input windows reach this rule before the fix?

Pitfall pattern from [[e:pr/StarRocks_starrocks/74791]] and [[e:pr/celerdata_celerdata-enterprise/56061]]: rules that rewrite one part of an operator but silently drop another field cause stale references downstream.


This briefing is a history-backed heuristic derived from merged bugfixes — verify each point against the actual change before acting on it.

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from e6b12c3 to da10983 Compare June 30, 2026 15:47
@github-actions

Copy link
Copy Markdown
Contributor

Module-risk briefing for PR #73592sql/optimizer (from 20 merged bugfixes)


Pitfall 1 — Empty analytic operator → BE prepare() segfault

Diff anchor: RequiredPropertyDeriver.java — new branch || node.isForceMergeSort() routes the window to GatherDistributionSpec.

Historical pattern: PR#74810 ([[e:pr/StarRocks_starrocks/74810]]) — column-prune after PushDownDistinctAggregateRule produced an empty ANALYTIC operator (zero window functions) that segfaults the BE during prepare(). The fix was to prune the empty node entirely.

Risk here: The [merge_sort] hint sets forceMergeSort=true at transformer time, before optimizer rewrites. A subsequent column-pruning pass could remove all window functions from the annotated window, leaving an empty ANALYTIC node on the forceMergeSort=true execution path. No guard added in this PR protects against that combination. The statistics-driven WindowSkewToMergeSortRule fires after predicate pushdown (QueryOptimizer.java) so is less exposed, but the hint path is not.

What to verify: Can a query with [merge_sort] hint + column pruning reduce the window to zero functions? If so, the existing empty-ANALYTIC guard (from PR#74810) should also cover forceMergeSort=true windows.


Pitfall 2 — Dict-decode column identity across the forceMergeSort rewrite path

Diff anchor: DecodeRewriter.java — adds windowOp.isForceMergeSort() as a positional argument when reconstructing PhysicalWindowOperator in visitPhysicalAnalytic.

Historical pattern: PR#74977 ([[e:pr/StarRocks_starrocks/74977]]) — SplitWindowSkewToUnion rewrites produced PhysicalUnionOperator with mismatched output column ids; DecodeRewriter forwarded stale column references, crashing the BE. PR#74159 ([[e:pr/StarRocks_starrocks/74159]]) — type-mismatch in aggregate functions after dict-decode transformation also caused a BE crash.

Risk here: The new forceMergeSort flag adds a distinct execution shape (OrderedPartitionExchanger + single merged stream) that DecodeRewriter has not previously encountered. The fix (forwarding the flag) is necessary but not sufficient — verify that the column-remapping context (ColumnRefFactory, outputColumnTranslations) in visitPhysicalAnalytic is also consistent when forceMergeSort=true, and that no decode-column re-mapping is silently dropped for this new path.

What to verify: Run a query with a low-cardinality string column in the PARTITION BY or ORDER BY clause of a [merge_sort]-hinted window (or with enable_window_skew_merge_sort=true). Check that BE does not crash and results are correct.


History-backed heuristics derived from 20 merged bugfixes in sql/optimizer. Verify each pitfall against the actual change — this is a review-focus aid, not a verdict.

@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch 2 times, most recently from fbe9ad7 to 6818f5c Compare July 2, 2026 12:34
martinr0x added 2 commits July 2, 2026 14:38
Signed-off-by: m.bogusz <m.bogusz@celonis.com>
Signed-off-by: m.bogusz <m.bogusz@celonis.com>
@martinr0x martinr0x force-pushed the mbogusz/force-distributed-sort-on-hint-os branch from e89d2ff to 8282790 Compare July 2, 2026 12:39
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

[Java-Extensions Incremental Coverage Report]

pass : 0 / 0 (0%)

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

[FE Incremental Coverage Report]

pass : 86 / 100 (86.00%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/sql/analyzer/AnalyticAnalyzer.java 3 7 42.86% [169, 170, 173, 175]
🔵 com/starrocks/sql/optimizer/transformer/QueryTransformer.java 1 2 50.00% [433]
🔵 com/starrocks/planner/AnalyticEvalNode.java 2 3 66.67% [148]
🔵 com/starrocks/sql/optimizer/operator/physical/PhysicalWindowOperator.java 3 4 75.00% [171]
🔵 com/starrocks/sql/optimizer/transformer/WindowTransformer.java 6 7 85.71% [845]
🔵 com/starrocks/sql/optimizer/operator/logical/LogicalWindowOperator.java 7 8 87.50% [185]
🔵 com/starrocks/sql/ast/expression/AnalyticExpr.java 7 8 87.50% [151]
🔵 com/starrocks/sql/optimizer/rule/transformation/WindowSkewToMergeSortRule.java 38 42 90.48% [64, 81, 100, 103]
🔵 com/starrocks/sql/optimizer/RequiredPropertyDeriver.java 1 1 100.00% []
🔵 com/starrocks/qe/SessionVariable.java 4 4 100.00% []
🔵 com/starrocks/sql/optimizer/QueryOptimizer.java 3 3 100.00% []
🔵 com/starrocks/sql/optimizer/rule/implementation/WindowImplementationRule.java 1 1 100.00% []
🔵 com/starrocks/sql/optimizer/rule/RuleType.java 1 1 100.00% []
🔵 com/starrocks/sql/plan/PlanFragmentBuilder.java 3 3 100.00% []
🔵 com/starrocks/planner/SortNode.java 3 3 100.00% []
🔵 com/starrocks/sql/optimizer/rule/tree/lowcardinality/DecodeRewriter.java 1 1 100.00% []
🔵 com/starrocks/sql/optimizer/rule/transformation/SplitWindowSkewToUnionRule.java 2 2 100.00% []

@sonarqubecloud

sonarqubecloud Bot commented Jul 2, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

[BE Incremental Coverage Report]

pass : 8 / 8 (100.00%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 be/src/exec/topn_node.cpp 3 3 100.00% []
🔵 be/src/exec/analytic_node.cpp 5 5 100.00% []

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants