fatmanverse commented on PR #12802:
URL: https://github.com/apache/seatunnel/pull/12802#issuecomment-6557065213
Thanks @SEZ9 for the detailed pass. I pushed 8972a5024f for the points
below. I checked a few of them against the EasyExcel 4.0.3 and POI 4.1.2
classes first, because the answer depends on what those libraries do.
**1. Unbounded `BigDecimal` / `toPlainString()`.** Fixed on the sink side.
You are right that the new `toPlainString()` in `ExcelGenerator` is the
dangerous call. It is now `toDecimalText`, which writes the plain text only
when its length (precision + |scale| + sign and point) fits an Excel text cell
(32767 characters) and otherwise writes `BigDecimal#toString()`. So
`1E+2147483647` becomes the cell text `1E+2147483647`
(`ExcelGeneratorTest#testDecimalWithLargeExponentIsWrittenAsScientificText`).
This covers every DECIMAL that reaches the Excel sink, not only those read from
Excel.
I did not add a precision/scale rejection on the read side. `new
BigDecimal(text)` is what the CSV, Text, JSON and XML readers already do for a
DECIMAL column, and before this PR the Excel reader also accepted values that
do not fit the declared type (e.g. `123456789012345678` into `decimal(20, 4)`).
Turning that into a hard failure would be a new behaviour change for Excel
alone. The new scale handling in point 5 is bounded by the declared precision,
so it never expands such a value either (the test reads `1E+2147483647` back
unchanged).
**2. Connector docs.** Added to `docs/{en,zh}/connectors/sink/LocalFile.md`
(after the Excel compression tip): when a DECIMAL becomes a text cell, that
`SUM` skips text cells, and the `CAST` workaround. Added to
`docs/{en,zh}/connectors/source/LocalFile.md` under `excel_engine`: numeric
columns read the stored number (`50%` reads as `0.5`), STRING columns keep the
displayed text, and DECIMAL also reads text cells.
**3. `.xls` with EasyExcel.** I don't think this one happens. In EasyExcel
4.0.3, `NumberRecordHandler` (v03) builds the cell with
`ReadCellData.newInstanceOriginal(BigDecimal.valueOf(record.getValue()), row,
column)`. That sets `originalNumberValue` and then derives `numberValue` from
it by rounding. `FormulaRecordHandler` calls
`setOriginalNumberValue(BigDecimal.valueOf(getValue()))` the same way. RK and
MulRK cells never reach EasyExcel as such: POI's `RecordFactoryInputStream`
converts them to `NumberRecord` first (`RecordFactory.convertToNumberRecord` /
`convertRKRecords`). So `.xls` takes the same fast path as `.xlsx`. To pin it,
I added `testXlsReadKeepsNumericPrecision` for both engines on an `.xls` file
(number cells and a formula cell).
**4. `excel_decimal_mode` option.** I'd rather not add an option in this PR.
Both uniform layouts are already available with the SQL transform before the
sink: `CAST(col AS DOUBLE)` gives number cells (rounded to a double), and
`CAST(col AS STRING)` gives text cells with every digit. The sink doc now says
this. A new option is a permanent contract, so I'd prefer to add it in a
follow-up if users ask for it. Happy to open an issue to track it if you think
it is needed.
**5. Declared scale.** Fixed in `ExcelCellUtils#toDeclaredScale`. A DECIMAL
now gets the declared scale when that is lossless and the result fits the
declared precision, so `5000.0` reads as `5000.0000` in `decimal(20, 4)` from
both number and text cells. I did not use `setScale(scale, HALF_UP)`. A value
with more fractional digits (`1.123456` into `decimal(20, 4)`) is returned
unchanged rather than rounded, as before this PR and as the other format
readers do; silently rounding data on read seemed worse than leaving it to the
sink. The round-trip tests now use `assertEquals` on the exact `BigDecimal`.
`testDecimalReadsWithDeclaredScaleWithoutRounding` covers padding, zero, the
no-rounding case, a value too wide for the precision, and huge exponents
(`1E+2147483647`, and `1000E+2147483647`, whose trailing zeros would underflow
the scale if they were stripped first).
**6. POI FORMULA branch.** Fixed in code rather than in the wording. Looking
at formula results also showed a real problem in the previous head. A computed
value carries binary noise: `(0.1+0.7)*10` is stored as `7.999999999999999` and
`0.1+0.2` as `0.30000000000000004`. With the shortest double text, an INT
column truncated the first to `7`, and a DECIMAL column got the noise. This
already happened with EasyExcel, which reads the full cached formula value, and
with number cells written by other tools. One helper,
`ExcelCellUtils#numberText`, now decides the text of a stored number for a
numeric column, and both engines use it for number cells and numeric formula
results:
- DOUBLE and FLOAT columns get the exact double (`1/3` reads as `1.0 / 3`
with both engines; POI used to return the 15-digit text).
- An integral value keeps all its digits.
- Integer and DECIMAL columns drop computation noise. A value is treated as
noise when it is within 4 ulp of the text Excel shows (`NumberToTextConverter`,
15 significant digits) and that text has at most 13 significant digits, i.e.
the digits Excel dropped were a run of 0s or 9s. So `7.999999999999999` reads
as `8`, `0.30000000000000004` as `0.30`, and `114.99999999999999` (`1.15*100`)
as `115`. A value with 16 or 17 real digits, such as `9.876543210987654`,
`1234567.890123456` or `98765432109876.99`, is not noise and is read exactly,
so BIGINT still truncates rather than rounds. The ulp distance alone was not
enough: for leading digits 8 or 9, many real 16-digit values are within 4 ulp
of their 15-digit text.
The sink uses the same helper to decide number vs text cell, so a DECIMAL
that would not read back unchanged (e.g. `0.30000000000000004`) is written as
text. STRING columns are unchanged. Covered by
`testComputedNumbersReadAsExcelShowsThem` (both engines; formula cells, noisy
number cells, and real 16/17-digit number cells) and
`testXlsReadKeepsNumericPrecision`. The upgrade note and the source doc now
describe this, including formula results.
**7. Hard cast.** Fixed: a non-`BigDecimal` value in a DECIMAL column goes
through `new BigDecimal(value.toString())`, as the old
`Double.parseDouble(value.toString())` path accepted it (covered in the same
`ExcelGeneratorTest` case).
**8. Test comment.** Reworded as you suggested.
Verification, on a JDK 17 container: spotless is clean, and the
`connector-file-base` unit tests pass (391 run, 0 failures, 1 skipped; the two
Excel test classes 32/32). On the previous head bff617912, 7 of the new cases
fail (scale padding, the POI `.xls` formula `1/3`, the noisy INT/DECIMAL reads
and the sink number-vs-text decision); the EasyExcel `.xls` case already passes
there, which matches point 3. The large-exponent sink case crashes the test JVM
there ("too large to fit in a String"). `LocalFileIT#testLocalFileExcel` passes
(6/6).
--
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]
