SEPURI-SAI-KRISHNA commented on PR #11724:
URL: https://github.com/apache/seatunnel/pull/11724#issuecomment-5235862400
Thanks for the review. All three points are addressed; on the first one I
want to correct the premise rather than quietly accept it.
**Issue 1 (zero divisor) — done, with a caveat on the framing.** The
expression context was not actually missing. `ZetaSQLEngine` catches everything
thrown out of `computeForValue` and rewraps it:
```java
// ZetaSQLEngine.java:342
} catch (Exception e) {
throw TransformCommonError.sqlExpressionError(expression.toString(), e);
}
```
so before this change a zero divisor already surfaced as
`TransformException: The expression 'a / b' of SQL transform execute failed`,
with `ArithmeticException("/ by zero")` as the *cause*. I only found this
because the test I wrote to your suggestion failed — it asserted on the outer
message and the wrapper had already supplied the context.
What was genuinely worth fixing is the cause, so that is what changed: the
`Division` branch now throws a `TransformException` naming the operation, which
also makes `/` consistent with `%` (`NumericFunction.mod` has always thrown
`TransformException("Mod by zero")`). The test asserts both layers — expression
on the wrapper, "Division by zero" on the cause — so the distinction is pinned
rather than assumed.
I've left the `INT`/`BIGINT`/`DOUBLE` division branches alone; they have the
same bare-`ArithmeticException` cause, but they're outside what this PR touches
and fixing them here would widen the diff for no benefit to the bug being fixed.
**Issue 2 (negative operands) — done.** Added
`testNegativeDivisionRoundsHalfUp` and
`testNegativeDivisionTieRoundsAwayFromZero`:
| Expression at scale 2 | `HALF_UP` (new) | `UP` (old) |
|---|---|---|
| `-10.00 / 3.00` | `-3.33` | `-3.34` |
| `-1.00 / 1000.00` | `0.00` | `-0.01` |
| `-5.00 / 1000.00` (exact tie) | `-0.01` | `-0.01` |
The middle row is the negative mirror of the "invents value" case — the old
mode manufactured a debit from a quotient that rounds to zero. The tie row is
the one case where both modes agree; I pinned it anyway because it is what a
later switch to `HALF_EVEN` would silently change.
**Issue 3 (e2e claim) — re-verified, and you were right to want it
checkable.** My original grep was too loose. Restricting to configs that
actually have a `transform` block (JDBC `query` options are executed by the
database, not by `ZetaSQLFunction`), extracting each config's declared
`decimal(...)` columns, and testing whether any of those column names appear as
an operand of `+ - * /` in a transform query:
```
configs with a transform block AND a decimal column: 78
of those, queries doing + - * / on a decimal column: 0
```
So no e2e assertions need updating. The two configs that look like
near-misses are `sql_transform.conf` (`age+1`, where `age` is an `int`) and
`case_when.conf` (`c_decimal > 1`, a comparison).
Full module suite after the changes: `Tests run: 1082, Failures: 0, Errors:
0, Skipped: 0`. Reverting the division changes fails 4 of the 7 decimal tests:
```
testDivisionByZeroReportsExpression:202 expected TransformException but was
ArithmeticException
testDivisionDoesNotInventValue:136 expected: <0.00> but was: <0.01>
testDivisionRoundsHalfUp:122 expected: <3.33> but was: <3.34>
testNegativeDivisionRoundsHalfUp:152 expected: <-3.33> but was: <-3.34>
```
On the procedural note: agreed, the fork run for `d22a063` hadn't finished
at review time. Its `unit-test (11, windows-latest)` leg failed in
`seatunnel-api` — a module this PR does not touch, which passes locally on this
branch (383 tests) and passed on the same runners for #11697 and #11721 the
same day. I'll re-run it and confirm the matrix is green before this is
considered for merge.
--
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]
