[fix](Nereids) Add shapeInfo() override in AggregateFunction to preserve table qualifier in EXPLAIN output#65264
Merged
Merged
Conversation
englefly
requested review from
924060929,
morrySnow and
starocean999
as code owners
July 6, 2026 09:49
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes. The direct aggregate qualifier fix helps the reported shape-plan case, but two shape-rendering regressions remain in the same AggregateFunction.shapeInfo() override.
Critical checkpoint conclusions:
- Goal/test: the PR aims to stabilize
EXPLAIN SHAPE PLANaggregate output. It fixes direct aggregate operands, but not wrapped aggregates, and introduces acount(*)spelling regression. - Scope: the code change is small, but the abstraction is incomplete because shape rendering still mixes
shapeInfo()andtoSql()across expression wrappers. - Concurrency/lifecycle/config/persistence/FE-BE protocol/data writes: not involved.
- Parallel paths: direct aggregate operands were updated; wrapper expression paths such as
Caststill usetoSql()and remain unfixed. - Tests: no new/updated regression output is included in the PR. I did not run tests because this checkout is missing the worktree/thirdparty prerequisites (
thirdparty/installed/bin/protoc). Static review only. - User focus: no additional user-provided review focus was supplied.
Subagent conclusions:
optimizer-rewriteproposed OPT-1, accepted as MAIN-2, and OPT-2 as a duplicate of MAIN-1.tests-session-configproposed TSC-1, merged as a duplicate source for MAIN-1.- Final convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same two-comment set.
| } | ||
|
|
||
| @Override | ||
| public String shapeInfo() { |
Contributor
There was a problem hiding this comment.
This base implementation loses the special Count(*) rendering. Count stores star-ness in the isStar flag and has zero children, then overrides computeToSql(), toString(), and toDigest() to print count(*). With this new base shapeInfo(), a Count() star aggregate never reaches those overrides: arity() is 0, the loop emits no arguments, and expression shape output such as PhysicalFilter.shapeInfo() for HAVING predicates prints count() instead of the canonical count(*). Please preserve the Count special case, for example by overriding Count.shapeInfo() or by reusing computeToSql() for the function wrapper while still calling child shapeInfo() for non-star arguments.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29342 ms |
Contributor
TPC-DS: Total hot run time: 179800 ms |
Contributor
ClickBench: Total hot run time: 25.01 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
16 tasks
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: Shape-plan predicates could lose slot qualifiers when an aggregate function was wrapped by expressions whose shapeInfo() fell back to SQL rendering. Those wrappers called child.toSql(), so expressions such as cast(avg(t1.c11) as DOUBLE) could be printed as cast(avg(c11) as DOUBLE). This change adds shapeInfo() implementations for common expression wrappers and scalar functions so shape rendering recursively preserves child shape information.
### Release note
Fix shape-plan explain output to preserve qualifiers in wrapped aggregate expressions.
### Check List (For Author)
- Test: Manual test
- Added ExpressionShapeInfoTest for wrapped aggregate shapeInfo qualifier preservation
- Ran git diff --check
- Attempted ./run-fe-ut.sh --run org.apache.doris.nereids.trees.expressions.ExpressionShapeInfoTest, but it failed before fe-core in fe-grpc generated protobuf compilation with missing OlapCommon.ColumnMessage
- Behavior changed: Yes (shape-plan explain output preserves qualifiers through wrappers)
- Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29296 ms |
Contributor
TPC-DS: Total hot run time: 179714 ms |
Contributor
ClickBench: Total hot run time: 24.93 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29337 ms |
Contributor
TPC-DS: Total hot run time: 181053 ms |
Contributor
ClickBench: Total hot run time: 25.14 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
starocean999
approved these changes
Jul 14, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
Author
|
/review |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
|
Codex automated review failed and did not complete. Error: Review step was skipped (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review completed after 3 rounds. I found three blocking shape-rendering regressions and left inline comments.
Checkpoint summary:
- Correctness/regression risk: the PR improves many wrapper paths, but special function renderers, PhysicalQuickSort detail order keys, and runtime-filter endpoints still bypass the intended shapeInfo recursion.
- Tests/coverage: existing coverage does not exercise crypto masking, high-order Lambda recursion, detail_shape_nodes='PhysicalQuickSort', or qualified runtime-filter endpoints.
- Scope/generated output: the source scope is focused and the broad .out churn is consistent with shape-plan text changes. The Count() / partial_count() regression remains current but was already raised in discussion_r3528093987, so I did not duplicate it.
- Compatibility/parallel paths: missed parallel renderers are the main issue. I did not find separate concurrency, lifecycle, config, persistence, data-write, or FE/BE protocol concerns in this PR.
- User focus: no additional user-provided review focus was supplied.
- Validation: static review only; no builds or tests were run per the review prompt.
| int arity = arity(); | ||
| for (int i = 0; i < arity; i++) { | ||
| Expression arg = child(i); | ||
| sql.append(arg.shapeInfo()); |
Contributor
There was a problem hiding this comment.
This generic renderer skips special rendering that previously came through toSql(), and there are at least two current regressions. CryptoFunction.computeToSql()/toString() intentionally mask argument 1 as '***', but AES/SM4 functions inherit this new shapeInfo() and now render the real key literal through arg.shapeInfo(). High-order functions have the same recursion gap: expressions such as array_map and array_filter/array_sort-style paths can reach a Lambda child, Lambda has no shapeInfo() override, and Lambda.computeToSql() renders its body and array arguments with toSql(), so qualifiers inside lambda expressions are still lost. Please keep the generic recursion, but add the needed shapeInfo() overrides for these special renderers and cover both cases with shape-info tests.
| return isAsc == that.isAsc() && nullFirst == that.isNullFirst() && expr.equals(that.getExpr()); | ||
| } | ||
|
|
||
| public String shapeInfo() { |
Contributor
There was a problem hiding this comment.
This helper fixes callers that use it, but the detailed sort shape path still bypasses it. PhysicalQuickSort.shapeInfo() renders detailed order keys with orderKeys.stream().map(OrderKey::toSql) when detail_shape_nodes contains PhysicalQuickSort, so a joined query sorted by qualified slots can still print ambiguous keys there even though adjacent project/filter expressions now use shapeInfo(). Please switch that call site to OrderKey::shapeInfo and add a detail_shape_nodes='PhysicalQuickSort' regression with qualified sort keys.
| --------PhysicalTopN[LOCAL_SORT] | ||
| ----------PhysicalProject | ||
| ------------hashJoin[INNER_JOIN colocated] hashCondition=((ss_items.item_id = ws_items.item_id)) otherCondition=((cast(cs_item_rev as DECIMALV3(38, 3)) <= (1.1 * ws_items.ws_item_rev)) and (cast(cs_item_rev as DECIMALV3(38, 3)) >= (0.9 * ws_items.ws_item_rev)) and (cast(ss_item_rev as DECIMALV3(38, 3)) <= (1.1 * ws_items.ws_item_rev)) and (cast(ss_item_rev as DECIMALV3(38, 3)) >= (0.9 * ws_items.ws_item_rev)) and (cast(ws_item_rev as DECIMALV3(38, 3)) <= (1.1 * cs_items.cs_item_rev)) and (cast(ws_item_rev as DECIMALV3(38, 3)) <= (1.1 * ss_items.ss_item_rev)) and (cast(ws_item_rev as DECIMALV3(38, 3)) >= (0.9 * cs_items.cs_item_rev)) and (cast(ws_item_rev as DECIMALV3(38, 3)) >= (0.9 * ss_items.ss_item_rev))) build RFs:RF13 item_id->i_item_id;RF14 item_id->i_item_id | ||
| ------------hashJoin[INNER_JOIN colocated] hashCondition=((ss_items.item_id = ws_items.item_id)) otherCondition=((cast(cs_items.cs_item_rev as DECIMALV3(38, 3)) <= (1.1 * ws_items.ws_item_rev)) and (cast(cs_items.cs_item_rev as DECIMALV3(38, 3)) >= (0.9 * ws_items.ws_item_rev)) and (cast(ss_items.ss_item_rev as DECIMALV3(38, 3)) <= (1.1 * ws_items.ws_item_rev)) and (cast(ss_items.ss_item_rev as DECIMALV3(38, 3)) >= (0.9 * ws_items.ws_item_rev)) and (cast(ws_items.ws_item_rev as DECIMALV3(38, 3)) <= (1.1 * cs_items.cs_item_rev)) and (cast(ws_items.ws_item_rev as DECIMALV3(38, 3)) <= (1.1 * ss_items.ss_item_rev)) and (cast(ws_items.ws_item_rev as DECIMALV3(38, 3)) >= (0.9 * cs_items.cs_item_rev)) and (cast(ws_items.ws_item_rev as DECIMALV3(38, 3)) >= (0.9 * ss_items.ss_item_rev))) build RFs:RF13 item_id->i_item_id;RF14 item_id->i_item_id |
Contributor
There was a problem hiding this comment.
This output still exposes another shape renderer that did not move from toSql() to shapeInfo(): the join predicates are now qualified, but the build RFs endpoints remain item_id->i_item_id. PhysicalHashJoin.shapeInfo() gets this text from RuntimeFilter.shapeInfo(), which currently formats both getSrcExpr() and targetExpression with toSql(). Please change that runtime-filter renderer to use shapeInfo() for both sides and refresh/add coverage with qualified RF endpoints.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
some regression case output are not stable, like query_p0/hint/multi_leading.groovy
After this pr, the explain output is fixed to
avg(t1.c11)Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)