[opt](build) optimize column code and move function into catalog module#61574
[opt](build) optimize column code and move function into catalog module#61574morrySnow wants to merge 1 commit intoapache:masterfrom
Conversation
morrySnow
requested review from
CalvinKirs,
dataroaring,
gavinchou,
morningman and
w41ter
as code owners
|
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 refactors FE code to move Function, ScalarFunction, AggregateFunction, FunctionName from fe-core to fe-catalog, extracts GsonUtilsBase from GsonUtils, moves utility classes (Id, IdGenerator, TreeNode, URI) to fe-common, introduces Function.BinaryType enum to decouple from Thrift, and removes dead code from Expr.java and TreeNode.java.
Critical Checkpoints
1. Goal and correctness: The stated goal is refactoring with no logic changes. However, a correctness bug was introduced in FoldConstantRuleOnBE.java (see inline comment). The AggregateFunction copy constructor improvement is a positive change.
2. Modification minimality: The PR is large (67 files, ~2200 lines changed) but mostly mechanical moves. The dead code removal and method relocation are reasonable.
3. Concurrency: No new concurrency concerns introduced.
4. Lifecycle / static initialization: GsonUtilsBase.GSON is a static final field in fe-common. GsonUtils in fe-core extends the builder and creates its own GSON. No static initialization order fiasco risk since GsonUtils.GSON_BUILDER references GsonUtilsBase.GSON_BUILDER which is in its own module dependency.
5. Configuration items: None added.
6. Incompatible changes: Function.BinaryType enum names match TFunctionBinaryType exactly, so valueOf(fn.getBinaryType().name()) conversion is safe. Serialization format uses @SerializedName on the binaryType field (bt), so GSON will serialize the new enum by name — this is compatible with old data as long as the enum names match, which they do.
7. Parallel code paths: The method relocations (e.g., extractSlots, getInputSlotRef, unwrapCast, getConjuncts) are correctly moved to their only call sites. The ScanNode.unwrapCast() is functionally equivalent to Expr.unwrapExpr(false) when implicitOnly=false.
8. Test coverage: The PR claims no tests needed due to refactoring. However, the FoldConstantRuleOnBE change is a logic bug, not just refactoring. No regression test covers the specific scenario of sibling constant sub-expressions.
9. Observability: No changes needed.
10. Transaction/persistence: Function.write() now uses GsonUtilsBase.GSON instead of GsonUtils.GSON. Since GsonUtilsBase.GSON lacks the runtime type adapters registered in GsonUtils.GSON, this could be an issue for Function subclasses that need polymorphic deserialization. However, Function serialization uses @SerializedName fields directly and doesn't appear to rely on RuntimeTypeAdapterFactory for its own hierarchy, so this is likely safe.
11. Performance: No performance issues introduced.
Issues Found
-
[CRITICAL]
FoldConstantRuleOnBE.collectConst(): ReplacingIdGenerator<ExprId>withIntegerbreaks constant folding for expressions with multiple constant siblings. JavaIntegeris immutable and passed by value — theid++insidecollectConstonly modifies the local copy. Sibling constants get the same ID, causing map key collisions and silent data loss. This can produce wrong query results. -
[MEDIUM]
ExprToSqlVisitor.visitSlotRef():expr.getSlotId() != nullandexpr.getSlotId() == nullwill throwNullPointerExceptionwhenexpr.descis null, becauseSlotRef.getSlotId()callsPreconditions.checkNotNull(desc)internally. The old code usedexpr.desc != nullwhich was a safe null check. Should useexpr.getDesc() != nullinstead.
| public static Expression foldByBE(ExpressionMatchingContext<Expression> context) { | ||
| IdGenerator<ExprId> idGenerator = ExprId.createGenerator(); | ||
| Integer idGenerator = 0; | ||
|
|
There was a problem hiding this comment.
[CRITICAL BUG] Integer is immutable in Java and passed by value. Replacing IdGenerator<ExprId> (a mutable object with shared state) with Integer breaks the ID generation logic.
When collectConst is called recursively for sibling children in the else branch (line 223), each sibling receives the same id value because the id++ on line 205 only modifies the local copy — the increment is invisible to the caller and subsequent siblings.
Example: For f(g(1), h(2)) where both g(1) and h(2) are constant:
collectConst(f(...), ..., id=0)→ not fully constant, enterselsecollectConst(g(1), ..., id=0)→ constant,id++→ localid=1, puts"1" → g(1)collectConst(h(2), ..., id=0)→ sameid=0from parent! →id++→ localid=1, puts"1" → h(2)overwriting g(1)
Result: g(1) is silently lost from the map. The replace() phase may substitute g(1)'s position with h(2)'s folded result, producing wrong query results.
Fix: Use AtomicInteger (already imported in this file!) instead of Integer:
AtomicInteger idGenerator = new AtomicInteger(0);
// ...
String id = String.valueOf(idGenerator.incrementAndGet());| && expr.desc != null) { | ||
| return expr.getLabel() + "[#" + expr.desc.getId().asInt() + "]"; | ||
| && expr.getSlotId() != null) { | ||
| return expr.getLabel() + "[#" + expr.getSlotId().asInt() + "]"; |
There was a problem hiding this comment.
[BUG] expr.getSlotId() calls Preconditions.checkNotNull(desc) internally (see SlotRef.java:130). If desc is null, this will throw NullPointerException instead of evaluating to null. The old code expr.desc != null was a safe null check.
This line and the one at line 182 (expr.getSlotId() == null) should use expr.getDesc() != null / expr.getDesc() == null instead, to preserve the original null-safety semantics.
| } | ||
| } else if (expr.desc == null) { | ||
| } else if (expr.getSlotId() == null) { | ||
| return "`" + expr.getCol() + "`"; |
There was a problem hiding this comment.
[BUG] Same issue as above: expr.getSlotId() == null will throw NullPointerException when desc is null, because SlotRef.getSlotId() internally calls Preconditions.checkNotNull(desc). The old code used expr.desc == null which was safe.
Should be: expr.getDesc() == null
TPC-H: Total hot run time: 26886 ms |
TPC-DS: Total hot run time: 168077 ms |
3 participants
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)