rzo1 commented on code in PR #144:
URL: https://github.com/apache/openjpa/pull/144#discussion_r3563619991
##########
openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/TestEJBQLFunction.java:
##########
Review Comment:
Had a look at this one. I debugged it by replaying the generated SQL against
a plain in-memory HSQLDB (`jdbc:hsqldb:mem:`) from a small standalone `main()`
and comparing the error codes / results of different cast variations:
```
ROUND(..., CAST(? AS DOUBLE)) -> -5563 incompatible data type in operation
ROUND(..., CAST(? AS INTEGER)) -> works
ROUND(..., ?) -> works
```
So your guess is exactly right: HSQLDB insists on an integer scale for
`ROUND`.
Where the `CAST(? AS DOUBLE)` comes from: `HSQLDictionary` sets
`requiresCastForMathFunctions = true`, and `DBDictionary.mathFunction()` then
promotes *both* arguments to the common numeric type (`Filters.promote(Double,
Integer)` → `double`) — even though the kernel (`Math` constructor) had already
typed the scale argument as `Integer`.
There is a second problem hiding behind it though: with only the scale cast
fixed, the query returns `21.0` instead of the expected `21.857`. The inner
`CAST(SUM(t0.age) AS NUMERIC)` is the culprit — in HSQLDB, `NUMERIC` without
precision/scale means **scale 0**, and the bare `?` divisor infers its type
from the other operand, so `7.0` is truncated to `7` and the division is
evaluated with scale 0:
```
CAST(SUM(age) AS NUMERIC) / ? -> 21
CAST(SUM(age) AS NUMERIC(40,20)) / ? -> 21.857142857142857...
```
Maybe something like this works — in `DBDictionary.mathFunction()`, keep a
separate cast type for the ROUND scale:
```java
int type = 0;
int rtype = 0;
if (requiresCastForMathFunctions && (lc != rc
|| (lhs.isConstant() || rhs.isConstant()))) {
Class c = Filters.promote(lc, rc);
type = getJDBCType(JavaTypes.getTypeCode(c), false);
rtype = type;
if (type != Types.VARBINARY && type != Types.BLOB) {
castlhs = (lhs.isConstant() && rhs.isConstant()) || lc != c;
castrhs = (lhs.isConstant() && rhs.isConstant()) || rc != c;
}
}
boolean mod = "MOD".equals(op);
boolean power = "POWER".equals(op);
boolean round = "ROUND".equals(op);
if (round && castrhs) {
// the second argument of ROUND is an integer scale; casting it
to
// the promoted numeric type of the first argument produces e.g.
// ROUND(x, CAST(? AS DOUBLE)), which databases reject
rtype = Types.INTEGER;
}
```
(and `appendCast(buf, rhs, rtype)` instead of `type` at the bottom of the
method)
plus in `HSQLDictionary` (`appendLength` is only ever called from
`appendCast`, so this does not affect DDL):
```java
@Override
protected void appendLength(SQLBuffer buf, int type) {
// HSQLDB gives CAST(x AS NUMERIC) without precision and scale a
scale
// of 0 and silently truncates all fractional digits, so casts
written
// for requiresCastForMathFunctions / requiresCastForComparisons
would
// corrupt decimal values; always spell out precision and scale.
// 40/20 is an arbitrary but generous choice - adjust if needed
if (type == Types.NUMERIC || type == Types.DECIMAL) {
buf.append("(40,20)");
} else {
super.appendLength(buf, type);
}
}
```
With both changes `TestEJBQLFunction` on HSQLDB is down to a single
remaining error (`testExtractWEEK` — HSQLDB does not know `EXTRACT(WEEK ...)`,
it wants `WEEK_OF_YEAR`, that one needs a separate field-name mapping), and
Derby stays green (64/64).
##########
openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/TestEJBQLFunction.java:
##########
Review Comment:
This one turned out to have the same root cause as the wrong ROUND *value*
(see the other comment). Debugged it the same way — standalone `main()` against
an in-memory HSQLDB, comparing variations:
```
SELECT 24 - 0.4 -> 23.6 (inline
literals)
SELECT ? - ? (24, 0.4) -> 24 (!)
SELECT CAST(? AS NUMERIC) - ? (24, 0.4) -> 24 (!) <- this
is what we generate
SELECT CAST(? AS NUMERIC(30,10)) - CAST(? AS ...) -> 23.6
```
OpenJPA renders JPQL literals as prepared-statement parameters, so the
inline form never reaches the DB. For `24 - 0.4`, `mathFunction()` promotes
`Long`/`BigDecimal` to `BigDecimal` and emits `CAST(? AS NUMERIC) - ?`. On
HSQLDB that goes wrong twice:
1. `CAST(? AS NUMERIC)` without precision/scale is `NUMERIC` with **scale
0** → `24` stays `24`, and
2. the bare `?` infers its type from the other operand — also `NUMERIC`
scale 0 — so `0.4` is truncated to `0`.
Result: `24 - 0 = 24`. So the `- 0.4` isn't ignored, it's truncated :)
The `appendLength` override from the ROUND comment fixes this one as well:
the cast becomes `CAST(? AS NUMERIC(40,20)) - ?`, the bare parameter inherits
the scaled type, and the result is `23.6`. The `CEILING`/`FLOOR` tests
(`SUM(c.age) + 0.4` etc.) are covered by the same fix.
Re "how can this be debugged": I set `openjpa.Log=SQL=TRACE` to capture the
generated SQL, then replayed it 1:1 in a small `main()` against
`jdbc:hsqldb:mem:` and mutated the casts until the error code / wrong value
pointed at the responsible piece.
--
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]
