github-actions[bot] commented on PR #61515: URL: https://github.com/apache/doris/pull/61515#issuecomment-4116740254
## Code Review Summary **Verdict: No issues found.** This is a clean, well-motivated compile-time optimization refactor. ### Critical Checkpoint Conclusions 1. **Goal**: Reduce template instantiation bloat in `function_datetime_floor_ceil.cpp`. The three extractions (core computation struct, free function, non-template base class) correctly reduce instantiations as claimed. **Achieved.** 2. **Modification scope**: Single file, single concern. Focused and minimal. **Good.** 3. **Concurrency**: N/A. All functions are stateless static methods with no shared mutable state. 4. **Lifecycle management**: N/A. No special lifecycle concerns. 5. **Configuration / Incompatible changes**: N/A. No config additions, no ABI/API changes. Internal implementation detail only. 6. **Parallel code paths**: N/A. Self-contained refactor within one file. 7. **Test coverage**: Existing regression tests for datetime floor/ceil functions cover correctness. Pure mechanical extraction with no logic changes — "no need to test" is reasonable. 8. **Observability / Transaction / Data writes**: N/A. 9. **FE-BE variable passing**: N/A. 10. **Performance (runtime)**: No runtime impact. All `Core::` calls resolve to the same static functions; the indirection is compile-time only. 11. **Other observations**: The `DateTimeFloorCeilCore` struct exposes previously-`private` methods as `public`, but since the struct is internal to a `.cpp` file and never exposed in a header, this has zero impact. ### Verification Details - All 9 `vector_*` call sites in `execute_impl` correctly delegate to `Core::` with identical arguments. - `convert_utc_to_local_impl` is functionally identical to the original `convert_utc_to_local`. - `FunctionDateTimeFloorCeilBase` correctly inherits from `IFunction` and provides the three overrides. Pure virtual methods (`get_name`, `execute_impl`) are still implemented by the concrete `FunctionDateTimeFloorCeil` subclass. - `DateTimeFloorCeilCore` uses only `Flag` and `PType` template parameters — confirmed that `ArgNum`/`UseDelta` are never referenced in the moved methods. - No naming collisions with existing `convert_utc_to_local` member function on `TimestampTzValue` (different function, different signature). -- 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]
