[fix](TIMESTAMPTZ) Preserve TIMESTAMPTZ for microsecond interval arithmetic - #66843
[fix](TIMESTAMPTZ) Preserve TIMESTAMPTZ for microsecond interval arithmetic#66843Mryange wants to merge 3 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Nereids forced day, hour, and minute microsecond interval arithmetic signatures to DATETIMEV2 even when the input was TIMESTAMPTZ. Preserve the TIMESTAMPTZ argument and return types so these interval operations can be planned and executed correctly.
### Release note
Support TIMESTAMPTZ values in day, hour, and minute microsecond interval addition and subtraction.
### Check List (For Author)
- Test: Regression test and FE build
- Regression test
- Behavior changed: Yes, TIMESTAMPTZ microsecond interval arithmetic is now supported
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Request changes: the patch does not yet preserve TIMESTAMPTZ for typed inputs below scale 6, and it leaves existing expected output stale for the zoned-string path that does work.
Critical checkpoint conclusions:
- Goal and proof: Not met. The new scale-3 value test exercises the old DATETIMEV2 coercion path, so its passing values do not prove TIMESTAMPTZ preservation or the TIMESTAMPTZ(6) result type.
- Scope and implementation: The six production edits are symmetric, but the family check runs after signature search has already selected DATETIMEV2 for typed TIMESTAMPTZ(0..5). No extra issue was found in the unchanged interval parser, nullable handling, or add/sub symmetry.
- Runtime and compatibility: Once the TIMESTAMPTZ family is selected, FE folding and BE execution agree and the required BE overloads already exist at the base revision. No separate protocol, storage, mixed-version, configuration, lifecycle, concurrency, transaction, ownership, observability, or performance issue was found.
- Tests and results: The new suite follows the repository's ordering and setup conventions, but its checked row is correct for the wrong family. The 18 existing zoned-literal expectations affected by the working branch were not regenerated. Per the review-runner contract, I did not run builds or tests.
- User focus: No additional review focus was supplied.
Review convergence: Round 2 completed with no new valuable findings from either full-scope pass or the focused boundary pass. The two inline findings below are the complete accepted set.
| @Override | ||
| public FunctionSignature computeSignature(FunctionSignature signature) { | ||
| signature = super.computeSignature(signature); | ||
| if (signature.argumentsTypes.get(0) instanceof TimeStampTzType) { |
There was a problem hiding this comment.
[P1] Select the TIMESTAMPTZ family before forcing scale 6
For the test's ts TIMESTAMPTZ(3), neither concrete MAX-scale signature matches identically. In the implicit round both candidates match, receive equal scores, and SearchSignature keeps the first DATETIMEV2(6) signature. Because search runs before this method, super.computeSignature has already lost the source family and this condition is false:
Project(day_microsecond_add(
CAST(ts#1: TIMESTAMPTZ(3) AS DATETIMEV2(6)), interval
) : DATETIMEV2(6))
Thus typed TIMESTAMPTZ columns, casts, and literals at scales 0-5 still take the old session-local DATETIMEV2 path in all six changed classes; only scale 6 and zoned string literals reach this branch. Please use wildcard-scale family signatures (the established sibling pattern), or otherwise preserve the typed family during selection without regressing the existing zoned-string selection, before promoting the selected family to scale 6. Also cover typed scales below 6.
| @@ -0,0 +1,4 @@ | |||
| -- This file is automatically generated. You should know what you did if you want to edit this | |||
| -- !timestamptz_microsecond_interval -- | |||
| 1 2024-01-03T05:07:09.246456 2024-01-01T01:01:00.999544 2024-01-02T05:07:09.246456 2024-01-02T01:01:00.999544 2024-01-02T03:07:09.246456 2024-01-02T03:01:00.999544 | |||
There was a problem hiding this comment.
[P1] Regenerate the existing zoned-literal expectations
This branch already changes timezone-bearing string literals: signature search prefers the TIMESTAMPTZ overload for them, so their results now serialize with an offset. test_add_sub_union_type.groovy has 18 existing affected qts (day add 11-13/sub 5-7, hour add 10-12/sub 5-7, and minute add 11-13/sub 5-7), but its .out file is not changed and still contains offset-free DATETIMEV2 values. The pre-existing second_microsecond_add_11/sub_5 outputs show the expected +08:00 form. Please regenerate that existing golden file as part of this change; otherwise the existing regression suite fails. The offset-free row here also shows that this test's scale-3 column still follows the wrong path described in the other comment.
### What problem does this PR solve? Issue Number: None Related PR: apache#66843 Problem Summary: Concrete scale-6 candidate signatures were selected before return-type computation, so TIMESTAMPTZ inputs with scales below 6 could match the DATETIMEV2 candidate. Use wildcard-scale candidates for all day, hour, and minute microsecond add/subtract functions, then promote the selected type family to scale 6. Extend regression coverage across TIMESTAMPTZ(0), TIMESTAMPTZ(3), and TIMESTAMPTZ(6), including return-type checks and existing timezone-literal expectations. ### Release note Preserve TIMESTAMPTZ results for day, hour, and minute microsecond interval arithmetic at all supported input scales. ### Check List (For Author) - Test: FE build and regression tests - Regression test - Behavior changed: Yes, TIMESTAMPTZ inputs now retain their type family and return scale 6 - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Request changes: the current head fixes both earlier review findings for the six changed day/hour/minute classes, but the same overload-selection defect remains in six reachable scale-promoting siblings.
Critical checkpoint conclusions:
- Goal and proof: Partially met. Typed TIMESTAMPTZ scales 0/3/6 now bind and execute correctly for the six changed functions, and every changed golden value is correct. However,
SecondMicrosecondAdd/Sub,MicroSecondsAdd/Sub, andMilliSecondsAdd/Substill return DATETIMEV2(6) for typed TIMESTAMPTZ(0-5), so the broader microsecond-arithmetic goal is incomplete. - Scope and implementation: The wildcard-family approach is small, symmetric, and correct in the changed classes. The remaining concrete-MAX sibling signatures are the only parallel instances of this search-before-compute pattern, and omitting them leaves the same root cause reachable through interval syntax and direct scalar calls.
- Runtime and compatibility: For the changed six, FE folding and non-folded BE execution select matching TIMESTAMPTZ overloads and preserve scale 6; DATETIMEV2 behavior and null propagation remain unchanged. The omitted siblings instead insert a TIMESTAMPTZ-to-DATETIMEV2 cast and bypass their existing TIMESTAMPTZ FE/BE overloads. No new protocol, storage-format, or mixed-version compatibility issue was introduced.
- Concurrency, lifecycle, configuration, persistence, and data writes: Not involved. The patch changes immutable signature declarations and regression artifacts only; it adds no locks, mutable shared state, configuration, EditLog/transaction path, or production write path.
- Tests and results: The new type assertions and ordered runtime queries correctly cover the changed six at scales 0/3/6, and all 18 new result cells plus 18 regenerated zoned-literal expectations are consistent with the SQL and session offset. The matrix does not cover the six omitted sibling paths. Per the review-runner contract, I did not run builds or tests.
- Performance and observability: The two-signature search shape and runtime implementations are unchanged, with no material performance or observability concern beyond the correctness issue.
- User focus: No additional review focus was supplied.
Review convergence: Round 2 completed with NO_NEW_VALUABLE_FINDINGS from both normal full-review passes and the separate risk-focused boundary pass. The single P1 inline comment is the complete accepted set.
| .args(DateTimeV2Type.MAX, VarcharType.SYSTEM_DEFAULT), | ||
| FunctionSignature.ret(TimeStampTzType.MAX) | ||
| .args(TimeStampTzType.MAX, VarcharType.SYSTEM_DEFAULT) | ||
| FunctionSignature.ret(DateTimeV2Type.WILDCARD) |
There was a problem hiding this comment.
[P1] Cover the remaining scale-promoting TIMESTAMPTZ siblings
These wildcard signatures fix the six day/hour/minute classes, but SecondMicrosecondAdd/Sub, MicroSecondsAdd/Sub, and MilliSecondsAdd/Sub still declare the ordered concrete pair DATETIMEV2(6) then TIMESTAMPTZ(6). For example, a typed scale-3 column still binds as:
Project(second_microsecond_add(
CAST(ts3#1: TIMESTAMPTZ(3) AS DATETIMEV2(6)), '0.000001'
) : DATETIMEV2(6))
Neither concrete signature is identical for scales 0-5, both tie in the implicit round, and search keeps the first DATETIMEV2 candidate before those classes inspect the selected family. All six paths are reachable and already have dedicated TIMESTAMPTZ FE/BE overloads, so they still return the wrong family and use session-local DATETIMEV2 semantics. This is distinct from the earlier thread, which covered only the six changed classes. Please use family wildcard signatures for these siblings too and extend the typed scale-0/3/6 type/runtime matrix to cover them.
### What problem does this PR solve? Issue Number: None Related PR: apache#66843 Problem Summary: The second-microsecond, microseconds, and milliseconds add/subtract functions still used concrete scale-6 candidate signatures. During overload selection, TIMESTAMPTZ inputs with scales below 6 could therefore match the DATETIMEV2 candidate before return-type computation. Use wildcard-scale candidates for the remaining functions, preserve the selected type family, and promote the argument and return type to scale 6. Extend the regression matrix so all twelve microsecond interval functions verify both return types and runtime values for TIMESTAMPTZ(0), TIMESTAMPTZ(3), and TIMESTAMPTZ(6). ### Release note Preserve TIMESTAMPTZ results for all microsecond interval arithmetic functions at every supported input scale. ### Check List (For Author) - Test: FE build and regression test - Regression test - Behavior changed: Yes, the remaining microsecond interval functions retain TIMESTAMPTZ and return scale 6 - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Approval opinion: the current head resolves all three earlier P1 review threads, and I found no new blocking issue.
Critical checkpoint conclusions:
- Goal and proof: Met. Same-family wildcard signatures now preserve DATETIMEV2 versus TIMESTAMPTZ before every changed function promotes argument 0 and the result to scale 6. The new regression matrix asserts and executes all 12 add/sub functions for typed TIMESTAMPTZ scales 0, 3, and 6.
- Scope, parallel paths, and conditions: The implementation is small and symmetric. It covers day/hour/minute/second microsecond interval add/sub plus microseconds and milliseconds add/sub; no remaining parallel scale-promoting arithmetic class uses the defective concrete-MAX overload pair. Zoned strings still choose TIMESTAMPTZ, timezone-free strings and bare NULL retain their established DATETIMEV2 default, and explicit family casts retain their family.
- Concurrency and lifecycle: Not involved. The production changes affect immutable signature declarations and deterministic signature computation only; they add no threads, locks, mutable shared state, static-initialization dependency, ownership, or cleanup path.
- Configuration and compatibility: No configuration, symbol, thrift field, storage format, or persisted metadata changes are introduced. Matching TIMESTAMPTZ BE overloads and registrations already exist at the base SHA, so new-FE/old-BE execution is supported and old-FE/new-BE behavior is unchanged.
- Error handling and nullability: Existing coercion errors, interval parsing, and
PropagateNullablebehavior are reused unchanged. Argument casts and return types stay in the same selected family, including typed NULL and reconstruction/constant-folding paths. - Tests and results: The new suite follows the regression conventions: fixed session zone, hardcoded table, drop-before-create, ordered query, deterministic expression order, generated
.out, and exacttimestamptz(6)slot checks. All 36 new arithmetic cells and all 18 regenerated zoned-literal expectations were independently checked; existing suites cover DATETIMEV2, literals, NULL/error cases, malformed/negative intervals, and boundary behavior. Per the review-runner contract, I did not run builds or tests. - Observability, transactions, persistence, and data writes: Not applicable. No new runtime control path, production write path, EditLog/transaction state, metric, or logging requirement is introduced.
- FE-BE variables and performance: No transmitted variable is added. Candidate count remains two and signature computation remains lazily cached; the change adds no material per-row CPU, memory, allocation, or I/O cost.
- Other issues: None found after the final changed-file, reconstruction/folding, FE-to-BE, expected-output, and duplicate-context sweeps.
- User focus: No additional review focus was supplied.
Review convergence: Round 1 completed with NO_NEW_VALUABLE_FINDINGS from both normal full-coverage reviews and the separate risk-focused dispatch/promotion review. All initial risk items are resolved, there are no accepted inline findings, and this is a verified zero-inline comment-only review.
|
PR approved by at least one committer and no changes requested. |
Problem Summary: Nereids forced day, hour, and minute microsecond interval arithmetic signatures to
DATETIMEV2even when the input wasTIMESTAMPTZ. Root cause: the FE signature computation unconditionally replaced the argument and return types. This change preservesTIMESTAMPTZfor these add and subtract functions while keeping the existingDATETIMEV2behavior unchanged.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)