DanielLeens commented on PR #12768: URL: https://github.com/apache/seatunnel/pull/12768#issuecomment-6190132288
Thanks for the thoughtful reply, @JeremyXin, and for being open to adding a warning log. I re-checked the code on head `95c07aff` (still the latest; no new commit since my review), and I would like to respectfully push back on the CSV comparison, because it is the part that decides Issue 1. **CSV header mapping is opt-in, Excel header mapping in this PR is not.** In `CsvReadStrategy` (dev), `getHeaders()` only uses the real file header when `firstLineAsHeader` is true, i.e. when the user sets `csv_use_header_line = true` (default `false`, see `LocalFile.md`). Otherwise it uses the schema column names as the "headers", which is effectively positional. So a CSV job that merely sets `skip_header_row_number = 1` keeps reading by position today. In this PR, any Excel job that merely sets `skip_header_row_number >= 1` is switched to name-based mapping as soon as one header text equals a schema field name. Those existing jobs never asked for name-based mapping, they only asked to skip a header row, and there is no way to opt out. **Why a warning log is not enough:** the affected jobs keep running successfully with `NULL` in the unmatched columns. A WARN line is easy to miss in a production job and the data is already wrong downstream by the time anyone notices. You are right that positional reading is wrong when columns are reordered, but that is a case the user would have had to hit and work around already; the silent-NULL case is a new failure for users whose positional reads were correct. Two small ways to keep your semantics where they matter and still keep existing jobs safe (either one clears Issue 1 for me): 1. **Option A (smallest):** switch to header mapping only when every field of the (projected) schema finds a header; otherwise keep the old positional read and log a WARN listing the unmatched fields. Reordered-column files like your `[b, a, c]` E2E still work, and partially matching legacy files are unchanged. 2. **Option B:** gate the mapping behind an explicit switch (for example reuse `csv_use_header_line` semantics, as you suggest for the follow-up, or a new Excel option, default off), which also matches the CSV precedent exactly. This needs docs in `docs/en` and `docs/zh`. If you prefer to keep the current behavior, it would need to be called out in `incompatible-changes.md` with migration guidance, and I would still consider it a blocker for existing production jobs, so I would recommend A or B. Once a new commit is pushed I will re-check it (together with the docs, tests and trim items from my review). Thanks again for the quick and constructive discussion! -- 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]
