SEPURI-SAI-KRISHNA opened a new pull request, #11918: URL: https://github.com/apache/seatunnel/pull/11918
### Purpose of this pull request Follow-up to **Issue 3** of @DanielLeens's review on #11724, which I committed to opening once that PR landed. It has now merged as `2833e485`, so this is that follow-up. `toBigDecimal(Number)` existed in two places, identical statement for statement and differing only in comments: | Location | Used by | |---|---| | `ZetaSQLFunction.java:854-867` | the `DECIMAL` branch of `executeBinaryExpr` (`+ - * /`) | | `NumericFunction.java:162-173` | `MOD` | Both were `private static`, both in the same package tree. The duplication was deliberate and temporary: #11697 introduced the `NumericFunction` copy and #11724 needed the same helper while #11697 was still in review, so adding a second copy avoided a file conflict between two PRs under concurrent review. Both have merged, so that reason has expired. As the review noted, the maintenance risk is the usual one: a future change to one copy silently does not reach the other — precisely the class of divergence that both of those PRs existed to fix. ### What changed `NumericFunction.toBigDecimal` becomes `public static` and `ZetaSQLFunction` calls it; the duplicate body is deleted. ```java // ZetaSQLFunction.executeBinaryExpr, DECIMAL branch -BigDecimal leftBigDecimal = toBigDecimal(leftValue); -BigDecimal rightBigDecimal = toBigDecimal(rightValue); +BigDecimal leftBigDecimal = NumericFunction.toBigDecimal(leftValue); +BigDecimal rightBigDecimal = NumericFunction.toBigDecimal(rightValue); ``` The two Javadoc blocks are merged rather than one being discarded, so both the "no `double` round-trip" rationale and the note about which callers share it survive in the single remaining copy. **On placement.** `ZetaSQLFunction` already imports `NumericFunction` and delegates to it in around thirty places (`ABS`, `MOD`, `CEIL`, `FLOOR`, …), so calling one more static method there introduces no new coupling, no new file, and no new import. The alternative — a separate utility class for a twelve-line helper — would add a file and force `NumericFunction` to import it for its own `MOD` implementation, which reads worse. Widening the method to `public` is required because the two classes sit in different packages (`…sql.zeta` and `…sql.zeta.functions`), so package-private is not available. Deliberately **not** included, to keep this a pure refactor: neither copy handles `BigInteger`, and the review explicitly declined to raise it (*"I could not find a path by which a `BigInteger` reaches a SeaTunnel numeric row type"*). Adding a branch here would be a behaviour change hiding inside a cleanup. ### Does this PR introduce _any_ user-facing change? No. The surviving method body is identical to both removed copies, so every input produces exactly the value it did before. No config option, default, public API contract, or SPI signature changes, and no persisted state is involved. ### How was this patch tested? Existing coverage already exercises both call sites, which is what makes this safe to verify by re-running rather than by adding new tests: - `SQLDecimalArithmeticTest` (8 tests, added by #11724) covers the `ZetaSQLFunction` `DECIMAL` path that now calls across. - `NumericFunctionTest` and `SQLNumericFunctionsTest` cover the `MOD` path in `NumericFunction`. Full module run on the current head: ``` $ ./mvnw -pl seatunnel-transforms-v2 test ... [INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 - in ...SQLDecimalArithmeticTest [INFO] Tests run: 1089, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` `./mvnw spotless:apply -pl seatunnel-transforms-v2` was run and left the diff unchanged. No new tests are added: a refactor that provably does not change behaviour is verified by the existing suite continuing to pass, and a test asserting that one method equals another it now delegates to would not fail on any regression this change could plausibly cause. ### Check list * [x] If any new Jar binary package adding in your PR, please add License Notice according [New License Guide](https://github.com/apache/seatunnel/blob/dev/docs/en/developer/new-license.md) — N/A, no binaries. * [x] If necessary, please update the documentation to describe the new feature. — N/A, no user-facing behaviour change. * [x] If necessary, please update `incompatible-changes.md` to describe the incompatibility caused by this PR. — N/A, no incompatible change. * [x] If you are contributing the connector code, please check that the following files are updated — N/A, no connector code. -- 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]
