[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 |
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
|
…mezone-naive Arrow timestamp
DATETIME / DATETIMEV2 is a timezone-naive wall-clock type, but over Arrow Flight
/ ADBC it was mapped to a timezone-aware Arrow timestamp built from the session
timezone, so ADBC/pyarrow clients 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)
Mapping DATETIMEV2 to a timezone-naive Arrow timestamp fixes this for pyarrow.
But it changes the wire encoding for consumers that treat the value as an instant
(java.sql.Timestamp via JDBC getObject, or the Flink connector), which shifts the
wall clock on a non-UTC client. To avoid breaking those consumers, the new
behavior is gated behind a session variable, off by default:
- New session variable enable_arrow_flight_datetime_naive (default false),
plumbed via TQueryOptions to the BE result sink. When on,
get_arrow_schema_from_expr_ctxs maps DATETIMEV2 to a timezone-naive Arrow
timestamp; when off, the previous timezone-aware mapping is kept. TIMESTAMPTZ
always keeps its timezone, and other Arrow paths (Parquet export, Python UDF)
are unchanged.
- DataTypeDateTimeV2SerDe::read_column_from_arrow interprets a timezone-naive
Arrow timestamp as UTC (mirroring the write side), so a cross-cluster
(type=doris) read stays correct for both encodings.
Tests: BE unit tests asserting the Arrow field timezone for both switch states;
regression cases in arrow_flight_sql_p0 (default and switched-on) and
external_table_p0/remote_doris.
morningman
force-pushed
the
fix-adbc-datetime-timezone
branch
from
July 19, 2026 03:41
4203f0d to
7ec9e8a
Compare
Contributor
Author
|
/review |
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 from the session timezone. Clients such as pyarrow/ADBC then interpreted the value as an instant and rendered a spurious timezone offset:Mapping
DATETIMEV2to a timezone-naive Arrow timestamp fixes this for pyarrow. However, it changes the wire encoding for consumers that interpret the value as an instant (java.sql.Timestampvia JDBCgetObject(), or the Flink connector), which would shift the wall-clock value on a non-UTC client. To avoid breaking those consumers, the new behavior is gated behind a session variable, off by default.Session variable:
enable_arrow_flight_datetime_naiveSession scope, default
false:false(default): keep the previous timezone-aware Arrow mapping forDATETIME/DATETIMEV2over Arrow Flight — no behavior change for existing clients.true: mapDATETIME/DATETIMEV2to a timezone-naive Arrow timestamp carrying the wall-clock value (what pyarrow /getString()read exactly).TIMESTAMPTZalways keeps its timezone. Other Arrow paths (Parquet export, Python UDF) are unaffected. On the read side,DataTypeDateTimeV2SerDe::read_column_from_arrowinterprets a timezone-naive Arrow timestamp as UTC (mirroring the write side), so cross-cluster (type=doris) reads stay correct for both encodings.Impact by scenario
Example: value
2024-07-19 12:00:00, client/session timezone+08:00.= false)= true12:00:0012:00:0012:00:00+08:0012:00:00getString()12:00:0012:00:00getObject()/getTimestamp()(java.sql.Timestamp)12:00:0004:00:0012:00:0020:00:00type=dorisfederation (viause_arrow_flight)12:0012:00getObject()/getTimestamp()and the Flink connector shift only on a non-UTC client becausejava.sql.Timestamp(and the connector) represent the value as an absolute instant, so a timezone-less wall clock has to be anchored using the client timezone.Release note
Add session variable
enable_arrow_flight_datetime_naive(session scope, default off). When enabled,DATETIME/DATETIMEV2returned over Arrow Flight / ADBC is a timezone-naive Arrow timestamp carrying the wall-clock value, so clients no longer see a spurious timezone offset.Check List (For Author)
Test
Behavior changed:
enable_arrow_flight_datetime_naive(default off) changes the Arrow FlightDATETIMEmapping only when enabled; the default behavior is unchanged.Does this need documentation?
Check List (For Reviewer who merge this PR)