rzo1 commented on code in PR #144:
URL: https://github.com/apache/openjpa/pull/144#discussion_r3588437336
##########
openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/TestEJBQLFunction.java:
##########
Review Comment:
I debugged the time stuff also with a small standalone `main()` against an
in-memory HSQLDB (`jdbc:hsqldb:mem:x`), creating the column once as `TIME WITH
TIME ZONE` (what OpenJPA currently generates for `java.time.OffsetTime` on
HSQLDB) and once as plain `TIME`, and inserting the values exactly the way our
`DBDictionary` binds them.
The key point is how OpenJPA handles `OffsetTime` in `DBDictionary`: on
store, the value is normalized to the JVM default offset and bound as a plain
`java.sql.Time`
(`val.withOffsetSameInstant(OffsetDateTime.now().getOffset()).toLocalTime()`);
on load, a plain `Time` is read and the JVM default offset is re-attached. So
the offset that ends up in the database is always the session offset — it
carries no real information — and the normalization can wrap the local time
past midnight. In your run: `04:57:15-09:00` became `20:57:15+07:00`, but the
second value (persisted from `LocalTime.now()` at offset `-09:00`) wrapped to
`05:12:34+07:00`.
HSQLDB then does the standard-compliant thing for `TIME WITH TIME ZONE` and
computes `MAX`/`MIN` by the UTC instant: `05:12:34+07:00` is `22:12:34Z` while
`20:57:15+07:00` is only `13:57:15Z`, so `MAX` returns `05:12:34+07:00` —
exactly the "wrong" value from the assertion, and `MIN` returns the mirror
image, which is why both are broken at once. My standalone program shows both
variations: with a `TIME WITH TIME ZONE` column I get your failure, with a
plain `TIME` column I get the expected values. It also explains why the failure
depends on the time of day / timezone the test runs in: it only bites when one
of the persisted values wraps past midnight during the normalization.
Every other dictionary avoids this by storing a zone-less type for
`OffsetTime` (Derby maps `TIME_WITH_TIMEZONE` to `TIME` in `getPreferredType`,
SQLServer and older H2 set `timeWithZoneTypeName = "TIME"`), which matches the
local-time semantics of our normalize-to-JVM-offset scheme. So I'd do the same
for HSQLDB. Maybe something like this works:
```java
// HSQLDictionary
@Override
public int getPreferredType(int type) {
if (type == Types.TIME_WITH_TIMEZONE) {
// HSQLDB compares TIME WITH TIME ZONE values by their UTC instant
// (SQL standard), but OpenJPA normalizes OffsetTime values to the
// JVM default offset on store and load. The stored offset therefore
// carries no information, while the normalization may wrap the local
// time around midnight, so MIN/MAX/ORDER BY on such a column return
// different rows than on other databases. Use a plain TIME column
// instead so comparisons happen on the normalized local time.
return Types.TIME;
}
if (dbMajorVersion > 1) {
return super.getPreferredType(type);
}
// ... existing 1.x handling unchanged
}
```
With that change `TestJava8TimeTypes` passes on `-Ptest-hsqldb` (17/17). To
be sure it isn't just passing by luck of the clock, I also ran it with
`TZ=Etc/GMT-11`, which deterministically triggers the midnight wrap: without
the change it fails with exactly your signature (`expected:<21:55:42+11:00> but
was:<00:57:15+11:00>` plus the mirrored `testMinOffsetTime` failure), with the
change it is green.
For completeness: binding the `OffsetTime` natively via `setObject` instead
would not help here — the aggregate would still be computed on UTC instants and
disagree with what the test (and all other supported databases) expect. This
also covers the `MIN(...)` case from your follow-up comment.
--
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]
