[fix](arrow-flight) Return DATETIMEV2 as a timezone-naive Arrow timestamp#65780
Open
morningman wants to merge 3 commits into
Open
[fix](arrow-flight) Return DATETIMEV2 as a timezone-naive Arrow timestamp#65780morningman wants to merge 3 commits into
morningman wants to merge 3 commits 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 |
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Request changes: four correctness/compatibility defects and two material coverage gaps remain.
Critical checkpoint conclusions:
- Data correctness: blocked. The aware read branch ignores Arrow's advertised timezone, the new naive round-trip corrupts supported fractional timestamps before 1970, and reachable legacy DATETIME values do not implement the advertised timestamp wire type.
- Compatibility and FE-BE parameter passing: the variable is forwarded and reaches both all-new sink modes, but mixed-version result BEs can emit unequal schemas and fail a default-parallel query. This is distinct from the already-discussed cross-cluster old-reader behavior.
- Configuration, persistence, and error handling: the default-off session definition and Thrift serialization are internally consistent; no additional persistence or validation defect was found. The unresolved mixed-schema behavior is not negotiated or rejected before execution.
- Concurrency, lifecycle, ownership, and performance: no new defect found. Sender sharing and schema construction remain bounded to existing result-sink lifecycle, and recursive type mapping adds no material hot-path cost.
- Tests: blocked. UTC CI can skip the only direct wire distinction, the serial sink is not covered, the Remote Doris suite never enables naive mode on its separate producer session, and no legacy DATETIME case fixes the public contract. Determined regression expectations should also follow the repository's generated
qt/.outconvention. - Scope and external callers: current-head default arguments preserve Parquet/Python callers, TIMESTAMPTZ remains aware, and nested mappings propagate the option. The existing Parquet INT96 and fixture-cleanup threads were not duplicated.
- Observability and documentation: no new logging/profile regression was found. The author already marks documentation as required; the session-variable documentation remains to be tracked before merge. No additional user review focus was supplied.
Validation was static only because the review contract forbids builds. Formatting/checkstyle are green. The macOS BE-UT failure is a runner JDK-version setup failure rather than code evidence; the main Doris compile/UT jobs were still pending at review time.
| // stores its value as an as-if-UTC epoch, so it must be read back with UTC. | ||
| // Interpreting it with the session timezone would shift the wall-clock value for | ||
| // cross-cluster (type=doris) reads. See apache/doris#65741. | ||
| const cctz::time_zone& real_ctz = type->timezone().empty() ? cctz::utc_time_zone() : ctz; |
Contributor
There was a problem hiding this comment.
[P1] Decode aware timestamps in their advertised timezone
This branch still discards non-empty Arrow timezone metadata. A producer session in UTC writes 2026-07-02 01:36:22 as raw 1782956182 with tz=UTC, but both Remote Doris readers pass the hard-coded +08:00, so this selects ctz and materializes 09:36:22. This is distinct from the existing old-reader/naive compatibility thread: it reproduces with the default aware encoding and current code on both ends. Please resolve type->timezone() for the aware case and add a producer/reader-timezone mismatch test.
| DateV2Value<DateTimeV2ValueType> v; | ||
| // convert second | ||
| v.from_unixtime(utc_epoch / divisor, ctz); | ||
| v.from_unixtime(utc_epoch / divisor, real_ctz); |
Contributor
There was a problem hiding this comment.
[P1] Preserve negative epochs in the new naive round-trip
The new test only covers positive generated values. For valid DATETIMEV2(3) '1969-12-31 23:59:59.500', the naive writer emits signed -500, but the reader casts that to UInt64 before division and remainder, so it passes an enormous positive second count to from_unixtime and derives 116000 rather than 500000 microseconds. Keep the raw epoch signed with floor division/normalized remainder (or reuse the signed epoch-micros helper), and add fractional pre-1970 round-trip cases here.
| // When true, DATETIME/DATETIMEV2 returned over Arrow Flight is mapped to a timezone-naive | ||
| // Arrow timestamp (carrying the wall-clock value); when false (default), the previous | ||
| // timezone-aware mapping is kept. See apache/doris#65741. | ||
| 1005: optional bool enable_arrow_flight_datetime_naive = false |
Contributor
There was a problem hiding this comment.
[P1] Gate naive schemas during rolling BE upgrades
Field 1005 is optional, so an old BE ignores true and keeps timestamp[..., tz=...], while a new BE emits timestamp[...]. With the default parallel result sink, FE registers an endpoint per result BE and FlightSqlConnectProcessor.fetchArrowFlightSchema() rejects schemas that are not exactly equal, so a query spanning old and new BEs fails before results can be fetched (and an all-old placement silently ignores the request). This is distinct from the existing cross-cluster Remote Doris thread. Please gate/negotiate the option across all scheduled result BEs, or reject it explicitly while the cluster is mixed-version.
| // The switch changes the wire encoding. On a non-UTC client, the timezone-aware (off) and | ||
| // timezone-naive (on) values differ when read as java.sql.Timestamp via getObject(). This also | ||
| // confirms the SET actually took effect over Arrow Flight. | ||
| if (java.util.TimeZone.getDefault().getRawOffset() != 0) { |
Contributor
There was a problem hiding this comment.
[P2] Make the end-to-end wire assertion deterministic
On a UTC JVM this block is skipped, while the unconditional getString() value is identical for aware and naive timestamps. The suite therefore still passes if SessionVariable.toThrift() or the parallel sink argument is removed. It also never sets enable_parallel_result_sink=false, so the serial sink branch is untested. Please inspect the returned Arrow field timezone (or force and restore a controlled non-UTC client timezone) and exercise both sink modes.
| ); | ||
| """ | ||
|
|
||
| qt_sql """ |
Contributor
There was a problem hiding this comment.
[P2] Enable naive mode in the producer session
This catalog query opens a separate authenticated Flight SQL session, and RemoteDorisScanNode injects only the parallel result-sink hint. Since the new variable defaults false, this assertion exercises only the pre-existing timezone-aware encoding; it can pass without the naive Remote Doris path that the suite comments claim to cover. Please arrange for the producer Flight query itself to enable enable_arrow_flight_datetime_naive=true and assert that round-trip (including the serial sink if it is supported).
| case TYPE_DATETIMEV2: | ||
| case TYPE_TIMESTAMPTZ: { | ||
| const std::string empty_timezone; | ||
| const bool naive = type->get_primitive_type() == TYPE_DATETIMEV2 && datetime_naive; |
Contributor
There was a problem hiding this comment.
[P2] Handle legacy DATETIME or narrow the option contract
The public SessionVariable/Thrift descriptions promise timezone-naive Arrow timestamps for both DATETIME and DATETIMEV2, but this condition only changes TYPE_DATETIMEV2; TYPE_DATETIME remains in the arrow::utf8() cases and its SerDe writes through StringBuilder. Explicit datetimev1 columns (and FE paths with date conversion disabled) therefore ignore the advertised wire-type behavior. Please either add matching timestamp schema/writer support for legacy DATETIME or narrow the contract to DATETIMEV2, with a legacy-type regression fixing the intended behavior.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29913 ms |
Contributor
TPC-DS: Total hot run time: 177687 ms |
Contributor
ClickBench: Total hot run time: 25.33 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…TETIMEV2 Arrow read Address the /review findings on apache#65780: - BE read side (data_type_datetimev2_serde): keep the Arrow epoch signed and floor the sub-second remainder so pre-1970 DATETIMEV2 values round-trip through the naive encoding. A prior static_cast<UInt64> mangled negatives (e.g. datetime(3) '1969-12-31 23:59:59.500' decoded as '.116000' plus a garbage date). This fixes both the naive and the default aware path. - type=doris federation readers: decode a timezone-aware Arrow timestamp in its own advertised timezone via a shared resolve_arrow_reader_timezone() helper, instead of the hard-coded +08:00 default, so a remote wall-clock round-trips when producer/consumer session timezones differ. Scoped to the two federation readers to avoid changing Paimon / arrow-ingest / Python UDF semantics, which share read_column_from_arrow. - Narrow the session-variable / thrift contract wording to DATETIMEV2 (legacy DATETIME v1 is already returned as a string and is unaffected). - Tests: add pre-1970 naive round-trip (DateTimeV2NaivePre1970RoundTrip) and aware-timezone resolution (ResolvesAwareArrowTimestampTimezone) BE unit tests; exercise both result-sink modes in the arrow-flight suite; correct the overclaiming federation-suite comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29596 ms |
Contributor
TPC-DS: Total hot run time: 177731 ms |
Contributor
ClickBench: Total hot run time: 25.74 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
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)