Doris-Breakwater commented on issue #67447: URL: https://github.com/apache/doris/issues/67447#issuecomment-5509518382
Breakwater-GitHub-Analysis-Slot: slot_a88666d37e90 ## Initial assessment **Confirmed correctness bug (high confidence from the code at `ddbaaab1388`; I did not independently start a cluster to rerun the SQL).** The issue is currently open with no labels, assignee, linked PR, or milestone. This should be routed to the BE file-scan/Parquet owners. The impact is silent value loss on a normal read: a conversion failure is materialized as `NULL` in the permissive nullable scan path, although the Parquet payload itself remains readable by another engine. ## Verified facts and root cause - Doris `DATETIMEV2` represents `0000-01-01` as a valid minimum: `MIN_DATE_V2`/`MIN_DATETIME_V2` encode year 0, and `vdatetime_value_test.cpp` explicitly verifies that day number 1 maps to `0000-01-01`. - The Parquet writer path accepts this value. `DataTypeDateTimeV2SerDe::write_column_to_arrow()` converts it to an Arrow timestamp without a year-1 range check, and `VParquetTransformer` writes that Arrow batch. The reporter's Spark result is also evidence that the written payload is not corrupt. - Both native Parquet materialization and the decoded-value path call `parquet_timestamp_micros()` / `parquet_int96_timestamp_micros()` before constructing a `DATETIMEV2`. The shared `MIN_DORIS_TIMESTAMP_MICROS` is `0001-01-01`, so every year-zero value fails before the target type can validate it. `ParquetMaterializationState::mark_conversion_failure()` then explains the observed `NULL` rather than a query error. - This guard was introduced in `a8b1fd95e2a` (`#65674`) with the native FileScannerV2 Parquet path. The same change also added the check to the pre-existing decoded-value conversion. Existing tests exercise the year-1 boundary for `TIMESTAMPTZ`, but there is no corresponding year-zero `DATETIMEV2` boundary test. The root cause is therefore a shared pre-materialization range check that conflates two different target contracts: `DATETIMEV2` permits year 0, while `TIMESTAMPTZ` retains a year-1 minimum. ## Important implementation caveats A one-line change from `0001-01-01` to `0000-01-01` is the right direction for `DATETIMEV2`, but it is not sufficient as a general fix: 1. For `isAdjustedToUTC=false`, `append_datetimev2_from_epoch_micros()` has a second boundary problem. At the proposed minimum, `days_since_epoch == -719528`, while `calc_daynr(1970, 1, 1) == 719528`; the computed `daynr` is 0 and is rejected. The exact minimum therefore still cannot materialize through that path after only widening the shared constant. 2. For `isAdjustedToUTC=true`, the raw value is an instant and the valid raw boundary depends on the target timezone. A valid local `0000-01-01` can lie outside a fixed UTC year-zero bound after offset conversion. Final civil-range validation should account for the timezone rather than treating the raw instant as a timezone-independent Doris civil value. 3. The helper is also used by `DataTypeTimeStampTzSerDe`. Widening a shared constant must not accidentally widen the `TIMESTAMPTZ` contract; its own post-conversion `is_valid_date()` currently enforces the year-1 minimum, but this should be explicit and tested. The safer design is to keep unit scaling/overflow and malformed-INT96 checks format-level, then apply the range appropriate to the target type and timestamp semantics during/after materialization. Rejecting year-zero on write would make Parquet behavior inconsistent with Doris's implemented and accepted `DATETIMEV2` domain and would discard an interoperable payload, so fixing the read side is preferable. ## Missing information These details are not blockers for confirming the defect, but they are needed to pin down the exact integration coverage: - The complete `local()` TVF query, including any explicit column mapping/types. - `SHOW VARIABLES LIKE 'enable_file_scanner_v2'` and the session `time_zone` used for both OUTFILE and readback. - The written Parquet field metadata (physical type, timestamp unit, and `isAdjustedToUTC`) from `parquet-tools schema/meta` or an equivalent inspector. ## Recommended next steps 1. Add a failing SerDe test for `DATETIMEV2(6)` covering `0000-01-01`, `0000-03-01`, the value immediately below the valid minimum, and the existing upper boundary. Cover adjusted and unadjusted timestamps; retain a separate assertion that `TIMESTAMPTZ` rejects year 0. 2. Cover plain and dictionary materialization, plus the raw-predicate conversion path, because all use the same timestamp helpers but have different failure/null propagation. 3. Add the reported OUTFILE -> `local()` round trip as a regression with FileScannerV2 enabled (the default), and compare with the legacy scanner. Run it in UTC and at least one non-UTC timezone to catch offset-boundary errors. 4. If the shared helper is split or made target-aware, also cover INT96 and millis/micros/nanos so overflow protection from `#65674` is preserved. -- 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]
