[fix](function) Prefer DATETIMEV2 over TIMESTAMPTZ when binding *_diff functions on non-literal string args

[lets-order-some-fries]

What problem does this PR solve?

Issue Number: close#66120

Related PR: #64127

Problem Summary:

DATEDIFF and the other eleven *_diff scalar functions silently return off-by-one (or off-by-N) results when (1) an argument is a string-typed non-literal — a varchar column, subquery projection slot, or UNION-ALL output slot — and (2) the session time_zone is not UTC.

All 12 functions list their TIMESTAMPTZ signature first in SIGNATURES. The timezone-coercion penalty in SearchSignature.doMatchTypes only fires for literals it can inspect (ExpressionUtils.getLiteralAfterUnwrapNullable, added in #64127), so for a varchar SlotReference every candidate signature ties and the tie-break keeps the first-listed candidate — TIMESTAMPTZ. The varchar value is then cast through timestamptz(6), which treats it as session-local time and shifts it to UTC, so day-number arithmetic runs on the shifted value.

Fix, as proposed by @Baymine in #66120: move the TimeStampTz signature to last in each of the 12 *Diff files. Literal-with-timezone arguments still route to TIMESTAMPTZ via the timeZoneCoersionScore branch; typed TIMESTAMPTZ / DATETIMEV2 / DATEV2 arguments still bind their identical-match signatures. Only the previously order-decided tie for non-literal string arguments changes: TIMESTAMPTZ → DATETIMEV2 (wall-clock semantics, independent of session time zone).

New unit test DiffFunctionSignatureTest covers all 12 functions in both directions: varchar slots must bind DATETIMEV2 (fails on master before this change) and TIMESTAMPTZ slots must still bind TIMESTAMPTZ. Happy to also add a SQL regression suite mirroring the issue's reproducer (varchar column / subquery slot / UNION-ALL slot under +08:00) if wanted — omitted here because the .out expected-files need a cluster run to generate.

Release note

Fixed DATEDIFF and the other *_diff functions returning wrong results on varchar columns and subquery slots when the session time_zone is not UTC.

Check List (For Author)

• Test

  • Regression test
  • Unit Test
  • Manual test (add detailed scripts or steps below)
  • No need to test or manual test. Explain why:
    • This is a refactor/code format and no logic has been changed.
    • Previous test can cover this change.
    • No code files have been changed.
    • Other reason

• Behavior changed:

  • No.
  • Yes. For *_diff calls whose string-typed argument is NOT a literal, the argument now binds to DATETIMEV2 (wall-clock) instead of TIMESTAMPTZ (UTC-shifting). That re-bind is the fix; literal arguments and typed date/datetime/timestamptz arguments are unaffected.

• Does this need documentation?

  • No.
  • Yes.

[hello-stephen]

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?

[morrySnow]

run buildall

[morrySnow]

/review

[github-actions]

Request changes: the signature reorder fixes the intended homogeneous VARCHAR fallback, but it also changes valid literal calls from TIMESTAMPTZ instant semantics to DATETIMEV2 civil semantics. One inline comment covers the two related, concrete failure classes.

Critical checkpoints:

• Goal and proof: all 12 functions now select DATETIMEV2 for uninspectable VARCHAR slots, and the new unit directly covers that plus typed TIMESTAMPTZ slots. The stated goal is still incomplete because mixed accepted literal forms can rebind; the unit omits those cases.
• Scope and clarity: the production edit is mechanically small, but signature order is a broader semantic tie-break than the test demonstrates.
• Concurrency: not involved.
• Lifecycle/static initialization: only immutable signature-list ordering changes; no non-intuitive lifecycle or cross-unit initialization dependency is introduced.
• Configuration: none added or changed.
• Compatibility: no wire, storage, serialized-format, or function-symbol change; existing FE folding and BE DATETIMEV2/DATEV2/TIMESTAMPTZ overloads remain aligned for rolling execution.
• Parallel paths: all 12 scalar classes were updated consistently. Analyzer cast insertion, cached/recomputed signatures, nullable/subquery/UNION shapes, TimeDiff precision, FE folding, and BE dispatch were traced; no separate missed path survived.
• Conditional logic: the existing summed timezone score and the narrower DateTimeChecker grammar are the blocking correctness seam described inline.
• Tests and results: the JUnit test is structurally consistent and covers every constructor, but it lacks mixed zoned/zone-less and checker-versus-cast grammar regressions. No generated result file changed. No local build or test was run because this review environment explicitly forbids builds.
• Observability: no new runtime path needs logs or metrics.
• Transactions/persistence and data writes: not involved.
• New FE-BE variables: none.
• Performance: constant-size signature lists and matching work are unchanged; no material CPU or memory concern.
• Other correctness: the inline issue is user-visible and blocking because elapsed-hour results change across DST.

User focus: no additional focus was supplied.

Completion status: three review rounds completed; every Round 3 agent returned NO_NEW_VALUABLE_FINDINGS, all candidates were accepted, deduplicated, or dismissed with evidence, and this review targets exact head f5c40e8. FE UT, compile, and performance checks were still pending at submission time.

[github-actions]

[P1] Preserve instant semantics for all accepted zoned literal forms

The timezone score is summed across both arguments, so a recognized zoned literal paired with a zone-less literal scores +1 - 1 = 0 and ties DATETIMEV2. With time_zone='America/Los_Angeles', hours_diff('2021-03-14 03:30:00-07:00', '2021-03-14 01:30:00') therefore changes from 1 elapsed hour on the old TIMESTAMPTZ winner to 2 civil hours after moving this overload last. There is a second instance of the same policy gap where both inputs have explicit zones: the cast grammar accepts and normalizes compact -0700, but DateTimeChecker requires a colon before offset minutes, so pairing 2021-03-15 00:00:00-0700 with 2021-03-14 00:00:00-08:00 again ties and changes 23 elapsed hours to 24 civil hours. Please preserve TIMESTAMPTZ whenever an inspectable literal supplies any accepted explicit zone, using the same accepted grammar as the cast path, without reintroducing the all-uninspectable-VARCHAR fallback; add regressions for both mixed cases.

[lets-order-some-fries]

Good catch — the summed score was a real regression from the reorder. Fixed in 66e0ff1.

doMatchTypes accumulated the timezone preference with ++/-- across arguments, so one zoned literal plus one zone-less literal cancelled to 0 and tied DATETIMEV2. With TIMESTAMPTZ moved last, that tie flipped to civil semantics. It now tracks "some argument carried an explicit zone" and "some argument did not" independently, and derives the score so that an explicit zone anywhere in the call wins:

• any zoned literal → TIMESTAMPTZ (instant semantics — the case you flagged)
• only zone-less literals → DATETIMEV2 (preserves [Fix](nereids) Fix nullable date literal binding and date *_diff folding #64127)
• no inspectable literal at all → 0, so list order decides → DATETIMEV2 (this PR's original fix, unchanged)

Both of your concrete cases are covered. hours_diff('2021-03-14 03:30:00-07:00', '2021-03-14 01:30:00') binds TIMESTAMPTZ again, in either argument order. The -0700 / -08:00 pairing does too — the recognized zone on the second argument now dominates instead of being cancelled by the unrecognized one.

New regressions in DiffFunctionSignatureTest: the mixed zoned/zone-less pair in both orders, and an all-zone-less pair asserting DATETIMEV2 so #64127's intent stays pinned.

On the compact-offset grammar, one thing worth separating out. A residual case remains: when every zone-carrying literal in the call uses the compact form, DateTimeChecker.hasTimeZone recognizes none of them and the call still binds DATETIMEV2. That behaviour is not changed by this PR — before the reorder those arguments scored -1 + -1 = -2 against DATETIMEV2's 0 and already lost the comparison, so it is a pre-existing recognition gap rather than something this PR introduces.

Closing it means widening DateTimeChecker.timeZone(), which currently requires a colon before the offset minutes. That predicate also backs isValidDateTime, which gates literal coercion for DATETIME, DATEV2 and TIMESTAMPTZ alike (TypeCoercionUtils:669-679), so widening it changes literal acceptance well beyond these 12 functions. I did not want to fold that into a targeted binding fix without you deciding it. Happy to do it here if you'd like it in one change, or as a follow-up — your call.

[lets-order-some-fries]

/review

[lets-order-some-fries]

@morrySnow when you have a moment — this one needs a committer trigger to move, and I want to make sure it isn't just sitting on a stale signal.

The /review findings are addressed in 66e0ff14 (pushed Aug 31): signature matching summed the timezone preference across arguments, so a zoned literal paired with a zone-less one cancelled to 0 and tied DATETIMEV2. It now tracks the two independently, so an explicit zone anywhere in the call decides instant semantics and cannot be cancelled out.

Two things are stale rather than failing:

• run buildall last ran against f5c40e8a, the pre-fix commit. 66e0ff14 has never been built.
• code-review still reads "Trigger /review to start automated review for 66e0ff14", so the visible CHANGES_REQUESTED is the verdict on the older commit. I posted /review myself on Sep 2 and it did not start the job — the trigger looks committer-only.

So a re-run of both would either clear it or give you something real to look at. No rush on my open question about widening DateTimeChecker.timeZone() for compact -0700 offsets — that one is genuinely your call, and I'm happy either way (here, or a follow-up).