SEZ9 commented on PR #13012: URL: https://github.com/apache/seatunnel/pull/13012#issuecomment-6593840115
@MindOfqc thanks for the detailed follow-up. On the Javadoc relocation in `5ca1cd51`: the description is what I'd expect — `matchesAsciiKeyword` moved down with the other private helpers after `isKeyword`, and the original Javadoc back on `hasTopLevelSelectWildcard` with no change to the helper body. Pure relocation is fine with me; the pre-existing >100-char lines are not something this PR needs to touch. Two things I need from you before I can call this settled: 1. The verification output in your comment is cut off after `Tests run: 1`. Could you paste the full surefire summary line (failures/errors/skipped) from the run on the new head? One test in `SqlWhereConditionHelperTest` also sounds low — please confirm that is the actual count, or whether the filter only picked up a subset. 2. Your comment refers to "Issue 1 / 2 / 3", but my previous review was itemised as six LOW findings (`PR13012-F1` – `PR13012-F6`), none of which was marked Medium or as a blocker. I don't want to guess at the mapping. Could you say explicitly which of F1–F6 you consider addressed by `5ca1cd51`, and which ones you are proposing to defer as follow-ups? In particular, since the helper is now named `matchesAsciiKeyword`, please confirm whether it does an ASCII-only comparison (F2) rather than `regionMatches(true, …)`, and whether the `columnList` slicing with uppercased-copy lengths (F5) is unchanged. Once I have the full test output and the explicit mapping, I'm happy to proceed with the remaining items as follow-ups where that is agreed. <!-- streview-comment:2237 --> -- 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]
