[BugFix] Decouple aggregate rewrite and output dictification logic to fix multi-input aggregate bugs

[farhad-celo]

Why I'm doing:

Currently, the dictification framework uses the same logic to decide both whether an aggregate should be rewritten and whether its output should be dictified. This overlapping logic can cause bugs in aggregates with multiple inputs, such as ARRAY_AGG(NON_LOW_CARD_COLUMN ORDER BY LOW_CARD_COLUMN). This PR decouples these two pieces of logic.

What I'm doing:

• Added new fields to DecodeInfo to distinguish between string output columns and the specific aggregations being processed.
• Centralized the decision logic for whether an aggregate output is low-cardinality into checkDependsOnExpr(), making it the single source of truth for this behavior across the entire framework.

Fixes #issue

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

[github-actions]

Module risk briefing for PR #74754 — sql/optimizer (low-cardinality subsystem)

sql/optimizer — Low-cardinality / dictification type confusion

Review pitfalls

Pitfall 4 — Low-cardinality DecodeCollector / DecodeContext changes:
Changes to DecodeCollector or DecodeContext have a recurring pattern of type-confusion bugs where aggregate column IDs are misclassified. When modifying the eligibility logic in initContext, verify:

• The guard !stringRefToDefineExprMap.containsKey(aggregateId) correctly excludes all aggregate IDs that already have an explicit define expression (i.e. those whose eligibility is already handled by checkDependsOnExpr).
• COUNT / COUNT DISTINCT are the only aggregates that should fall through to the manual allStringColumns.add(aggregateId) path — confirm no other aggregate shape (e.g. MIN, MAX, SUM over a non-encoded column) can reach that branch.
• The new test covers a JOIN scenario; CTEConsume and UNNEST scenarios are not covered. Historical bugs in this area have reproduced specifically through CTE consumer projections ([[e:pr/StarRocks_starrocks/72418]]) and UNNEST element-type mismatches ([[e:pr/StarRocks_starrocks/72027]]). Consider whether the fix interacts with those shapes.

Recurring bug classes (cited fixes)

• [[e:pr/StarRocks_starrocks/74159]]: DecodeCollector.initContext produced aggregates with type-mismatch between children and function signature (e.g. max(VARCHAR) with args: INT; result: INT) — directly the same method changed here.
• [[e:pr/StarRocks_starrocks/72418]]: Dictification rewrite failed for CTEConsume projections, producing invalid plans.
• [[e:pr/StarRocks_starrocks/72027]]: UNNEST column define expressions kept ARRAY type instead of element type, causing BE crash on dictionary lookup.

from 20 merged bugfixes

---

History-backed heuristic — verify each pitfall against the actual change before acting on it.

[CelerData-Reviewer]

@codex review

[chatgpt-codex-connector]

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

Reviewed commit: 2411aa45f0

ℹ️ 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".

[CelerData-Reviewer]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c9136f3540

ℹ️ 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".

[CelerData-Reviewer]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 12b9eb1c7f

ℹ️ 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".

[CelerData-Reviewer]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6789ca090d

ℹ️ 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".

[github-actions]

[FE Incremental Coverage Report]

✅ pass : 42 / 42 (100.00%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/sql/optimizer/rule/tree/lowcardinality/DecodeInfo.java	9	9	100.00%	[]
🔵	com/starrocks/sql/optimizer/rule/tree/lowcardinality/DecodeContext.java	13	13	100.00%	[]
🔵	com/starrocks/sql/optimizer/rule/tree/lowcardinality/DecodeCollector.java	13	13	100.00%	[]
🔵	com/starrocks/sql/optimizer/rule/tree/lowcardinality/DecodeRewriter.java	7	7	100.00%	[]

[CelerData-Reviewer]

@codex review

[github-actions]

Module-risk briefing for PR #74754 (low-cardinality dictification — sql/optimizer, from 20 merged bugfixes)

sql/optimizer — Diff-aligned review pitfalls

Pitfall: Low-cardinality + CTE + UNNEST

This PR modifies DecodeContext.java and DecodeRewriter.java — the exact files flagged by two prior dictification bugs. Historical failure modes to verify against:

• CTEConsume projection dictification produced invalid plans (fixed in #72418). The AggregateRewriter changes here (new aggId field, revised visitCall and visitVariableReference logic) are in the same code path that failed for CTEConsume projections.
• UNNEST column define expressions retained ARRAY type instead of element type, causing a BE crash during dictionary lookup (fixed in #72027). The DecodeRewriter.java changes here affect how fragment string columns are propagated.

The new test file is LowCardinalityStructTest.java (struct-focused). Consider whether CTEConsume plan tests and UNNEST-element-type cases are covered alongside the new inProgressStringAggregations / finalizingStringAggregations split.

Recurring class: Low-cardinality/dictification type confusion (DecodeCollector)

DecodeCollector.java is the same hotspot where #74159 recently fixed DecodeCollector producing aggregates with a type-mismatch between argument and return type (e.g. max(VARCHAR) with args: INT; result: INT). This PR introduces a new IdentityHashMap<CallOperator, ColumnRefSet> aggFnToSupportColumns and records info.inputStringColumns per function at three visit sites (visitPhysicalTopN, visitPhysicalAnalytic, visitPhysicalHashAggregate). Verify that the support-column snapshot captured at local-stage visit time correctly reflects what the global-stage rewriter will see — particularly for split aggregates where inputStringColumns differs between the two phases.

---

History-backed heuristic from the sql/optimizer bugfix record — verify each point against the actual change.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfed010bbe

ℹ️ 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".

[github-actions]

Module-risk briefing for PR #74754 — history-grounded review focus for the sql/optimizer low-cardinality decode path

---

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

Pitfall: Low-cardinality/dictification type confusion · [[e:pr/StarRocks_starrocks/74159]] [[e:pr/StarRocks_starrocks/72418]] [[e:pr/StarRocks_starrocks/72027]]

The most recent fix in this exact area (PR #74159) was about DecodeCollector producing aggregate function calls with a type-mismatch between argument types and the call signature (e.g. max(VARCHAR) with args: INT; result: INT). This PR makes substantial changes to the same tracking logic in DecodeCollector — migrating from aggIdToSupportColumns (int-keyed HashMap) to aggFnToSupportColumns (identity-keyed IdentityHashMap<CallOperator, ColumnRefSet>).

Key risk to verify:

• aggFnToSupportColumns.put(value, ...) is called in three separate visitPhysical* paths (visitPhysicalTopN, visitPhysicalAnalytic, visitPhysicalHashAggregate). Any aggregate function instance that ends up in stringAggregateExprs but not in aggFnToSupportColumns will hit the Preconditions.checkNotNull(supportColumns) in rewriteStringAggregations — confirm no code path adds to stringAggregateExprs without a matching aggFnToSupportColumns.put.
• The buildAggregateFunction change in AggregateRewriter now checks fieldStringRef != null && supportColumns.contains(fieldStringRef) instead of merely fieldMapping.containsKey("col" + (i+1)). This is more conservative and correct (avoids dictifying struct fields that aren't string-typed), but confirm this doesn't silently under-dictify when a struct field IS a valid string candidate but happens to be absent from supportColumns due to how the new inProgressStringAggregations/finalizingStringAggregations split propagates.

Pitfall: Low-cardinality + CTE + UNNEST · [[e:pr/StarRocks_starrocks/72418]] [[e:pr/StarRocks_starrocks/72027]]

This PR modifies both DecodeRewriter.java and DecodeContext.java. Historically, changes to these files without CTEConsume plan coverage have produced invalid plans: PR #72418 found that dictification fails for CTEConsume projections, and PR #72027 found that UNNEST column define-expressions retained ARRAY type instead of element type, causing a BE crash on dictionary lookup.

The new tests (testArrayAggNonLowCardStringInput, testDecodeInMiddle) exercise JOIN and multi-aggregate scenarios but do not cover CTEConsume or UNNEST paths. The fragmentUsedDictExprs propagation in DecodeRewriter.rewriteImpl now additionally unions inProgressStringAggregations and finalizingStringAggregations — verify that CTE consumer operators still see the correct dictified column set when in-progress aggregations cross a CTE boundary.

---

This is a history-backed heuristic derived from past bugfixes in the same module — verify each point against the actual change before acting on it.

[github-actions]

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

[github-actions]

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)