Uh oh!
There was an error while loading. Please reload this page.
[fix](fe)eliminate noop cast in thrift by overriding treeToThriftHelper - #67192
Conversation
hello-stephen
commented
Aug 27, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
starocean999
commented
Aug 27, 2026
run buildall |
morrySnow
left a comment
There was a problem hiding this comment.
add ut, test case and add intro pr number in description
| @@ -122,6 +122,15 @@ protected void treeToThriftHelper(TExpr container) { | |||
| super.treeToThriftHelper(container); | |||
| } | |||
There was a problem hiding this comment.
this override should be removed
starocean999
commented
Aug 27, 2026
run buildall |
freemandealer
commented
Aug 27, 2026
/review |
hello-stephen
commented
Aug 27, 2026
FE Regression Coverage ReportIncrement line coverage |
There was a problem hiding this comment.
Automated review completed for head 34d26c8ad899d7e2dfe500a2ffce5114ea2bd96e.
- Correctness: the visitor-aware override is the correct dispatch point for both root and nested thrift serialization. Eliding a no-op cast preserves flattened child arity and forwards visitor-specific normalization to the child.
- Tests: the new
CHAR(6) -> TEXTregression is non-vacuous and would fail on the parent implementation; theINT -> TEXTcontrol verifies that a real cast remains serialized. - Compatibility, lifecycle, performance, and security: no issue found. The change adds no mutable state, I/O, concurrency, or external-interface surface, and query-cache slot/function normalization remains intact.
- Existing review context: the earlier request to remove the redundant one-argument override is addressed in the current head.
- User focus: no additional focus was provided; the full PR was reviewed.
- Validation: the live PR head/base match the authoritative bundle,
git diff --checkis clean, and the live compile, FE UT, CheckStyle, coverage, dependency-review, and regression checks are green. No local build was run, per the review workflow.
Conclusion: no blocking issue found and no new inline comment is needed.
Uh oh!
There was an error while loading. Please reload this page.
PR approved by anyone and no changes requested. |
PR approved by at least one committer and no changes requested. |
master is ok, this bug only exists in 4.1 and 4.0
Issue Number: close #xxx
Related PR: #39670
Problem Summary:
CastExpr's treeToThriftHelper function eliminate noop cast, but there is another override version shoud be handled.
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)