lets-order-some-fries commented on PR #67238:
URL: https://github.com/apache/doris/pull/67238#issuecomment-5479693225
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 #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
