SEPURI-SAI-KRISHNA commented on PR #11724: URL: https://github.com/apache/seatunnel/pull/11724#issuecomment-5327962280
Thanks for the sixth-round review, and in particular for re-running the arithmetic on the test's own numbers instead of taking the assertion on trust. One thing worth saying explicitly, since it was a deliberate choice rather than a hedge. On **Issue 2**, the obvious way to write that assertion would have been to present it as a precision *bound*. I didn't, because nothing in the DECIMAL branch actually bounds precision — `setScale` fixes decimal places and leaves integer digits alone. Claiming the assertion proves more than it does would repeat exactly the mistake that produced the original blocker: the pre-fix code was wrong precisely because the declared type and the emitted value were assumed to agree without anything enforcing it. So the Javadoc says the check documents the range these operands stay within and is not a proof, and the real fix (a near-ceiling test plus a `MathContext` or explicit overflow check) stays open as a follow-up. Same reasoning behind scoping the `testEmittedScaleMatchesDeclaredType` invariant to DECIMAL-on-DECIMAL and naming your Issue 1 in the Javadoc rather than quietly narrowing the claim. **On CI** — your diagnosis of `unit-test (11, windows-latest)` (job `95634437084`, `AbstractSeaTunnelServerTest.before` → `IllegalStateException: Node failed to start!`) matches what I see, and it is in `seatunnel-engine-server`, which this PR does not touch. For a second data point on the same class of problem: #11721's run failed today too, on a completely different leg (`8, ubuntu-latest`), and that one turned out to be a Maven Central `Connection reset` while downloading `com.clickhouse:clickhouse-client:0.3.2-patch11` — zero test failures in an 18,544-line log. Two runs, two unrelated infrastructure flakes, two different legs. Which brings me to the fail-fast point you made. I owe an issue on it from my 2026-08-14 comment, and I now have the evidence rather than a hunch. `.github/workflows/backend.yml:480-483` declares the `unit-test` matrix with no `fail-fast: false`, while `benchmark-test` at `:511-512` sets it explicitly — so the omission looks like an oversight, and it is a one-line fix. The cost is visible in both of today's runs: a single flake on one platform cancels the other three legs, discarding precisely the cross-platform signal a 2x2 matrix exists to produce, and making a Windows-only Hazelcast problem indistinguishable from a real regression until someone reads the log by hand. I will file that separately rather than widen this PR. Follow-ups from your Issue Summary, unchanged and still owned: - **Issue 3** — the `toBigDecimal` unification between `ZetaSQLFunction` and `NumericFunction` goes up as its own PR once this lands, as promised. - **Issues 1, 2, 4** — tracked, out of scope here, and none of them made worse by this change. Requesting a rerun of `unit-test (11, windows-latest)` for a clean board. -- 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]
