[refactor](Expr) Refactor Expr, Function, and related classes for module isolation#61216
[refactor](Expr) Refactor Expr, Function, and related classes for module isolation#61216morrySnow wants to merge 4 commits intoapache:masterfrom
Conversation
…ule isolation - Decouple Function toThrift/toSql: create FunctionToThriftConverter and FunctionToSqlConverter, remove methods from Function/ScalarFunction/ AggregateFunction/AliasFunction - Inline FunctionName#toThrift at single call site; move FunctionName from analysis to catalog package - Move SearchDslParser and ANTLR grammars (SearchParser.g4, SearchLexer.g4) to analysis package alongside SearchPredicate - Move isExplainVerboseContext and buildFieldBindingExplainLines from SearchPredicate into ExprToSqlVisitor - Inline buildAnalyzerSqlFragment into InvertedIndexUtil; extract InvertedIndexProperties (Env-free) for MatchPredicate usage - Create NameFormatUtils in fe-common; move Utils.normalizeName there - Move SHADOW_NAME_PREFIX from SchemaChangeHandler to Column - Move FractionalFormat to org.apache.doris.common in fe-common module Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…ion, and refactor PrintableMap - Extract IndexType enum from IndexDefinition to org.apache.doris.catalog.info.IndexType in fe-catalog module. Update all files referencing IndexDefinition.IndexType. - Create BasicPrintableMap in fe-foundation with protected hooks (shouldIncludeEntry, formatValue) for subclass customization. Zero Doris-specific dependencies. - Rename PrintableMap to DatasourcePrintableMap, extending BasicPrintableMap. Sensitive key masking and hidden key filtering now use the hook pattern. - Update Index.java to use InvertedIndexProperties directly instead of InvertedIndexUtil. - Update Index.getPropertiesString() to use BasicPrintableMap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
morrySnow
force-pushed
the
expr_refactor
branch
from
94a6ef8 to
18dd1d2
Compare
- Create InvertedIndexPropertiesTest with 30 tests covering all methods including buildAnalyzerSqlFragment tests migrated from InvertedIndexSqlGeneratorTest. - Delete InvertedIndexSqlGenerator (was just a pass-through delegate). - Switch AnalyzeProperties and SinglePartitionDesc to BasicPrintableMap (no password masking needed for these use cases). - Clean up FQN references in ExprToThriftVisitor and ShowCreateFunctionCommand. - Remove deprecated normalizeName delegate from Utils. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ctionToThriftConverter - BasicPrintableMapTest: 12 tests covering null/empty map, quotation, wrapping, custom separators/delimiters, subclass hooks, and deterministic ordering. - FunctionToSqlConverterTest: 25 tests covering ScalarFunction/AggregateFunction/ AliasFunction SQL generation, IF NOT EXISTS, GLOBAL, binary types, and dispatcher. - FunctionToThriftConverterTest: 17 tests covering Thrift conversion for scalar/ aggregate functions, base fields, return type precision handling, and dispatcher. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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 is a large FE refactoring (122 files, +2285/-965) focused on module isolation: decoupling Function toThrift/toSql into converters, extracting InvertedIndexProperties, moving SearchDslParser/ANTLR grammars, moving FractionalFormat and NameFormatUtils to fe-common, and several other class relocations.
Critical Checkpoint Conclusions
1. Goal and Correctness: The stated goal is module isolation/decoupling. The refactoring is well-executed overall. The FunctionToThriftConverter and FunctionToSqlConverter are line-by-line faithful reproductions of the original instance methods. The InvertedIndexProperties extraction is clean with proper delegation. Two non-refactoring changes are included: a bug fix in BinaryPredicate.commutative() and a serialization fix adding @SerializedName("noOp") to CastExpr.
2. Modification minimality: The PR touches 122 files, but most changes are mechanical (import updates, reference updates). The core structural changes are well-scoped.
3. Concurrency: No concurrency changes. N/A.
4. Lifecycle management: No new lifecycle concerns. N/A.
5. Configuration items: No configuration changes. N/A.
6. Incompatible changes: The @SerializedName("noOp") addition to CastExpr is a serialization format change but is forward and backward compatible (Gson silently ignores unknown fields on deserialization, and missing fields use default false). This is a correct bug fix.
7. Parallel code paths: The FunctionToThriftConverter changes dispatch from virtual methods (polymorphism) to instanceof checks. This is functionally equivalent for the current class hierarchy but means future Function subclasses would need to add branches to the converter rather than overriding a method. This is acceptable for a converter pattern.
8. Special conditional checks: The EncryptKeyName.java removal of ClusterNamespace.getNameFromFullName(db) is a concern (see inline comment). The BinaryPredicate.commutative() GT->LT fix is a confirmed correct bug fix.
9. Test coverage: The PR description indicates "no need to test" as a refactor with no logic change. However, the BinaryPredicate.commutative() GT->LT change IS a logic change (bug fix) and has no unit test coverage for this method. The CastExpr @SerializedName addition is also a behavioral change for serialization round-tripping.
10. Observability: No observability changes needed. N/A.
11. Transaction/persistence: The CastExpr @SerializedName fix correctly adds persistence for the noOp field. No EditLog concerns.
12. FE-BE variable passing: No new FE-BE variables. N/A.
13. Performance: No performance concerns. The instanceof dispatch in FunctionToThriftConverter is negligible overhead.
14. Other issues: See inline comments for the EncryptKeyName concern.
| } | ||
| return ClusterNamespace.getNameFromFullName(db) + "." + keyName; | ||
| return db + "." + keyName; | ||
| } |
There was a problem hiding this comment.
Potential issue: Removing ClusterNamespace.getNameFromFullName(db) may produce incorrect output for environments upgraded from older Doris versions.
ConnectContext.currentDb can still contain a cluster prefix (e.g., "default_cluster:mydb") in certain code paths — for instance, MTMVPlanUtil.java:178 sets ctx.setDatabase(databaseIf.get().getFullName()), where Database.getFullName() returns the fullQualifiedName which may include the legacy cluster prefix. Similarly, encrypt keys deserialized from old FE images may have db persisted with the cluster prefix.
Without the stripping call, toString() would produce "default_cluster:mydb.mykey" instead of "mydb.mykey", and toSql() would produce invalid SQL like KEY default_cluster:mydb.mykey.
This may be intentional as part of the v3.0 cleanup (the Database class has a comment: "ClusterNamespace.getNameFromFullName should be removed in 3.0"), but please verify that all callers that set db have been migrated away from cluster-prefixed names, or that this change is coordinated with the broader cleanup.
TPC-H: Total hot run time: 27855 ms |
TPC-DS: Total hot run time: 153265 ms |
3 participants
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)