[refine](function) use concrete column pointers for local result columns#63938
Merged
yiguolei merged 1 commit intoJun 2, 2026
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated code review summary for PR 63938.
Overall opinion: approval. I did not find blocking correctness, lifecycle, concurrency, or compatibility issues in the changed BE refactor paths.
Critical checkpoint conclusions:
- Goal and tests: The PR goal is to keep concrete column pointers where the concrete type is already known and remove immediate casts back from generic pointers. The changed code accomplishes that goal without apparent behavior changes. No new tests were added, which is acceptable for this narrow refactor because existing function/storage coverage should exercise these paths.
- Scope and clarity: The actual GitHub PR file list is focused on 11 BE files and the changes are small/local. The raw local two-SHA diff is noisy due to shallow/grafted history, so review was based on GitHub PR metadata and patch.
- Concurrency: The modified code does not introduce new shared state, locks, threads, or atomic/lifecycle signals. Existing function context/table-function object lifecycles are preserved.
- Lifecycle/static initialization: No new global/static cross-translation-unit initialization dependency was introduced. The DateTrunc state remains per FunctionContext THREAD_LOCAL state and uses shared_ptr-managed storage as before.
- Configuration/compatibility: No configuration, storage format, thrift/protocol, persisted metadata, or function symbol compatibility changes were introduced.
- Parallel code paths: The PR consistently applies the same concrete-pointer cleanup to the changed function, table-function, and variant/storage local result-column creation sites. I did not identify a missing parallel path required for correctness.
- Conditional checks/error handling: No new speculative defensive checks were added. Existing Status-returning paths remain checked, and no ignored Status was introduced.
- Test coverage/result files: No test result changes are part of this PR. I did not run the full build or test suite in this review.
- Observability: No new runtime behavior or distributed operation was introduced, so no additional logging or metrics appear necessary.
- Transaction/persistence/data visibility: The changes do not affect transaction processing, visible versions, delete bitmap alignment, or persistence.
- Data writes/modifications: The storage changes only adjust local column pointer types and preserve ColumnVariant insertion/padding behavior; no write atomicity or crash-recovery behavior is changed.
- FE/BE variable passing: No FE/BE transmitted variables or send paths are changed.
- Performance: The refactor removes redundant casts/assert_mutable calls and should be neutral or slightly cleaner on hot paths without adding allocations or copies.
User focus points: No additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29548 ms |
Contributor
TPC-DS: Total hot run time: 171826 ms |
yiguolei
approved these changes
Jun 2, 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
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 BE expression and storage code creates a concrete column type and then immediately casts the generic
ColumnPtrorMutableColumnPtrback to the same concrete type before writing data. This adds unnecessary casts and makes the ownership intent less direct. Root cause: several local result columns were declared as generic column pointers even though the concrete column type was already known at creation time.This PR refines those local variables to keep concrete column pointers where the type is explicit, and directly accesses the concrete column data. It also updates the explode-numbers table function member to use a concrete column pointer. The change is limited to local refactoring and does not change runtime behavior.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)