SEPURI-SAI-KRISHNA opened a new pull request, #12969:
URL: https://github.com/apache/seatunnel/pull/12969
Yes, and thank you for maintaining it: this is a contribution to Apache
SeaTunnel.
I am both the reporter and the implementer here, so rather than block on the
three questions the contribution guide asks me to put to the developer, I am
answering them up front.
**Which issue does this change address, and what are the expected behavior
and acceptance criteria?**
Closes #12968. A null cell in a `ROW` or `MAP` column must read back as
`null`, which is what the three list converters already do. A cell holding a
genuinely empty map must keep reading as an empty map, so "absent" and "empty"
stay distinct rather than both collapsing. No other converter behaviour changes.
**Which modules may change, and which APIs, configuration settings and data
formats must remain compatible?**
Only `seatunnel-connectors-v2/connector-common`, in two files. No `Option`,
no SPI signature and no config key changes: the `Converter` interface is
untouched and both converters keep their `@AutoService` registration. The one
observable change is that a null `MAP` cell now yields `null` instead of an
empty map, recorded in the EN and ZH incompatible-changes docs.
**Which build checks, tests and real-environment validations are required
before submission?**
`spotless:apply`, `spotless:check`, `-DskipTests verify`, the
`connector-common` unit tests, and the two consumer modules' unit tests.
Results are in the Validation section. I could not run the Doris or StarRocks
IT locally; see Scope.
## The defect
`ArrowToSeatunnelRowReader` dispatches by `SqlType`: `MAP` to
`MapConverter`, `ROW` to `StructConverter`, `ARRAY` to the list converters. All
three go through the three argument `convert(rowIndex, fieldVector,
genericsConverters)` overload. Two of the five complex converters never checked
the Arrow validity bit there:
- `StructConverter` called `entrySet()` on a null `getObject(rowIndex)`, so
one null struct cell ended the whole read with `NullPointerException`.
- `MapConverter` positioned a `UnionMapReader` and looped on `next()`, which
yields nothing for a null cell, so it returned an empty map.
`ArrowToSeatunnelRowReader` has exactly two consumers in `src/main`,
`DorisValueReader` and `StarRocksBeReadClient`, so both defects reach user data
from a Doris or StarRocks source.
## The change
Six lines of production code: the same two line guard the siblings use,
added to each generics overload.
```java
if (fieldVector.isNull(rowIndex)) {
return null;
}
```
This is deliberately the existing idiom rather than a new one. `#8137`
introduced all five complex converters in a single commit and guarded only
three:
| Converter | two argument overload | generics overload |
|---|---|---|
| `ListConverter` | guards null | guards null |
| `LargeListConverter` | guards null | guards null |
| `FixedSizeListConverter` | guards null | guards null |
| `StructConverter` | guards null | **was missing** |
| `MapConverter` | guards null | **was missing** |
`git log -S'isNull'` on both files shows no guard was ever added to the
generics overload and later removed, so this closes a day one oversight rather
than reversing a decision. I left the guard uncommented to match the three
siblings exactly.
## Tests
A focused class, `ArrowToSeatunnelRowReaderNullCellTest`, beside the
existing `ArrowToSeatunnelRowReaderCloseTest` in the same package. Three cases,
all reading single record batch streams through the production byte array
constructor, so a failure points at the converter dispatch and not at a batch
boundary. Each fixture carries a plain `id` column that must keep its value on
the null row.
- `testNullStructCellReadsAsNull`
- `testNullMapCellReadsAsNull`
- `testEmptyMapCellStaysAnEmptyMap`
The third exists so the guard is pinned as separating absent from empty
rather than collapsing both to `null`. I added it because I was about to assert
that property in the docs without having measured it.
**Mutation evidence.** Reverting one guard at a time fails exactly one test,
and nothing else:
| Mutation | Result |
|---|---|
| both guards present | 3 tests, 0 failures |
| map guard removed | 1 failure (`testNullMapCellReadsAsNull`), 0 errors;
the empty-map test still passes |
| struct guard removed | 1 error (`NullPointerException`), 0 failures |
| neither guard (plain `dev`) | 1 failure and 1 error |
So each guard is independently necessary and neither test masks the other.
**On the test class choice.** The contribution guide asks me to extend an
existing class rather than add one. I chose a new focused class for two
reasons. `ArrowToSeatunnelRowReaderCloseTest` is the precedent for exactly this
in the same package. And the natural alternative,
`ArrowToSeatunnelRowReaderTest`, is rewritten by my own open #12650, so putting
these cases there would make the two PRs conflict for no benefit. Happy to move
them if you would rather.
Incidentally this also retires a TODO: `ArrowToSeatunnelRowReaderTest`
declares a `map` column but never populates it, `writeKeyAndValue` there is
dead code, and the assertion is a `// todo check map` citing apache/arrow#44664
as "The java api has problems building MapVectors". The new fixture builds a
`MapVector` with a populated, a null and an empty cell through the Java API, so
that gap now has a worked example.
## Scope
Documentation is the EN and ZH `incompatible-changes.md` entries. Neither
`Doris.md` nor `StarRocks.md` has a section on complex-type null semantics, and
the `connector-common` changelog is generated from commit history, so there is
no other doc to update.
I did **not** add IT coverage, and I want to be explicit about the gap
rather than leave it implied. `connector-common` has no e2e module of its own
and no e2e test references the Arrow reader. The nearest indirect coverage is
`DorisMultiReadIT`, which declares 14 nullable `MAP` columns but builds its
maps once outside the row loop and inserts the same non-null single entry maps
into all 100 rows, so no null map ever reaches the converter. Adding a null-map
row there would give end-to-end coverage, but I have no Docker socket access in
this environment, so I would be shipping an unverified change into the 180
minute `doris-connector-it` lane. I would rather state that than guess. Say the
word and I will add it, or open it as a test-only follow-up.
## Validation
All on `dev` at `14a27ac26`, JDK `17.0.20.1`, Maven 3.8.4.
| Check | Result |
|---|---|
| `spotless:apply` then `spotless:check` on the module | BUILD SUCCESS, no
tracked file reformatted |
| `-DskipTests verify` on the module | BUILD SUCCESS |
| `connector-common` unit tests | **14 tests**, 0 failures (11 existing + 3
new) |
| `connector-starrocks` unit tests | **94 tests**, 0 failures |
| `connector-doris` unit tests | **152 tests**, 0 failures |
The two consumer modules were run against the patched `connector-common`
installed locally, because the map change is observable to them.
Markdown is excluded from Spotless in the root `pom.xml`, so the doc edits
need no format pass.
## Duplicate search
Searched before implementing and again before opening, on both `gh search
issues` and the separate `gh search prs`, open and closed: `arrow struct null`,
`arrow map null`, `StructConverter null`, `MapConverter null`, `arrow converter
null`, `doris struct null`, `starrocks struct null`, `ROW type null arrow`. All
empty. #11219 touches both converters but only changes their SPI registration.
## Overlap with open PRs
No open PR touches the Arrow converters or the new test file, so the code
change is conflict free.
`incompatible-changes.md` is a different matter: 22 open PRs edit it, and
every one inserts at the top of the `## dev` section, as this one does and as
the project's convention has it. A textual conflict there is expected and
trivial to resolve, including against my own #12909. Whichever lands second
rebases.
--
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]
