Skip to content

[refine](sort) remove VSortExecExprs wrapper class#62428

Merged
HappenLee merged 4 commits intoapache:masterfrom
Mryange:remove-_materialize_tuple-use
Apr 14, 2026
Merged

[refine](sort) remove VSortExecExprs wrapper class#62428
HappenLee merged 4 commits intoapache:masterfrom
Mryange:remove-_materialize_tuple-use

Conversation

@Mryange
Copy link
Copy Markdown
Contributor

@Mryange Mryange commented Apr 13, 2026

What problem does this PR solve?

VSortExecExprs was a legacy wrapper class (ported from Impala) that held
VExprContextSPtrs _ordering_expr_ctxs along with a dead _materialize_tuple
branch. Investigation showed that FE's SortInfo.toThrift() never sets
sort_tuple_slot_exprs in TSortInfo, so _materialize_tuple is always
false and the materialization code path is unreachable dead code.

This PR removes the VSortExecExprs class entirely and replaces all usages
with direct VExprContextSPtrs.

Changes

  • Delete vsort_exec_exprs.h and vsort_exec_exprs.cpp
  • Replace VSortExecExprs& with const VExprContextSPtrs& in all sorter
    constructors (FullSorter, TopNSorter, HeapSorter, PartitionSorter)
  • Update all operator/sink files: sort_sink_operator,
    local_merge_sort_source_operator, exchange_source_operator,
    partition_sort_sink_operator, viceberg_sort_writer
  • Remove dead materialization block in Sorter::partial_sort and
    Sorter::_prepare_sort_columns
  • Update all affected unit tests

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

  • Behavior changed:

    • No.
    • Yes.

  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@Thearas
Copy link
Copy Markdown
Contributor

Thearas commented Apr 13, 2026

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented Apr 13, 2026

run buildall

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 13, 2026

BE UT Coverage Report

Increment line coverage 63.64% (35/55) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 53.03% (20135/37966)
Line Coverage 36.58% (189384/517660)
Region Coverage 32.87% (147121/447634)
Branch Coverage 33.98% (64367/189411)

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 13, 2026

BE Regression && UT Coverage Report

Increment line coverage 90.57% (48/53) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 73.69% (27397/37180)
Line Coverage 57.34% (295941/516076)
Region Coverage 54.60% (246680/451766)
Branch Coverage 56.21% (106795/189993)

@Mryange
[fix](be) Fix ordering expr prepared with wrong row descriptor in Par…
66c3397 
…titionSortSink

### What problem does this PR solve?

Issue Number: N/A

Problem Summary: In PartitionSortSinkOperatorX::prepare(), ordering expr contexts
were incorrectly prepared using _child->row_desc() instead of _row_descriptor
(output row descriptor). The ordering expressions contain slot IDs from the output
row descriptor (which may reference a full scan tuple), not the projected child row
descriptor. This caused "VSlotRef k1 have invalid slot id" errors for window
functions with PARTITION BY.

### Release note

None

### Check List (For Author)

- Test: Regression test (test_window_function) - manually reproduced and verified fix
- Behavior changed: No
- Does this need documentation: No
@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented Apr 13, 2026

run buildall

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented Apr 13, 2026

/review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 13, 2026

No issues found in this review.

Critical checkpoints:

  • Goal: The change removes the dead VSortExecExprs wrapper and replaces it with direct VExprContextSPtrs consistently across the affected BE sort paths. FE still only populates ordering_exprs, so I did not find a remaining runtime dependency on sort_tuple_slot_exprs.
  • Scope: The modification is small and focused on wrapper removal plus dead-code cleanup.
  • Concurrency: Applicable. The refactor preserves the existing model where each local state clones prepared/opened expr contexts before task-local use. I did not find a new shared-mutable-state or locking issue.
  • Lifecycle: Applicable. Expr-context creation, prepare/open, clone, and destruction semantics remain aligned with existing VExprContext ownership patterns. No static-init/lifecycle issue found.
  • Config: No new configuration items.
  • Compatibility: No FE/BE protocol, storage format, or symbol-compatibility issue found. The optional thrift field still exists, and the removed BE-only wrapper was not part of the external protocol.
  • Parallel paths: Applicable. The wrapper removal was propagated across sort sink/source, exchange merge, partition sort, local merge sort, sorter implementations, and the iceberg sort writer. I did not find a missed parallel path among the touched code.
  • Special conditions: No new non-obvious conditional branch was introduced.
  • Test coverage: Unit tests were updated in the touched sort/operator areas. I did not run them in this review, so residual risk is limited to unexecuted coverage.
  • Observability: No observability regression found; existing profiles/counters remain intact.
  • Transaction/persistence/data writes: Not applicable for most files. For the iceberg writer path, I did not find a behavioral regression in the sort-expression lifecycle.
  • FE/BE variable passing: No new transmitted variables.
  • Performance: The change removes an unused abstraction and dead branch in the sort hot path. I did not find an obvious performance regression.

Residual risk: review-only conclusion; no local build or test execution was performed.

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 13, 2026

BE UT Coverage Report

Increment line coverage 63.64% (35/55) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 53.03% (20135/37966)
Line Coverage 36.58% (189354/517660)
Region Coverage 32.83% (146966/447634)
Branch Coverage 33.98% (64354/189411)

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented Apr 13, 2026

run nonConcurrent

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 13, 2026

BE Regression && UT Coverage Report

Increment line coverage 90.57% (48/53) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 73.66% (27386/37180)
Line Coverage 57.35% (295961/516076)
Region Coverage 54.56% (246493/451766)
Branch Coverage 56.22% (106812/189993)

@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented Apr 13, 2026

run nonConcurrent

1 similar comment
@Mryange
Copy link
Copy Markdown
Contributor Author

Mryange commented Apr 13, 2026

run nonConcurrent

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Apr 13, 2026

BE Regression && UT Coverage Report

Increment line coverage 90.57% (48/53) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 73.65% (27384/37180)
Line Coverage 57.34% (295905/516076)
Region Coverage 54.54% (246397/451766)
Branch Coverage 56.20% (106783/189993)

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Apr 14, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 14, 2026

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 14, 2026

PR approved by anyone and no changes requested.

@HappenLee HappenLee merged commit 18d1558 into apache:master Apr 14, 2026
29 of 31 checks passed
@mymeiyi mymeiyi mentioned this pull request Apr 21, 2026
Merged
@Mryange Mryange mentioned this pull request Apr 27, 2026
Merged
16 tasks
yiguolei pushed a commit that referenced this pull request Apr 28, 2026
@Mryange
[branch-4.1] pick some sort pr (#62856)
4803f79 
### What problem does this PR solve?


#59207


#62428

Problem Summary:

### Release note

None

### Check List (For Author)

- Test <!-- At least one of them must be included. -->
    - [ ] Regression test
    - [ ] Unit Test
    - [ ] Manual test (add detailed scripts or steps below)
    - [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
        - [ ] Previous test can cover this change.
        - [ ] No code files have been changed.
        - [ ] Other reason <!-- Add your reason?  -->

- Behavior changed:
    - [ ] No.
    - [ ] Yes. <!-- Explain the behavior change -->

- Does this need documentation?
    - [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->

### Check List (For Reviewer who merge this PR)

- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BiteTheDDDDt BiteTheDDDDt BiteTheDDDDt approved these changes

@Gabriel39 Gabriel39 Gabriel39 approved these changes

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by one committer. dev/4.1.x dev/4.1.x-conflict reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Mryange @Thearas @hello-stephen @BiteTheDDDDt @Gabriel39 @HappenLee