DanielLeens commented on PR #12661: URL: https://github.com/apache/seatunnel/pull/12661#issuecomment-6143431661
Thanks @hutiefang76 and @SEZ9. I went through @SEZ9's review of head `0070253` again and checked the new points against the code. Head is unchanged (`0070253232`), so this is a follow-up note rather than a new full review. **Sincere apology first:** I missed Issue 1 below in my last round, even though it was in the diff I was reading. Thank you @SEZ9 for catching it. Verification: - `timestamp_format` does not exist as a main-code option: a search of the Java sources at `0070253` only finds it in two test Javadocs (`CsvTextFormatSchemaTest.java:428`, `TextFormatSchemaTest.java:524`). The real file-connector option on both source and sink is `datetime_format` (`FormatOptions.DATETIME_FORMAT_LEGACY`). The new text in `docs/en/connectors/source/LocalFile.md`, `docs/zh/connectors/source/LocalFile.md` and both `incompatible-changes.md` entries says "the sink option `timestamp_format`", so a user following it would look for an option that cannot be set. **+1 to @SEZ9's Issue 1**; please change the name to `datetime_format` (and ideally say that on the source side this option is not used to pick the CSV/Text TIMESTAMP parsing pattern, which is the point of the sentence). - **+1 to @SEZ9's Issues 3 and 4** (the incompatible-changes entry opens a new `###` prose section instead of the page's bullet format under `### Format Changes`, and describes the old CSV error as something that was only "attempted"). Please align it with the neighbouring entries and reference the PR. - **+1 to Issue 2** (the second parse after the re-match is unguarded). This is a Low-to-Medium robustness point: a value that matches a supported regex but is not a valid calendar date escapes as a raw `DateTimeParseException` without field context. The incompatible-changes text already acknowledges it, so wrapping it with `CommonError.formatDateTimeError(field, fieldName)` would make the behaviour match the docs. - Issues 5, 6 and 7 from @SEZ9 overlap with the carried-over items in my last review (alternating precision costs an exception per row; the two `matchTimestampFormatter` helpers cache differently; the note exists only on the LocalFile page). They stay non-blocking. **Updated status:** the code fix itself is still correct and well tested, and I still see no blocker in the runtime path. But because the docs would point users to a non-existent option, I would treat Issue 1 as a must-fix before merge (a one-word change in four places), with Issues 2-4 recommended in the same push. **CI (unchanged from my last review):** the apache-side `Build` check for this head is still `ACTION_REQUIRED` (https://github.com/apache/seatunnel/runs/113884350736), and I cannot find a fork run for `0070253`. Please make sure GitHub Actions is enabled in your fork and push a commit with the doc fix, which will also start CI. The branch is now 21 commits behind `dev` with no conflict; syncing with `dev` in the same push is welcome. Happy to take a quick look once it is updated, and thanks again for the thorough work. -- 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]
