SEZ9 commented on PR #12618: URL: https://github.com/apache/seatunnel/pull/12618#issuecomment-6029404625
Thanks @SEPURI-SAI-KRISHNA. Agreed on the split: keeping the lint-rule guard out of this PR and tracking it in #12620, sequenced after this PR and the other fix land so the rule is evaluated against the post-fix module rather than the 24-site baseline on `dev` `af80a6704`, makes sense to me. Nothing further needed on that here; we can continue the guard discussion on #12620. On the red `Build` for run `37140738764` (head `48599a8f6`): your breakdown is convincing. The three failing integration tests (`TestFilterRowKindIT`, `BackpressureSlowSinkIT`, `PaimonWithS3IT`) each map to an already-tracked issue, and I agree that swapping `toUpperCase()` / `toLowerCase` for the `Locale.ROOT` overloads cannot change a row count, a checkpoint, or an S3 privilege check on a non-Turkish/Azerbaijani/Lithuanian runner. No need for a blind rerun. What I would still like before merging: 1. A rebase onto current `dev` to pick up whatever has landed since `48599a8f6`. If the Flink leg of `transform-v2-it-part-1` fails again with the same `expected: <0> but was: <1>` signature in `TestFilterRowKindIT`, I am fine treating that as the known issue and not blocking on it. 2. A quick confirmation that the 11 `src/main` call sites are the complete set in calcite, nlpmodel and validator, i.e. no `toUpperCase()` / `toLowerCase` call without a `Locale` remains in those three modules after this change. A one-line grep result in the PR description is enough, and it gives #12620 its starting point. Once the rebase is pushed I will do the review pass on the diff itself. <!-- streview-comment:1564 --> -- 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]
