You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
function_datetime_floor_ceil.cpp has excessive template instantiation bloat. The class FunctionDateTimeFloorCeil<Flag, PType, ArgNum, UseDelta> has 4 template parameters, producing 192 instantiations. However, all heavy computation functions (9 vector_* variants, time_round_two_args, floor_opt, etc.) only depend on Flag and PType — they never use ArgNum or UseDelta. This means these expensive functions are compiled 4x more than necessary.
This PR applies three optimizations:
Extract DateTimeFloorCeilCore<Flag, PType> struct: All heavy computation functions are moved into a separate struct with only 2 template parameters. FunctionDateTimeFloorCeil becomes a thin shell that delegates to Core::vector_*(). This reduces instantiations of the heaviest code from 192 → 48 (÷4).
Extract convert_utc_to_local_impl<DateValueType> free function: This function only depends on DateValueType, not Flag/ArgNum/UseDelta. Extracting it reduces its instantiations from 192 → 3.
Extract FunctionDateTimeFloorCeilBase non-template base class: Three virtual overrides (get_number_of_arguments, is_variadic, use_default_implementation_for_nulls) use zero template parameters. Moving them to a non-template base eliminates 192 redundant copies.
Release note
None
Check List (For Author)
Test
Regression test
Unit Test
Manual test (add detailed scripts or steps below)
No need to test or manual test. Explain why:
This is a refactor/code format and no logic has been changed.
Verdict: No issues found. This is a clean, well-motivated compile-time optimization refactor.
Critical Checkpoint Conclusions
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.
Modification scope: Single file, single concern. Focused and minimal. Good.
Concurrency: N/A. All functions are stateless static methods with no shared mutable state.
Lifecycle management: N/A. No special lifecycle concerns.
Configuration / Incompatible changes: N/A. No config additions, no ABI/API changes. Internal implementation detail only.
Parallel code paths: N/A. Self-contained refactor within one file.
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.
Observability / Transaction / Data writes: N/A.
FE-BE variable passing: N/A.
Performance (runtime): No runtime impact. All Core:: calls resolve to the same static functions; the indirection is compile-time only.
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
approvedIndicates a PR has been approved by one committer.reviewed
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
function_datetime_floor_ceil.cpphas excessive template instantiation bloat. The classFunctionDateTimeFloorCeil<Flag, PType, ArgNum, UseDelta>has 4 template parameters, producing 192 instantiations. However, all heavy computation functions (9vector_*variants,time_round_two_args,floor_opt, etc.) only depend onFlagandPType— they never useArgNumorUseDelta. This means these expensive functions are compiled 4x more than necessary.This PR applies three optimizations:
Extract
DateTimeFloorCeilCore<Flag, PType>struct: All heavy computation functions are moved into a separate struct with only 2 template parameters.FunctionDateTimeFloorCeilbecomes a thin shell that delegates toCore::vector_*(). This reduces instantiations of the heaviest code from 192 → 48 (÷4).Extract
convert_utc_to_local_impl<DateValueType>free function: This function only depends onDateValueType, notFlag/ArgNum/UseDelta. Extracting it reduces its instantiations from 192 → 3.Extract
FunctionDateTimeFloorCeilBasenon-template base class: Three virtual overrides (get_number_of_arguments,is_variadic,use_default_implementation_for_nulls) use zero template parameters. Moving them to a non-template base eliminates 192 redundant copies.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)