morningman commented on PR #70121:
URL: https://github.com/apache/doris/pull/70121#issuecomment-6621842977
Review result: **REQUEST_CHANGES** (2 Major findings).
The full review covered commit `13d6ded9e571105f3576a6dc48721878e3e7f69a`.
Before posting, I rechecked the code relevant to these two findings at the
current head, `73dbcc1d452e2beb35e07dfb03fe74ec7c653288`; both remain. This was
a targeted recheck, not a full review of the newer head.
1. **F-01: Major, data-error, regression. Preserve CSV control bytes through
actual task planning.**
The new [escape
assignment](https://github.com/apache/doris/blob/73dbcc1d452e2beb35e07dfb03fe74ec7c653288/fe/fe-core/src/main/java/org/apache/doris/load/routineload/RoutineLoadJob.java#L2202-L2208)
makes ALTER effective on the job, but
[NereidsDataDescription](https://github.com/apache/doris/blob/73dbcc1d452e2beb35e07dfb03fe74ec7c653288/fe/fe-core/src/main/java/org/apache/doris/nereids/load/NereidsDataDescription.java#L422-L423)
converts the byte back to text with `new String(new
byte[]{taskInfo.getEscape()})`. Under UTF-8, `escape='é'` selects byte `C3`;
decoding that incomplete sequence produces U+FFFD, and the subsequent CSV
property analysis selects `EF`. The final scan therefore uses an unrelated
escape byte.
A concrete trigger is an ASCII-created single-table CSV job with
`escape='|'`: pause it, ALTER to `escape='é'`, resume, and load `1\t，\n` into
`(k INT, v VARCHAR(32))`. The full-width comma is `EF BC 8C`, with no requested
`C3` escape. BE removes the unexpected `EF` escape byte after line UTF-8
validation, so the loaded string can contain corrupted bytes `BC 8C`. Before
this PR, the same ALTER left the effective ASCII escape unchanged, so this
input was preserved. The pre-existing high-bit CREATE problem does not
eliminate this newly activated ALTER regression.
Please carry the already interpreted byte through the final scan
attributes without a default-charset byte-to-text-to-byte conversion, and add
coverage that checks the final Thrift text parameters or ingested string. The
current task-info assertions stop before the failing conversion. This is
separate from the earlier metadata-writer and literal-U+FFFD comments: it
occurs during live task planning with input `é`, without replay or a literal
replacement character. The latest CREATE/ALTER rejection of literal U+FFFD does
not prevent this conversion.
2. **F-02: Major, functional-loss, not a new regression. Include the
supported multi-table CSV path in the fix.**
Multi-table CSV jobs use a separate per-table planning path.
[setMultiTableBaseTaskInfo](https://github.com/apache/doris/blob/73dbcc1d452e2beb35e07dfb03fe74ec7c653288/fe/fe-core/src/main/java/org/apache/doris/nereids/load/NereidsStreamLoadTask.java#L371-L390)
copies separators, format and other job settings, but omits both `enclose` and
`escape`. Updating the job fields therefore does not reach these tasks; the
controls remain zero.
For `t(k INT, v VARCHAR(32))`, create a comma-separated multi-table CSV
job, pause it, ALTER `enclose='"'`, resume, and publish `t|1,"a,b"`. After
removing the destination prefix, the plain CSV reader still splits the payload
into three fields instead of loading `(1, 'a,b')`. This omission predates the
PR, but leaves the promised ALTER repair incomplete for a supported parallel
path.
Please copy both controls in the existing overlay and add a multi-table
CSV case that checks the final plan or loaded row after ALTER/replay:
```diff
this.formatType = task.getFormatType();
+ this.enclose = task.getEnclose();
+ this.escape = task.getEscape();
this.stripOuterArray = task.isStripOuterArray();
```
Validation: static source review, including the final planning and BE
CSV-consumption paths. No build, tests, or live ingestion reproduction was run.
The original review covered all five changed files and converged in two rounds
using GPT-6.1 Sol (xhigh).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
