SEZ9 commented on PR #10472: URL: https://github.com/apache/seatunnel/pull/10472#issuecomment-5140456497
Thanks @DanielCarter-stack for the detailed analysis. Responding to both points: **Issue 1 (charTo4ByteLength for CHAR):** I don't think we should apply `charTo4ByteLength` here. In SAP HANA, `CHAR`/`VARCHAR` are ASCII (single-byte) types, which is why the existing `HANA_VARCHAR`/`HANA_ALPHANUM` branches use the raw length, while `NVARCHAR`/`SHORTTEXT` (Unicode, up to 4 bytes per character) get the x4 conversion. Grouping `HANA_CHAR` with the VARCHAR branch is consistent with HANA's actual semantics — applying the x4 multiplier would over-allocate. The MySQL/PostgreSQL/Oracle comparison doesn't hold directly because their `CHAR` types are typically Unicode-capable. So the `convert()` side of this PR looks correct to me as-is. **Issue 2 (reconvert tests):** Small correction — the PR head `22fb8a5c` does add a `CHAR` branch in `reconvert()` (it checks `column.getSourceType()` inside `case STRING:` at `SapHanaTypeConverter.java:414`). But I agree with the underlying concern: that branch is the weakest part of the PR and needs both fixes and test coverage. Concretely, what I'd like to see before merge: 1. **Bound the length** in the new `CHAR` branch like the neighboring types do — cap against HANA's `CHAR` max length, and handle a null `columnLength` explicitly instead of silently emitting `CHAR` (which HANA treats as `CHAR(1)` → truncation). 2. **Guard against cross-dialect source types.** The branch currently matches `sourceType == "CHAR"` from *any* upstream dialect, so a Unicode `CHAR` from e.g. MySQL would be downgraded to HANA's ASCII `CHAR` and can corrupt non-ASCII data. This should be restricted to HANA-originated columns (or fall back to `NVARCHAR`). 3. **Unit tests for `reconvert()`** covering: normal `CHAR(n)`, null `columnLength`, length exceeding the `CHAR` max, and a non-HANA source type. 4. **Fix the e2e additions** — `CHAR_VALUE_10` is declared as `CHAR(15)`, and there's no boundary coverage. Please align the column name/definition and ideally include a non-trivial length case. Happy to re-review once those are addressed. <!-- streview-comment:14 --> -- 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]
