morrySnow commented on PR #61146: URL: https://github.com/apache/doris/pull/61146#issuecomment-4132076790
## Code Review — PR #61146 `[feat](join) support outer join reorder in dphyper` Reviewed with 4 models: **Gemini 3 Pro**, **GPT-5.4**, **GPT-5.3-Codex**, **Claude Opus 4.6**. 12 findings total. --- ### 🔴 Critical / High Severity #### 1. Copy-paste bug: `otherConditions` uses wrong getter **`GraphSimplifier.constructEdge()`** Both `hashConditions` and `otherConditions` call `.getHashJoinConjuncts()`. The second should be `.getOtherJoinConjuncts()`. This **silently drops non-equi predicates** and **duplicates hash predicates**, corrupting cost estimation during graph simplification. #### 2. PriorityQueue never re-heapified after key mutation **`GraphSimplifier.updatePQ()`** When `bestStep.benefit` changes on a queued `BestSimplification`, the `PriorityQueue` is not `remove()+add()`ed. Java `PriorityQueue` doesn't re-sort on in-place key mutations, so `poll()` may return a stale, non-optimal simplification step. The old hypergraph implementation (`hypergraph/GraphSimplifier.java`) explicitly reinserted entries — this v2 should do the same. #### 3. `threeRightJoin` caches cost using wrong edge **`GraphSimplifier.threeRightJoin()`** Stats are derived with `edge2`, but cost is cached with `calCost(edge1, bitmap2, bitmap3)`. `threeLeftJoin()` is consistent; this is asymmetric and incorrect. Should be `calCost(edge2, bitmap2, bitmap3)`. #### 4. `getLiteralAlias()` destructively removes entries, breaks enumeration retry **`HyperGraph.getLiteralAlias()` / `PlanReceiver.reset()`** `getLiteralAlias()` calls `nodeToLiteralAlias.remove()`, but `reset()` doesn't restore alias state. When enumeration is retried after simplification, literal aliases are lost, potentially dropping constant/derived slots needed by upper joins or the final output. #### 5. `foundEdgesContain` mutates cached BitSet by reference **`SubgraphEnumerator.EdgeCalculator.foundEdgesContain()`** Gets cached `BitSet` by reference from `containSimpleEdges`, then `.or()`s complex edges into it, **permanently corrupting** the cache entry. Fix: clone before mutating: `BitSet edgeMap = (BitSet) containSimpleEdges.get(subgraph).clone();` #### 6. `dpHypOptimize()` restores stale `rewritePlan` after memo rebuild **`Optimizer.dpHypOptimize()`** After rebuilding memo from the rewritten plan, it calls `setRewritePlan(oldRewrittenPlan)`, leaving `CascadesContext` internally inconsistent (memo no longer matches `rewritePlan`). Either keep them aligned or explicitly document the intentional divergence. #### 7. Fragile `getBestLogicalPlan` may silently discard optimization results **`Group.getBestLogicalPlan()`** If no logical expression's children match the best physical plan's children, the fallback returns the first (original, unoptimized) logical expression. DPhyper's optimized join order can be silently discarded in this path. --- ### 🟡 Medium Severity #### 8. Cost model: division-by-zero + ignoring probe cost **`GraphSimplifier.calCost()`** `1 / leftStats.getRowCount()` → `Infinity` when left table is empty. Also, the formula ignores probe-side magnitude, making cost comparisons unreliable for large probe sides. #### 9. NPE when stats are null in `GraphSimplifier` **`GraphSimplifier.calCost()`** `cacheCost.get(bitmap)` auto-unboxes null `Double` → NPE when stats were missing during initialization and `cacheCost` was not populated for that bitmap. #### 10. `nonInnerJoinedNodes` overwritten instead of accumulated **`HyperGraph.Builder.addJoin()`** Uses `=` instead of `|=` when setting `nonInnerJoinedNodes`, losing earlier non-inner join node tracking when processing sibling subtrees. Should be: `nonInnerJoinedNodes |= LongBitmap.or(leftSubtreeNodes, rightSubtreeNodes);` #### 11. Dead null-check in `DPhyperNode` constructor **`DPhyperNode(int, Group)`** `Preconditions.checkArgument(group != null)` is placed after `super(group.getLogicalExpression()...)`, which would NPE first. The check is unreachable dead code. Use `Objects.requireNonNull(group)` before the `super()` call, or remove the redundant check. #### 12. Test catches exceptions without failing **`GraphSimplifierConsistencyTest.randomTest()`** Catches `Exception`, prints diagnostics, but does not call `Assertions.fail()` or rethrow. Regressions will silently pass. Add `Assertions.fail(ex)` in the catch block. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
