SEPURI-SAI-KRISHNA opened a new pull request, #12909:
URL: https://github.com/apache/seatunnel/pull/12909
Yes, and thank you for maintaining it: this is a contribution to Apache
SeaTunnel.
### Scope agreed before submission
Per this repository's contribution workflow, the three questions were put to
the developer and answered before any code was written:
- **Issue and acceptance criteria**: closes #12908. A decimal the target ORC
column cannot represent must fail the write, naming the value and the column,
mirroring what the Parquet sink in this module already does. Rejected
alternatives: logging a warning and still writing `NULL`, which keeps losing
data; and adding a config option to choose, which this repository asks not to
do without necessity.
- **Modules and compatibility**: only `connector-file-base`, only
`OrcWriteStrategy`. No option, schema, file layout or API change. In-range
values are written exactly as before.
- **Required validation**: `spotless:apply`, the mandatory `-DskipTests
verify`, the full module unit run, and a mutation matrix. No E2E case, for the
reason given below.
### Purpose of this pull request
The ORC sink wrote `NULL` instead of the value whenever a `DECIMAL` did not
fit the target column, with no exception, warning or log line, so a job
completed successfully while dropping data.
`decimalColumnVector.set(row, HiveDecimal.create(value))` has two silent
null sources. `HiveDecimal.create` returns `null` above ORC's maximum precision
of 38, and `DecimalColumnVector.set` internally calls `enforcePrecisionScale`,
which returns `null` when the value does not fit the precision and scale the
column actually declares. Neither was checked.
Measured on `dev`, writing one row and reading it back:
| value | declared column | before | after |
| --- | --- | --- | --- |
| `123.45` | `decimal(10,2)` | `123.45` | `123.45` |
| 39 digits | `decimal(38,0)` | `null` | fails |
| `12345678901234` | `decimal(10,2)` | `null` | fails |
The third case is the reachable one: a value merely wider than its declared
column, from a narrow or inferred JDBC precision, a sampled schema, a
schema-less source, or a SQL transform that multiplies decimals.
Given the same two inputs the Parquet sink in this module already throws
`AvroTypeException: Cannot encode decimal with precision 39 as max precision
38` and `... precision 16 as max precision 10`, so the two sinks disagreed
about whether silent loss is acceptable. This change makes them agree.
### The change
Apply both guards and fail with the value and the target column:
```java
HiveDecimal hiveDecimal = HiveDecimal.create(decimal);
HiveDecimal enforced =
hiveDecimal == null
? null
: HiveDecimal.enforcePrecisionScale(
hiveDecimal, decimalColumnVector.precision,
decimalColumnVector.scale);
if (enforced == null) {
throw new FileConnectorException(ILLEGAL_ARGUMENT, "Decimal value [...]
does not fit ...");
}
decimalColumnVector.set(row, enforced);
```
**Checking `HiveDecimal.create` alone is not enough**, and the mutation
matrix below pins that: it only catches the precision-above-38 case, while the
reachable case is nulled inside `set()` by `enforcePrecisionScale`.
### Does this PR introduce any user-facing change?
Yes. A job that silently dropped values now fails at the offending row.
Documented in `docs/en` and `docs/zh` `incompatible-changes.md`, with migration
guidance: widen the declared precision or scale, or round the value before the
sink.
### How was this patch tested?
A new test in the existing `OrcWriteStrategyTest`, which had no decimal
coverage at all. It writes a decimal into a declared `decimal(p,s)` column and
reads it back, covering the in-range control, a value wider than the declared
column, and a value beyond ORC's maximum precision of 38, asserting that each
failure names the value and the target column.
Mutation matrix:
| mutation | result |
| --- | --- |
| revert the guard entirely (the original defect) | fails, on the
wider-than-declared case |
| guard `HiveDecimal.create` only (the plausible half-fix) | **still
fails**, on the same case |
The second row is the point: a reviewer or a later refactor might reasonably
check only `create`, and the test rejects that.
Checks run locally:
- `./mvnw spotless:apply` then `spotless:check`: clean
- `./mvnw -q -DskipTests verify -pl <module> -am`: exit 0, covering the
changed module and its dependency closure rather than the whole repository
- `./mvnw test -pl <module>`: **355 tests, 0 failures, 0 errors**
No E2E case is added. ORC does have E2E configs under
`connector-file-local-e2e`, but every overflow here is a failure case and those
configs assert that the job succeeds, so only an in-range control could go
there, which the unit test already pins.
### 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)
* [x] If necessary, please update the documentation to describe the new
feature. https://github.com/apache/seatunnel/tree/dev/docs
* [x] If necessary, please update `incompatible-changes.md` to describe the
incompatibility caused by this PR.
--
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]
