github-actions[bot] commented on PR #61682: URL: https://github.com/apache/doris/pull/61682#issuecomment-4123891726
## Code Review — No Blocking Issues Found Reviewed the full diff (47 files, +1349/-425). This is a well-executed mechanical refactoring with several embedded bug fixes. ### Critical Checkpoint Conclusions **1. Goal & Correctness**: The PR replaces legacy `from_date_str` methods and boolean template parameters (`IS_STRICT`, `IS_DATETIME`) with strongly-typed enums (`DatelikeParseMode`, `DatelikeTargetType`) and centralized cast functions. All replacement sites are correct — the new code preserves the original dispatch logic for every caller. **2. Modification Scope**: Focused on the enum migration and API consolidation. Also fixes several pre-existing bugs: - `from_decimal_batch` for DateV1/DateTimeV1 had `IS_STRICT=true` in the template but `params.is_strict=false` at runtime — now correctly uses `NON_STRICT` - `from_decimal` slow-dispatch overload had `TimeValue::TimeType&` parameter type (wrong for Date/DateTime context) — now correctly uses `VecDateTimeValue&` - `function_test_util.cpp` called `from_string_strict_mode<false>` with `is_strict=false` — template/runtime mismatch fixed to both use STRICT - `FieldTypeTraits<TIMESTAMPTZ>::from_string` defaulted to non-strict — now correctly uses strict mode for OLAP field parsing - Typo fix: `CastToTimstampTz` → `CastToTimestampTz` **3. Concurrency**: N/A — all changes are in stateless static functions. **4. Lifecycle Management**: N/A. **5. Configuration Items**: None added. **6. Incompatible Changes**: `from_date_str` removed from `VecDateTimeValue` and `DateV2Value<T>` public API. This is internal BE API only. **7. Parallel Code Paths**: Enum migration applied uniformly across all datelike types (Date, DateTime, DateV2, DateTimeV2, TimeV2, TimestampTz). No paths missed. **8. Special Conditional Checks**: `DCHECK(IsStrict == params.is_strict)` guards added to catch template/runtime parameter mismatches in debug builds — appropriate use of assertions. **9. Test Coverage**: New `data_type_serde_datelike_batch_test.cpp` (454 lines) covers from_string, from_float, from_decimal batch operations for all types including mixed valid/invalid rows. The mixed-batch tests (`datev1_from_decimal_batch_mixed`, `datetimev1_from_decimal_batch_mixed`) serve as regression tests for the IS_STRICT bug fix. Existing test files updated to use new API. **10. Observability**: N/A for this refactoring. **11. Performance**: Zero runtime cost — enum-to-bool conversions are `constexpr`. The `static_cast<void>(to_scale)` in `CastToDateOrDatetime::from_float` is a no-op to suppress unused parameter warning after interface unification. **12. Other Observations**: - `CastToTimestampTz` properly extracted into its own file with `friend` access to `CastToDatetimeV2`'s private `_internal` methods - The `_read_column_from_arrow` change correctly hardcodes `DATE_TIME` since `VecDateTimeValue()` defaults to `TIME_DATETIME`, matching old runtime dispatch behavior - `read_date_text_impl` / `read_datetime_text_impl` correctly use `DATE` / `DATE_TIME` respectively, matching the old `from_date_str` runtime type check - New `Int64` overloads in `io_helper.cpp` properly use `binary_cast` round-trip LGTM. -- 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]
