Uh oh!
There was an error while loading. Please reload this page.
(opt)(regression) SlotReference.shapeInfo() do not print table name - #65346
(opt)(regression) SlotReference.shapeInfo() do not print table name#65346englefly wants to merge 3 commits into
Conversation
englefly
commented
Jul 8, 2026
run buildall |
hello-stephen
commented
Jul 8, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
englefly
commented
Jul 8, 2026
/review |
There was a problem hiding this comment.
Automated review result: request changes.
Critical checkpoint conclusions:
- Goal/test: the PR tries to make explain output table-qualified by changing
SlotReference.computeToSql()globally, buttoSql()is also used as a semantic/generated name in Nereids. The current code does not safely accomplish the goal, and no new regression/unit test was added for the affected name-binding paths. - Scope/focus: the patch is small, but the behavioral surface is broad because every
SlotReference.toSql()caller now sees qualified names. The explain-only goal should be handled in display formatting or in targeted callers, not by changing the base slot SQL string globally. - Parallel paths/compatibility: set operations, recursive CTEs, and FK join elimination all use
toSql()for generated output or alias names, so this can break outer binding and change client-visible labels. - Concurrency, lifecycle, config, persistence, data writes, FE-BE protocol, and observability: not materially involved in this one-file FE expression rendering change.
- Tests/validation: static review only. I did not run FE build/checkstyle because this checkout is missing
thirdparty/installedandthirdparty/installed/bin/protoc, which the FE instructions require before building. The checkstyle issue is verified fromfe/check/checkstyle/checkstyle.xml. - User focus: no additional user-provided review focus was present.
Subagent conclusions:
optimizer-rewriteproposedOPT-REWRITE-001; I verified it as another impact ofMAIN-001and folded it into the accepted correctness inline comment.tests-session-configproposedTSC-001; I verified and accepted it asMAIN-002.- Convergence round 1 ended after the final candidate update with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same ledger/comment set.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
englefly
commented
Jul 8, 2026
run buildall |
hello-stephen
commented
Jul 8, 2026
TPC-H: Total hot run time: 29297 ms |
hello-stephen
commented
Jul 8, 2026
TPC-DS: Total hot run time: 154574 ms |
hello-stephen
commented
Jul 8, 2026
ClickBench: Total hot run time: 25.07 s |
hello-stephen
commented
Jul 8, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jul 8, 2026
FE Regression Coverage ReportIncrement line coverage |
englefly
commented
Jul 9, 2026
run buildall |
englefly
commented
Jul 9, 2026
/review |
There was a problem hiding this comment.
I found one blocking issue in the current head: the code changes shape-plan slot rendering, but the regression expected outputs that exercise that rendering were not updated.
Critical checkpoints:
- Goal/test proof: The goal appears to be stabilizing
explain shape planoutput, but current code and checked.outfiles disagree, so the tests do not prove the change. - Scope/focus: The source change is small and localized to
SlotReference.shapeInfo(). - Parallel paths: I checked hash join, nested-loop join, binary predicate, aggregate/function, and
toSql()paths. The older generated-name issue fromcomputeToSql()is resolved on the current head. - Concurrency/lifecycle/config/compatibility/persistence/data writes/FE-BE protocol: Not involved in this one-file FE display-path change.
- Validation:
git diff --checkpassed on the fetched base/head range. I did not run FE/regression tests because this checkout is missing.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc.
User focus: No additional user-provided focus was supplied.
Subagent conclusions: TSC-001 was accepted as the inline comment below. OR-001 was merged as duplicate/supporting optimizer evidence for the same issue. Convergence round 1 ended with both optimizer-rewrite and tests-session-config returning NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
| } else { | ||
| return qualifier.get(qualifier.size() - 1) + "." + name.get(); | ||
| } | ||
| return name.get(); |
There was a problem hiding this comment.
This leaves the checked shape-plan output stale. The live diff now only changes SlotReference.shapeInfo() to return the bare name, and shape-plan join text is built by PhysicalHashJoin / PhysicalNestedLoopJoin through Expression.shapeInfo() and BinaryOperator.shapeInfo(). Existing regression outputs still expect qualified slots, e.g. test_hint.out still has hashCondition=((t1.c1 = t2.c2)), and the PR body's cited multi_leading.out line still expects avg(t1.c11). With this change those direct slot references render as c1/c2, and aggregate/function arguments still go through toSql(), so the expected files need to be regenerated or the implementation needs to keep the intended qualified display behavior. Otherwise CI can keep comparing against stale shape-plan text.
hello-stephen
commented
Jul 9, 2026
TPC-H: Total hot run time: 29479 ms |
hello-stephen
commented
Jul 9, 2026
TPC-DS: Total hot run time: 180059 ms |
hello-stephen
commented
Jul 9, 2026
ClickBench: Total hot run time: 25.09 s |
englefly
commented
Jul 10, 2026
run buildall |
hello-stephen
commented
Jul 10, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jul 10, 2026
TPC-H: Total hot run time: 29442 ms |
hello-stephen
commented
Jul 10, 2026
TPC-DS: Total hot run time: 180801 ms |
hello-stephen
commented
Jul 10, 2026
ClickBench: Total hot run time: 25.06 s |
hello-stephen
commented
Jul 10, 2026
FE Regression Coverage ReportIncrement line coverage |
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)