[fix](arrow-flight) Return DATETIMEV2 as a timezone-naive Arrow timestamp#65780
Open
morningman wants to merge 1 commit into
Open
[fix](arrow-flight) Return DATETIMEV2 as a timezone-naive Arrow timestamp#65780morningman wants to merge 1 commit into
morningman wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29441 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review completed for head 5c95ff786d415602c1b00ff45183587179c197b6 after two convergence rounds. Requesting changes for two P1 correctness regressions; one P3 test-lifecycle issue is also annotated inline.
Critical checkpoint conclusions:
- Goal and proof: Mapping Doris DATETIMEV2 to a timezone-naive Arrow timestamp is correct for direct Arrow Flight/ADBC clients, and the added schema, unit round-trip, Arrow Flight, and same-head Remote Doris tests prove that path. The shared implementation does not yet accomplish the goal safely for every existing caller: forced-INT96 Parquet shifts same-version values, and a head producer shifts values for an older Remote Doris consumer.
- Scope and clarity: The production edit is small and readable, but
convert_to_arrow_typeis a shared schema factory used beyond Arrow Flight. M3 shows that applying the client-facing change globally is too broad without Parquet-specific handling; M1 shows that the Remote Doris wire contract also needs version/capability scoping. - Concurrency and lifecycle: No new thread, shared mutable state, locking, static-initialization, ownership/COW, or resource-lifecycle behavior is introduced. The only lifecycle issue is M2 in the regression suite: it removes fixtures after the assertion despite the repository rule to retain them for debugging.
- Configuration: No new configuration item is added. Existing FE and BE defaults set Parquet OUTFILE
enable_int96_timestamps=true, which makes M3 a default, reachable path rather than an opt-in edge case. - Compatibility: M1 is an incompatible Arrow Flight wire-encoding change for a head-producer/base-consumer
type=dorispairing. Base/base, base/head, and head/head preserve the wall clock; head/base silently shifts it by eight hours under Doris's+08:00default. The ticket carries no encoding capability/version. - Parallel paths and conditions: Arrow Flight same-head, non-INT96 Parquet (
isAdjustedToUTC=false), Python UDF/UDAF/UDTF interchange, memory scratch, nullable and nested array/map/struct SerDes, and TIMESTAMPTZ's distinct timezone-aware branch were traced and found symmetric. Forced INT96 is the distinct failing parallel path because it erases timezone metadata before Doris reads the value with the session timezone. - Tests and expected results: Added outputs are deterministic, ordered where needed, and consistent with the intended same-head wall-clock behavior. Coverage does not exercise mixed versions. Existing
test_hive_read_parquetalready covers explicit INT96 export/read-back and its wall-clock expectations would regress under this patch. M2 should remove only the terminal cleanup; the pre-setup drops already make reruns deterministic. - Observability: No additional logs or metrics are needed for this local conversion change once correctness and compatibility are fixed.
- Transactions, persistence, data writes, and FE/BE variables: No transaction, EditLog, storage-format persistence, data-write atomicity, or new FE-to-BE variable propagation is involved. Parquet output is affected at its serialization boundary as described in M3.
- Error handling, memory safety, and performance: No new unchecked Status, exception-boundary, allocator, nullable-shape, memory-accounting, or hot-path performance issue was found.
- Release note and documentation: The release note accurately describes the intended direct Arrow Flight/ADBC behavior, but the implementation must first avoid the two inline correctness regressions. No separate user documentation requirement was identified.
- User focus:
.code-review.ClGMH5/review_focus.txtcontained no additional focus request; the whole PR was reviewed.
Validation was static, as required by the review runner; no local build or test was attempted. Clang Formatter, CheckStyle, and Linux compile checks passed when inspected. The macOS BE-UT job stopped before tests because the runner selected JDK 25 while the script requires JDK 17; Linux BE-UT was still running at submission time.
| // See apache/doris#65741. | ||
| case TYPE_DATETIMEV2: | ||
| if (type->get_scale() > 3) { | ||
| *result = std::make_shared<arrow::TimestampType>(arrow::TimeUnit::MICRO); |
Contributor
There was a problem hiding this comment.
[P1] Preserve Remote Doris compatibility with old readers
This changes the DATETIMEV2 wire encoding for every type=doris Flight query that returns that type. A head producer emits 2026-07-02 23:30:45.000000 as a timezone-empty DATETIMEV2(6) timestamp with raw value 1783035045000000; a pre-change Remote Doris reader always calls from_unixtime(..., +08:00) and returns 2026-07-03 07:30:45.000000. Old-to-new and same-version pairs work, but new-to-old silently changes data, and the Flight ticket carries no capability/version. Please negotiate the naive-DATETIMEV2 encoding (or retain the legacy encoding for old consumers) and add a mixed-version test before changing this shared schema.
Contributor
Author
There was a problem hiding this comment.
Thanks for the analysis. This is intentional, and we will not keep backward compatibility with the old encoding, because the old encoding is itself incorrect: DATETIME/DATETIMEV2 is a timezone-naive wall-clock type, so encoding it as a timezone-aware Arrow timestamp (which makes clients render a spurious +00:00 instant) is exactly the bug being fixed here.
A mixed-version type=doris federation is only a transient rolling-upgrade window. We intentionally do not add a capability/version handshake for it; once both sides are upgraded the value is correct, and we do not preserve the previous (buggy) behavior for old readers.
| } else if (type->get_scale() > 0) { | ||
| *result = std::make_shared<arrow::TimestampType>(arrow::TimeUnit::MILLI); | ||
| } else { | ||
| *result = std::make_shared<arrow::TimestampType>(arrow::TimeUnit::SECOND); |
Contributor
There was a problem hiding this comment.
[P1] Keep DATETIMEV2 correct when Parquet forces INT96
This shared branch also feeds VParquetTransformer, and Parquet OUTFILE defaults enable_int96_timestamps to true. In that mode Arrow serializes the raw timestamp integer directly to INT96 and the Parquet column has no isAdjustedToUTC marker. Head now writes DATETIMEV2(0) value 2026-07-02 01:36:22 as as-if-UTC 1782956182; Doris's INT96 readers then apply the +08 session timezone and return 09:36:22. Base wrote the +08-adjusted integer and round-tripped. Please preserve local DATETIME semantics for the forced-INT96 path (or scope the naive mapping away from it); the existing test_hive_read_parquet INT96 export/read-back expectations should remain unchanged.
| select k, dt0, dt3, dt6 from `test_remote_doris_datetime_naive_tz_catalog`.`test_remote_doris_datetime_naive_tz_db`.`t` order by k | ||
| """ | ||
|
|
||
| sql """ DROP DATABASE IF EXISTS test_remote_doris_datetime_naive_tz_db """ |
Contributor
There was a problem hiding this comment.
[P3] Keep the regression fixtures after the assertion
The suite already drops the database and catalog before setup. These final drops violate root AGENTS.md test standard 3, which requires leaving test objects in place for failure investigation. Please remove both post-test drops; the existing pre-setup cleanup already keeps reruns deterministic.
Contributor
TPC-DS: Total hot run time: 177777 ms |
Contributor
ClickBench: Total hot run time: 25.18 s |
…tamp
DATETIME / DATETIMEV2 is a timezone-naive wall-clock type, but the Arrow
Flight / ADBC result mapped it to a timezone-aware Arrow timestamp built with
the session timezone. ADBC/pyarrow clients therefore treated the value as an
instant and rendered a spurious UTC offset, e.g.
SELECT CAST('2026-07-02 01:36:22.069504' AS DATETIME(6))
-> 2026-07-02 01:36:22.069504+00:00 (wrong; should be timezone-naive)
Fix on the BE Arrow conversion path:
- convert_to_arrow_type: add a datetime_naive flag. The Arrow Flight result
schema (get_arrow_schema_from_expr_ctxs) now maps DATETIMEV2 to an Arrow
timestamp WITHOUT timezone metadata. All other callers keep the existing
timezone-aware mapping, so Parquet export (including forced INT96) and
Python UDF round-trips are unchanged. TIMESTAMPTZ always keeps its timezone.
- DataTypeDateTimeV2SerDe::read_column_from_arrow: symmetrically interpret a
timezone-naive Arrow timestamp as UTC instead of the session timezone, so a
cross-cluster (type=doris) Flight read does not shift the wall-clock value.
Tests: BE unit tests asserting the Arrow field timezone (naive only on the
Flight path, preserved otherwise and for TIMESTAMPTZ) and a non-UTC reader
round-trip; regression cases in arrow_flight_sql_p0 and
external_table_p0/remote_doris.
morningman
force-pushed
the
fix-adbc-datetime-timezone
branch
from
July 18, 2026 07:03
5c95ff7 to
4203f0d
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29789 ms |
Contributor
TPC-DS: Total hot run time: 178741 ms |
Contributor
ClickBench: Total hot run time: 25.26 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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?
Issue Number: close #65741
Problem Summary:
DATETIME/DATETIMEV2is a timezone-naive wall-clock type, but over Arrow Flight / ADBC it was mapped to a timezone-aware Arrow timestamp built with the session timezone. Clients such as pyarrow/ADBC then interpreted the value as an instant and rendered a spurious UTC offset:This PR fixes the BE Arrow conversion path:
convert_to_arrow_type: splitDATETIMEV2fromTIMESTAMPTZ.DATETIMEV2now maps to an Arrow timestamp without timezone metadata;TIMESTAMPTZkeeps its timezone because it has instant semantics.DataTypeDateTimeV2SerDe::read_column_from_arrow: symmetrically interpret a timezone-naive Arrow timestamp as UTC instead of the session timezone, so a cross-cluster (type=doris) read does not shift the wall-clock value by the session offset (the receive path defaults to+08:00).Part of the Arrow Flight / ADBC tracker #65615.
Release note
Fix Arrow Flight / ADBC returning
DATETIMEvalues as timezone-aware UTC timestamps.DATETIME/DATETIMEV2is now returned as a timezone-naive Arrow timestamp, so clients no longer see a spurious+00:00offset.Check List (For Author)
Test
Behavior changed:
DATETIME/DATETIMEV2as a timezone-naive Arrow timestamp instead of a timezone-aware one built from the session timezone.Does this need documentation?
Check List (For Reviewer who merge this PR)