[opt](nerieds) PhysicalOlapScan and PhysicalFileScan should print its id in plan#62509
Merged
Merged
Conversation
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
Contributor
Author
|
/build.sh |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Summary
Requesting changes for one blocking regression and one missed parallel path.
regression-test/suites/query_p0/stats/partitionRowCount.groovynow hardcodes a statement-localAbstractPlan.id(19). That id comes fromStatementContext.getNextObjectId()/StatementScopeIdGenerator.newObjectId(), so unrelated optimizer rewrites can renumber the scan and break the case without any semantic plan change.PhysicalHudiScanstill overridestoString()and is created viaLogicalHudiScanToPhysicalHudiScan, so the Hudi file-scan path still does not print an id.
Critical Checkpoints
- Goal of task: Partially achieved.
PhysicalOlapScanand basePhysicalFileScannow printAbstractPlan.id, butPhysicalHudiScandoes not. The added regression does not prove the behavior robustly because it hardcodes a statement-local id. - Small and focused: Yes.
- Concurrency: Not applicable; these changes only affect explain-string formatting.
- Lifecycle/static init: Not applicable.
- Config changes: None.
- Compatibility: User-visible physical-plan explain text changes; acceptable if intentional, but tests must avoid brittle ids.
- Parallel code paths: Not fully covered;
PhysicalHudiScanremains on the old format. - Special conditions/comments: None.
- Test coverage: Insufficient. The only updated regression is brittle, and there is no coverage for the Hudi path.
- Test result changes: The modified expectation is not stable.
- Observability: Table name is still present in the payload, so explain readability is mostly preserved.
- Transaction/persistence/data writes/FE-BE variable passing: Not applicable.
- Performance: No meaningful impact.
- Other issues: None beyond the above.
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
Contributor
FE Regression Coverage ReportIncrement line coverage |
morrySnow
approved these changes
Apr 20, 2026
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
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?
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)