SEZ9 commented on PR #12802: URL: https://github.com/apache/seatunnel/pull/12802#issuecomment-6593757440
Thanks for the detailed follow-up and for pushing e4e890825a, @fatmanverse. On the two points you walked through: - **Stale sink test**: the reasoning for the `0.30000000000000004` row makes sense — it is the shortest text of `0.1 + 0.2`, so a NUMERIC cell round-trips unchanged and the test expectation should follow the sink. Agreed on the fix and on recording the "why" in the comment. - **Integer columns**: the deviation from "General" display text is well justified; restoring it would reintroduce exactly the BIGINT digit loss this PR is about. Routing the POI number cell, POI formula, and EasyExcel paths through `ExcelCellUtils#numberText(double, SqlType)` so `123456789012345.67` reads as `123456789012346` and `98765432109876.99` as `98765432109877` is the behavior I'd expect. I'll verify the behavior in the code on my next pass. A few items from my earlier review that I don't see covered yet in this comment — a short status line per item is enough: - Bounding `new BigDecimal(cellText)` on untrusted text cells (e.g. `1E+2147483647`) before `toPlainString()` can allocate ~2 GB. - EasyExcel `.xls` still falling back to display text because `getOriginalNumberValue()` is only populated by the xlsx SAX parser. - The upgrade note saying numeric columns "keep their full stored value" — does it still match the code for the POI FORMULA branch and the EasyExcel path without an original number value after this change? - With the `0.30000000000000004` row now NUMERIC, DECIMAL columns still get a per-value mix of number and text cells; is there a way for users to keep the previous all-numeric output? - DECIMAL read from number cells ignoring the declared `DecimalType` scale (`5000.0000` → `5000.0`) and the test relying on `compareTo`. - The hard `(BigDecimal) value` cast in `setDecimalCellValue`. - The test comment in `testPoiReadKeepsNumericPrecision` attributing the text cell to the sink when the fixture is raw POI. - Surfacing the new numeric semantics in the file connector source/sink docs, not just incompatible-changes.md. Once those are addressed or explicitly deferred, I'm happy to take another pass. <!-- streview-comment:2233 --> -- 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]
