SEPURI-SAI-KRISHNA commented on PR #12618: URL: https://github.com/apache/seatunnel/pull/12618#issuecomment-6073290799
Thanks @DanielLeens, both taken. The first is a better test than mine. **Issue 1, reachability.** You are right, and I verified rather than taking it on trust: - `ModalityType.supportsExtension` has no caller in any `src/main` repo-wide. A grep returns only its own declaration; the two assertions in my test are its only callers. - `ModelInvocationCacheKey` is reached from `ModelInvocationRuntime.buildCacheKey:127`, but `AbstractModel:46` uses the single-argument `new ModelInvocationRuntime(options)`, which installs `ModelInvocationCache.NOOP`. The only implementation under `src/main` is `NoopModelInvocationCache`, whose `get` returns empty and `put` does nothing, so a mangled key has no observable effect in-tree. The description now reads three production-reachable, two latent, six hardening, with callers named, and the summary table at the top is rewritten to match. That table is why this needed saying twice: my earlier correction fixed the lower section and left the summary claiming five, so the description contradicted itself again. Your test, whether a production caller exists at all, is strictly stronger than mine, which only asked what the converted token is compared with. Credited in the description. Both latent changes stay with their tests. The cache key is the one I would least like to drop: it matters only for an out-of-tree `ModelInvocationCache` shared across workers with differing default locales, which is worth protecting against. **Issue 2, the unclosed engine.** Fixed. `CalciteSQLEngine` is `AutoCloseable`, so the test now wraps it in try-with-resources inside the locale block instead of calling `close()` as the last statement of the `try`. Flagging rather than leaving you to notice: that makes mine the only try-with-resources in a file with 316 bare `engine.close()` calls. Happy to use a plain `finally` if you would rather the file stayed uniform. Your call. I checked my other three tests for the same mistake. They construct only `HashMap`, `VectorFieldSpec`, `SrcField` and `MultimodalFieldValue`, none closeable, so it was isolated to the calcite one. Verified after the change: `CalciteSQLEngineTest` 325 tests 0F 0E; full module 1228 tests 0F 0E; reverting all eleven sites still fails exactly those four tests and nothing else, re-run against the current module rather than carried over; spotless clean. On CI, agreed: `all-connectors-it-7 (8)` is `PostgresCDCIT`, the known #11847 flake in a CDC module this diff does not touch. -- 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]
