[refactor](fe) Extract toThrift from descriptor classes into DescriptorToThriftConverter by morrySnow · Pull Request #62312 · apache/doris

morrySnow · 2026-04-10T03:55:11Z

What problem does this PR solve?

Problem Summary:
The toThrift() methods on SlotDescriptor, TupleDescriptor, and DescriptorTable mix serialization logic with domain model classes. Following the same pattern as FunctionToThriftConverter, this PR extracts all three toThrift() methods into a new DescriptorToThriftConverter utility class, keeping the domain classes focused on their core responsibilities.

Changes:

Created DescriptorToThriftConverter as a final utility class with private constructor and 3 static toThrift overloads
Created DescriptorToThriftConverterTest with 17 unit tests covering all code paths
Updated all 11 call sites across 7 files to use DescriptorToThriftConverter.toThrift()
Removed toThrift() from SlotDescriptor, TupleDescriptor, and DescriptorTable
Added necessary getters: getMaterializedColumnName(), getThriftDescTable(), getTupleDescs()
Cleaned up unused imports (TSlotDescriptor, TTupleDescriptor, TableIf, Logger/LogManager)

Release note

None

Check List (For Author)

Test: Unit Test (DescriptorToThriftConverterTest with 17 test cases)
Behavior changed: No
Does this need documentation: No

…orToThriftConverter ### What problem does this PR solve? Issue Number: close #xxx Problem Summary: The toThrift() methods on SlotDescriptor, TupleDescriptor, and DescriptorTable mix serialization logic with domain model classes. Following the same pattern as FunctionToThriftConverter, this PR extracts all three toThrift() methods into a new DescriptorToThriftConverter utility class, keeping the domain classes focused on their core responsibilities. ### Release note None ### Check List (For Author) - Test: Unit Test (DescriptorToThriftConverterTest with 17 test cases) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

morrySnow · 2026-04-10T03:55:20Z

run buildall

morrySnow · 2026-04-10T03:55:20Z

/review

hello-stephen · 2026-04-10T03:55:26Z

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:

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

github-actions

1 finding:

fe/fe-core/src/main/java/org/apache/doris/analysis/DescriptorTable.java: getTupleDescs() is a new public accessor that returns the live tupleDescs.values() view. External callers can mutate it via remove()/clear() without updating slotDescs or the id generators, which can leave DescriptorTable internally inconsistent. Because the new converter lives in the same package, this helper should stay package-private or return an unmodifiable view.

Critical checkpoint conclusions:

Goal of task: Partially accomplished. The Thrift conversion logic was extracted and the visible call sites were migrated, but the refactor introduced the encapsulation regression above.
Small, clear, focused change: Not fully. The refactor widens DescriptorTable's public API unnecessarily.
Concurrency: Not applicable for the touched code; no new thread-safety issues were introduced.
Lifecycle/static initialization: No special lifecycle or static initialization issues found.
Configuration: No configuration changes.
Compatibility/incompatible change: No FE-BE protocol or storage compatibility change found.
Functionally parallel code paths: The call sites changed in this PR were updated consistently.
Special conditional checks: No new risky conditional logic was introduced.
Test coverage: Added unit tests cover the main Thrift conversion paths, but they do not protect against the new mutable API exposure.
Observability: Not applicable.
Transaction/persistence: Not applicable.
Data writes/modifications: Not applicable.
FE-BE variable passing: Not applicable.
Performance: No material performance regression found in the converter logic.
Other issues: No additional concrete correctness issues found beyond the public mutable accessor.

github-actions · 2026-04-10T03:58:56Z

fe/fe-core/src/main/java/org/apache/doris/analysis/DescriptorTable.java

-            result.addToTableDescriptors(tbl.toThrift());
-        }
-        thriftDescTable = result;
-        return result;


getTupleDescs() exposes a live tupleDescs.values() view as public API. That collection supports remove()/clear(), so external callers can now mutate DescriptorTable without updating slotDescs or the ID generators, leaving the object internally inconsistent. Since DescriptorToThriftConverter is in the same package, this helper can stay package-private, or it should at least return an unmodifiable view.

hello-stephen · 2026-04-10T05:21:22Z

FE UT Coverage Report

Increment line coverage 88.52% (54/61) 🎉
Increment coverage report
Complete coverage report

… path types with Java equivalents ### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#62312 Problem Summary: 1. Remove TDescriptorTable cache from DescriptorTable - the thriftDescTable field, getThriftDescTable(), and setThriftDescTable() are removed. DescriptorToThriftConverter now always regenerates the Thrift representation. 2. Create Java equivalent classes (ColumnAccessPath, ColumnAccessPathType) to replace Thrift types (TColumnAccessPath, TAccessPathType, TDataAccessPath, TMetaAccessPath) used directly in domain code. The Thrift types are now confined to DescriptorToThriftConverter, which handles the conversion. DataAccessPath and MetaAccessPath are merged into a single ColumnAccessPath class since both are structurally identical (List<String> path). Type discrimination is via the ColumnAccessPathType enum (DATA, META). ### Release note None ### Check List (For Author) - Test: Unit Test (DescriptorToThriftConverterTest - 22 tests) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

morrySnow · 2026-04-10T06:04:50Z

run buildall

morrySnow · 2026-04-10T06:04:58Z

/review

github-actions · 2026-04-10T06:09:39Z

No issues found in this review.

Critical checkpoints:

Goal and correctness: The PR cleanly extracts descriptor-to-Thrift serialization into DescriptorToThriftConverter, updates the FE call sites consistently, and preserves behavior based on the reviewed call chains.
Scope/minimality: The change stays focused on FE descriptor serialization and access-path type cleanup without unrelated logic changes.
Concurrency/lifecycle/config/compatibility: No new concurrency, lifecycle, config, persistence, or rolling-upgrade compatibility concerns were introduced.
Parallel paths: The relevant FE serialization paths and nested-column/variant access-path plumbing were updated consistently.
Conditional checks/observability: No new risky conditional branches or observability gaps stood out.
Testing: Added or updated FE unit tests cover the converter and nested/variant pruning paths. Local Maven validation in this runner was blocked because fe-core depends on the unpublished snapshot artifact org.apache.doris:fe-foundation:1.2-SNAPSHOT.

Residual risk: META access-path handling still appears dormant in the current pruning logic, so if that path is enabled later it should get dedicated behavioral tests.

github-actions · 2026-04-10T09:57:28Z

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

github-actions · 2026-04-10T09:57:31Z

PR approved by anyone and no changes requested.

…orToThriftConverter (apache#62312) Problem Summary: The `toThrift()` methods on `SlotDescriptor`, `TupleDescriptor`, and `DescriptorTable` mix serialization logic with domain model classes. Following the same pattern as `FunctionToThriftConverter`, this PR extracts all three `toThrift()` methods into a new `DescriptorToThriftConverter` utility class, keeping the domain classes focused on their core responsibilities. **Changes:** - Created `DescriptorToThriftConverter` as a `final` utility class with private constructor and 3 static `toThrift` overloads - Created `DescriptorToThriftConverterTest` with 17 unit tests covering all code paths - Updated all 11 call sites across 7 files to use `DescriptorToThriftConverter.toThrift()` - Removed `toThrift()` from `SlotDescriptor`, `TupleDescriptor`, and `DescriptorTable` - Added necessary getters: `getMaterializedColumnName()`, `getThriftDescTable()`, `getTupleDescs()` - Cleaned up unused imports (`TSlotDescriptor`, `TTupleDescriptor`, `TableIf`, `Logger/LogManager`)

github-actions bot reviewed Apr 10, 2026

View reviewed changes

starocean999 approved these changes Apr 10, 2026

View reviewed changes

github-actions bot added approved Indicates a PR has been approved by one committer. reviewed labels Apr 10, 2026

924060929 approved these changes Apr 10, 2026

View reviewed changes

924060929 merged commit 795a4d0 into apache:master Apr 10, 2026
29 of 31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor](fe) Extract toThrift from descriptor classes into DescriptorToThriftConverter#62312

[refactor](fe) Extract toThrift from descriptor classes into DescriptorToThriftConverter#62312
924060929 merged 2 commits intoapache:masterfrom
morrySnow:slot-thrift

morrySnow commented Apr 10, 2026

Uh oh!

morrySnow commented Apr 10, 2026

Uh oh!

morrySnow commented Apr 10, 2026

Uh oh!

hello-stephen commented Apr 10, 2026

Uh oh!

github-actions bot left a comment

Uh oh!

github-actions bot Apr 10, 2026

Uh oh!

hello-stephen commented Apr 10, 2026

Uh oh!

morrySnow commented Apr 10, 2026

Uh oh!

morrySnow commented Apr 10, 2026

Uh oh!

github-actions bot commented Apr 10, 2026

Uh oh!

github-actions bot commented Apr 10, 2026

Uh oh!

github-actions bot commented Apr 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

morrySnow commented Apr 10, 2026

What problem does this PR solve?

Release note

Check List (For Author)

Uh oh!

morrySnow commented Apr 10, 2026

Uh oh!

morrySnow commented Apr 10, 2026

Uh oh!

hello-stephen commented Apr 10, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

hello-stephen commented Apr 10, 2026

FE UT Coverage Report

Uh oh!

morrySnow commented Apr 10, 2026

Uh oh!

morrySnow commented Apr 10, 2026

Uh oh!

github-actions bot commented Apr 10, 2026

Uh oh!

github-actions bot commented Apr 10, 2026

Uh oh!

github-actions bot commented Apr 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants