[refactor](fe) Replace TFunctionBinaryType with Function.BinaryType to decouple from Thrift#61786
Conversation
…o decouple from Thrift ### What problem does this PR solve? Issue Number: close #xxx Problem Summary: Function and its subclasses were tightly coupled to the Thrift class TFunctionBinaryType. This refactoring introduces a pure-Java BinaryType enum inside Function, replaces all internal usages of TFunctionBinaryType with it, and confines Thrift conversion to FunctionToThriftConverter only. ### Release note None ### Check List (For Author) - Test: No need to test (pure refactoring, no behavioral change) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR introduces a pure-Java Function.BinaryType enum to replace the Thrift-generated TFunctionBinaryType throughout the FE codebase, confining Thrift usage to FunctionToThriftConverter. This is a clean refactoring that improves separation of concerns.
Critical Checkpoint Conclusions
Goal and correctness: The goal is to decouple FE internal code from Thrift types. The code achieves this — all 17 files are updated consistently and all TFunctionBinaryType references are successfully confined to FunctionToThriftConverter.java and its test.
Modification scope: The change is minimal and focused — pure mechanical enum type replacement with identical enum names.
Serialization/persistence compatibility (EditLog/Image): No issue. Function persists via Gson (@SerializedName("bt")). Gson serializes enums by name. The new Function.BinaryType has identical names (BUILTIN, HIVE, NATIVE, IR, RPC, JAVA_UDF, AGG_STATE, PYTHON_UDF) as TFunctionBinaryType. Existing persisted metadata will deserialize correctly. Rolling upgrade compatibility is preserved.
Concurrency: Not applicable — no concurrent data structures are modified.
Lifecycle management: Not applicable.
Configuration items: None added.
Incompatible changes: None. Thrift wire format is unchanged since toThriftBinaryType() correctly converts at the serialization boundary.
Parallel code paths: Both cloud and non-cloud paths are handled identically since this is an enum type change only.
Test coverage: Existing tests in FunctionToSqlConverterTest and FunctionToThriftConverterTest are updated. No behavioral change means existing regression tests implicitly cover correctness.
Observability: Not applicable.
Transaction/persistence: EditLog read/write paths use Gson with identical enum names — verified compatible.
FE-BE variable passing: toThriftBinaryType() correctly maps all 8 enum values at the Thrift boundary in toThriftBase().
Performance: No impact — enum comparison is the same cost.
Minor Observation
fromThriftBinaryType() is currently dead code — it has zero callers in the codebase. It may be useful for future bidirectional conversion, but per the project's convention against speculative code, consider removing it or adding a comment explaining its anticipated use.
Verdict
No blocking issues found. The refactoring is correct, backward-compatible, and well-scoped.
| /** | ||
| * Converts a Thrift {@link TFunctionBinaryType} to {@link Function.BinaryType}. | ||
| */ | ||
| public static Function.BinaryType fromThriftBinaryType(TFunctionBinaryType thriftType) { |
There was a problem hiding this comment.
Nit: fromThriftBinaryType() is currently dead code — there are zero callers in the entire codebase. The FE only converts Function.BinaryType → TFunctionBinaryType (when sending to BE), never the reverse direction. Consider either removing this method to keep the diff minimal and avoid unused code, or adding a brief comment explaining its anticipated future use.
|
Re: review comment about
|
TPC-H: Total hot run time: 26036 ms |
TPC-DS: Total hot run time: 169553 ms |
FE UT Coverage ReportIncrement line coverage |
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
5 participants
What problem does this PR solve?
Problem Summary:
Functionand its subclasses were tightly coupled to the Thrift classTFunctionBinaryType. This refactoring introduces a pure-JavaBinaryTypeenum insideFunction, replaces all internal usages ofTFunctionBinaryTypewith it, and confines Thrift conversion toFunctionToThriftConverteronly.Changes:
Function.BinaryTypeenum (pure Java, no Thrift dependency) with 8 values: BUILTIN, HIVE, NATIVE, IR, RPC, JAVA_UDF, AGG_STATE, PYTHON_UDFtoThriftBinaryType()andfromThriftBinaryType()conversion methods inFunctionToThriftConverterTFunctionBinaryTypewithFunction.BinaryTypeTFunctionBinaryTypenow only appears inFunctionToThriftConverter.java(the Thrift serialization boundary)Release note
None
Check List (For Author)