github-actions[bot] commented on PR #61227: URL: https://github.com/apache/doris/pull/61227#issuecomment-4044665051
## Code Review Summary **PR**: [env](compiler) reduce template instantiation in scan_operator to improve compile time ### Overall Assessment: LGTM - Clean refactoring, no correctness issues found. ### Critical Checkpoint Conclusions **1. Goal & Correctness**: The PR moves 9 methods (5 non-template, 4 PrimitiveType-templated) from `ScanLocalState<Derived>` to `ScanLocalStateBase` to reduce template instantiation from N(Derived)×M(PrimitiveType) to just M. The refactoring preserves identical behavior — verified that all moved methods only use base class members, virtual dispatch, or `_parent` (accessible via `PipelineXLocalStateBase`). Compile time improvement from ~289s to ~210s is significant. **2. Modification scope**: Minimal and focused — only 2 files changed (header and source), with a net reduction of 7 lines. All changes are mechanical: method signatures changed from `ScanLocalState<Derived>::` to `ScanLocalStateBase::`, members moved up the hierarchy, and one `Derived::Parent` cast replaced with direct `_parent->node_id()`. **3. Concurrency**: `_eos` (std::atomic<bool>) was already thread-safe — just moved to base class with same declaration. No new concurrency patterns introduced. **4. Virtual dispatch correctness**: All 8 virtual methods moved to `ScanLocalStateBase` have proper default implementations. Verified that all 3 subclasses with overrides (`OlapScanLocalState` 8/8, `FileScanLocalState` 7/8, `MockScanLocalState` 6/8) use the `override` keyword, so the compiler will catch any signature mismatch. The remaining 4 subclasses correctly inherit the default `UNACCEPTABLE` behavior. **5. Member initialization**: `_max_pushdown_conditions_per_column` is copied from parent in `ScanLocalState<Derived>::init()` (line 144), which runs before `open()` where normalize methods are called. The default value of 1024 matches the parent's default. Correct. **6. Template instantiation**: The PrimitiveType-templated methods on `ScanLocalStateBase` are defined in the same `.cpp` file as `ScanLocalState<Derived>::_normalize_predicate` which calls them via `std::visit`. The explicit class instantiations at the bottom of the file will trigger implicit instantiation of these base class template methods. No additional explicit instantiations needed. **7. `_parent->node_id()` replacement**: The old code cast to `Derived::Parent` then called `node_id()`. Since `node_id()` is a virtual method on `OperatorXBase` (the static type of `_parent`), calling it directly is semantically identical and correct. **8. Parallel code paths**: No other code paths affected — the methods remain in the same call chain, just at a different level in the class hierarchy. **9. Performance**: This is purely a compile-time optimization. Runtime behavior is unchanged — the virtual method table adds negligible overhead since these methods were already virtual (declared virtual in the old `ScanLocalState<Derived>` template class). The only minor pre-existing issue noticed is `_should_push_down_binary_predicate` taking `const std::set<std::string> fn_name` by value rather than by const reference, but this is not introduced by this PR. **10. Test coverage**: This is a pure refactoring with no logic changes. The claim of no behavior change is credible given the analysis above. Existing regression tests cover the functional paths. -- 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]
