morrySnow commented on PR #62737: URL: https://github.com/apache/doris/pull/62737#issuecomment-4873318209
## Inline Review Comments ### 1. Dead code — `fe/.../RuntimeFilterContext.java` (new method around diff line 386) The new `removeRuntimeFilter(RuntimeFilter rf)` method is defined but **never called** anywhere in the codebase. Grep across the entire `fe/` directory confirms only the definition exists. If this was intended for decoupled RF cleanup when `preferDecoupled == true`, the call site is missing. If unused, it's untested dead code. ### 2. Encapsulation break — `fe/.../SessionVariable.java` (diff line 1750) `runtimeFilterWaitInfinitely` was changed from `private` to `public`. A public getter method already exists — the new code in `RuntimeFilterTranslator.setWaitTimeMs()` should use it instead of direct field access. ### 3. Redundant work — `fe/.../RuntimeFilterGenerator.java` (diff lines 508 and 563) `hasFilterInSubtree(decoupledBuilder.right())` is called at line 508 for pruning, and if it returns `true`, the same subtree is walked **again** at line 563 inside `shouldPreferDecoupledRf()`. The result from line 508 should be cached and reused. ### 4. Code clones — `fe/.../RuntimeFilterGenerator.java` (diff lines 476 and 491) `markDecoupledRfAsNonBlocking()` and `markStandardRfAsNonBlocking()` are ~85% identical — both iterate filters matching on the same four fields. Consider extracting a shared helper. ### 5. Magic number — multiple files `exprOrder == -1` is used as a sentinel for decoupled RFs in 6+ files (FE Java and BE C++). No named constant exists. Define `DECOUPLED_RF_EXPR_ORDER = -1` to document the sentinel and prevent accidental misuse. ### 6. Latent issue — `be/.../runtime_filter_producer_helper_cross.h` (diff line 229) `_init_expr` override copies all `build_expr_ctxs` into `_filter_expr_contexts` without sizing to `runtime_filter_descs.size()`. If a decoupled RF reaches a cross-join builder, `_insert()` could dereference an out-of-bounds index. Currently latent but `generateDecoupledRuntimeFilters` explicitly allows cross joins. -- 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]
