[fix](expr) fix mixed const probe constant handling regressions#63810
Open
Mryange wants to merge 2 commits into
Open
[fix](expr) fix mixed const probe constant handling regressions#63810Mryange wants to merge 2 commits into
Mryange wants to merge 2 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
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 review summary:
No blocking PR-introduced issues were found in the changed lines. Existing inline review context was empty, and there was no additional user-provided focus.
Critical checkpoint conclusions:
- Goal/test proof: The PR targets mixed-const constant handling for ColumnConst cloning, quantile_percent, regexp_count, and uniform. The added unit tests cover each targeted behavior.
- Scope: The code changes are small and focused on the described BE constant-handling paths.
- Concurrency/lifecycle: No new shared state, locking, threads, or non-trivial lifecycle management were introduced. regexp_count continues to use FunctionContext thread-local regex state.
- Configuration/compatibility/protocol: No new configs, storage formats, or FE/BE protocol changes.
- Parallel paths: The changed paths match the specific functions affected by the mixed-const probe; no additional equivalent PR-introduced path requiring the same fix was identified.
- Error handling: New production Status-returning paths are propagated through existing function execution wrappers; no unchecked production Status was introduced.
- Tests/results: Added BE unit tests cover ColumnConst clone_resized aliasing, quantile_percent const percentile preservation, regexp_count mixed const/null combinations, and uniform mixed-const execution.
- Observability/transactions/data writes: Not applicable; this PR does not add operational paths, transactions, or data-write logic.
- Performance: The new ColumnView use is in regexp_count where regex work dominates per-row access overhead; no obvious hot-path regression was found.
Focus response: No additional user focus was provided.
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Review result: no blocking findings found in the actual PR diff.
Critical checkpoint conclusions:
- Goal/test proof: The patch targets mixed constant execution regressions for ColumnConst cloning, regexp_count, quantile_percent, and uniform. The added BE unit tests cover the changed paths.
- Scope/focus: The actual PR is small and focused across BE expression/column handling plus targeted tests.
- Concurrency/lifecycle: No new concurrency, static initialization, or non-obvious lifecycle management was introduced.
- Config/compatibility/protocol: No new configuration items, storage format changes, or FE-BE protocol changes.
- Parallel paths: The changes are localized to the affected functions and ColumnConst clone behavior; no missed parallel path was identified in this review.
- Error handling/memory: No ignored Status or new untracked large allocation issue identified.
- Data correctness: The changes preserve constant column semantics and avoid sharing the nested data column after clone_resized.
- Test coverage: Targeted BE unit tests were added for the changed behavior. I did not run tests in this review environment.
- Observability/transactions/persistence: Not applicable to this PR.
User focus: No additional user-provided review focus was present.
Contributor
TPC-H: Total hot run time: 31788 ms |
Contributor
TPC-DS: Total hot run time: 172277 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
2 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?
Problem Summary:
The mixed const execution probe exposed several constant-handling problems in BE vectorized functions.
This change fixes those behaviors and adds focused unit tests for the uncovered cases.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)