SEPURI-SAI-KRISHNA opened a new pull request, #29366:
URL: https://github.com/apache/flink/pull/29366
## What is the purpose of the change
`TIMESTAMPDIFF` mishandles `TIME` operands in two ways.
A year-month unit between two `TIME` values returns a large wrong number:
`TIMESTAMPDIFF(MONTH, TIME '12:00:00', TIME '13:00:00')` returns `118277`
instead of `0`. A day-time unit mixing `TIME` with `DATE` or `TIMESTAMP`
fails
during planning with `scala.MatchError`.
Both live in `generateTemporalPlusMinus`. The match arm admits
`{TIMESTAMP, TIME, DATE}²`, and after Calcite widens a `DATE` operand to
`TIMESTAMP` seven pairs can actually arrive there.
**Year-month.** `(TIME, TIME)` had no case and fell through to the default,
which emits `subtractMonths($ll, $rr)`. `qualifyMethod` writes the method
name
into generated Java, so Janino resolves the overload from the operand types.
`TIME` is an `int` internally, so it binds `DateTimeUtils.subtractMonths(int,
int)` — documented as *"the number of months between two dates, each
represented as the number of days since the epoch"*. The milliseconds of day
are read as an epoch day number, putting the two values ~3.6M days apart.
`BuiltInMethods.SUBTRACT_MONTHS` declares the `(long, long)` millisecond
overload, but that declaration does not reach the generated source. The same
default case is correct for `(DATE, DATE)`, where epoch days is the right
reading — which is why it was written that way.
**Day-time.** The block has no case for `(TIMESTAMP, TIME)` or
`(TIME, TIMESTAMP)` and no catch-all, so those two arrivals throw.
`TIME` is a time point on 1970-01-01. That is not a new interpretation: the
existing assertion at `TemporalTypesTest.scala:1327`,
`TIMESTAMPDIFF(MONTH, TIME '00:00:00', TIMESTAMP '2021-02-04 12:00:00')` =
`613`,
is exactly 1970-01-01 → 2021-02-04 in months, and the mixed
`TIME`/`TIMESTAMP`
cases already use the millisecond overload. This change makes `(TIME, TIME)`
agree with its own neighbours, so `0` is the consistent result.
Why it was not caught: every existing year-month `TIME` assertion uses
`TIME '00:00:00'`, whose internal value is `0` — and `0` denotes 1970-01-01
whether it is read as milliseconds of day or as an epoch day. The only
non-midnight `TIME` assertions use `SECOND` and `MINUTE`, a different branch.
This completes FLINK-39385, which fixed the same `MatchError` for the
`(TIME, TIME)` pair.
## Brief change log
- `ScalarOperatorGens.generateTemporalPlusMinus`, year-month branch:
explicit
`(TIME, TIME)` case casting both operands to `long`, selecting the
millisecond overload of `subtractMonths`
- same method, day-time branch: cases for `(TIMESTAMP, TIME)` and
`(TIME, TIMESTAMP)`, treating a `TIME` as epoch millis on 1970-01-01,
matching the convention already used by the year-month branch
- 12 assertions added to the existing `TemporalTypesTest#testTimestampDiff`
Only the two pairs that can reach the day-time branch are added; `DATE` is
widened to `TIMESTAMP` before code generation, so `(DATE, TIME)` cases would
be
unreachable.
Scope is SQL, matching FLINK-39385. The Table API rejects `TIME` operands for
this function before code generation, unchanged by this PR.
## Verifying this change
This change added tests and can be verified as follows:
- `TemporalTypesTest#testTimestampDiff` covers both defects. Without the
production change it fails with
`expected: <0> but was: <9856>` and `MatchError
(TIMESTAMP_WITHOUT_TIME_ZONE,TIME_WITHOUT_TIME_ZONE)`;
with it, the class passes 38/38
- expected values were derived independently rather than read off the new
behaviour, including the truncation direction, which is toward zero and
was
confirmed on the unchanged `(TIMESTAMP, TIMESTAMP)` path
- all nine source type pairs were exercised for a year-month and a day-time
unit, before and after, to confirm the previously correct results are
unchanged and that no pair still fails
- verified on constant-folded literals and on real table columns, so the
fix
is confirmed on the runtime path and not only in plan-time reduction
- 962 tests across the temporal, interval, calc and codegen suites pass
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): **no**
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: **no**
- The serializers: **no**
- The runtime per-record code paths (performance sensitive): **no** — the
generated expression changes for `TIMESTAMPDIFF` with `TIME` operands,
but
adds no per-record work; an `int`→`long` widening cast is free
- Anything that affects deployment or recovery: JobManager (and its
components), Checkpointing, Kubernetes/Yarn, ZooKeeper: **no**
- The S3 file system connector: **no**
## Documentation
- Does this pull request introduce a new feature? **no**
- If yes, how is the feature documented? **not applicable** —
`TIMESTAMPDIFF`
is already documented; this corrects its behaviour for `TIME` operands
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: Claude Code (Opus 5)
--
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]
