fatmanverse commented on PR #12802: URL: https://github.com/apache/seatunnel/pull/12802#issuecomment-6571092711
Thanks @DanielLeens and @SEZ9. I pushed e4e890825a for the points below. **Daniel, Issue 1 (stale sink test).** You were right, and CI confirmed it: `ExcelGeneratorTest.testDecimalKeepsAllDigits:320 expected:<STRING> but was:<NUMERIC>`. The `0.30000000000000004` row now expects a NUMERIC cell holding `0.1 + 0.2`, and the comment says why: it is the shortest text of its double, so a number cell reads it back unchanged. **Daniel, Issue 2 (integer columns).** Fixed. One deviation from the literal suggestion: before this PR the POI number-cell path used the "General" display text, which writes values of 1e11 and above in scientific notation (`1.23457E+11`), so restoring it would bring back lost BIGINT digits. Instead, `ExcelCellUtils#numberText(double, SqlType)` gives integer columns `NumberToTextConverter.toText` (Excel's 15 significant digits) for a fractional value only. A whole number keeps `Double.toString`. All three paths use it: POI number cell, POI formula, EasyExcel. As a result: - `(0.1+0.7)*10` and the same stored double read as `8` in both engines, as on `dev`. - `123456789012345.67` reads as `123456789012346` and `98765432109876.99` as `98765432109877`, the values Excel shows. - `1234567890123456` (16 digits, a whole number) reads exactly. - For a formula result the integer read is the same as on `dev`, which already used `NumberToTextConverter`. The test is renamed `testComputedNumbersReadByColumnType` and asserts `8L` again. The source docs and `incompatible-changes.md` (en/zh) describe the integer rule. **Daniel, Issue 3 (formula results in DECIMAL).** The Impact section (en/zh) now says that a formula result read into a DECIMAL column keeps every digit of its `double` and can exceed the declared scale. The Migration Guide and the source page suggest `ROUND(col, 2)` in the SQL transform. I suggest `ROUND` rather than `CAST` on purpose: on current `dev`, `CAST` to DECIMAL still rounds with CEILING (#12915, fixed by #12920). **Daniel, Issue 4 and SEZ9's two checks (text-length guard).** 1. The guard never builds the plain text. `ExcelGenerator#plainTextLength` computes the length of `toPlainString()` exactly from precision, scale and sign (the earlier version was a conservative estimate), and only then calls `toPlainString()` or `toString()`. 2. The new `testDecimalPlainTextLengthLimit` covers the boundary in both directions. `1E+32766` and `-1E-32764` have plain texts of exactly 32767 characters and are written as plain text. `1E+32767` and `-1E-32765` are one character over and are written as `1E+32767` and `-1E-32765`. **SEZ9, the truncated reply.** The full text of my previous reply is on the PR page: https://github.com/apache/seatunnel/pull/12802#issuecomment-6557065213. The notification email cut it off. It answers the remaining points: docs placement, the `.xls` / EasyExcel `getOriginalNumberValue()` path, mixed cells, declared scale, upgrade note, the cast, and the test comment. Some of it is now superseded: the computation-noise heuristic described there was removed in 48536eab1 at Daniel's request. Verified on JDK 17. With the new tests on the previous head (`db7f0935a5`), exactly the three new assertions fail: `c_bigint row 0 expected <8> but was <7>` in both engines, and the 32767 boundary. With this commit, `ExcelGeneratorTest` passes 6/6 and `ExcelReadStrategyTest` 29/29, and `connector-file-base` runs 394 tests with 0 failures. -- 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]
