SEPURI-SAI-KRISHNA commented on PR #12495:
URL: https://github.com/apache/seatunnel/pull/12495#issuecomment-5865327547
Thank you for the thorough review, @DanielLeens.
**Issue 1 is fixed.**
`SQLTransformTest.testEngineOptionValueIsLocaleIndependent` now asserts the
output field names instead of just non-null:
```java
Assertions.assertEquals(
Arrays.asList("id", "name", "age"),
sqlTransform.transformTableSchema().getColumns().stream()
.map(Column::getName)
.collect(Collectors.toList()));
```
You were right that the old assertion passed for the wrong reason. I re-ran
the single-site revert to confirm the tightened version still catches the
regression: with `SQLTransform:engine` reverted the test fails at line 1256
with `IllegalArgumentException`, and it passes with the fix. Module suite is
still 1162 tests, 0 failures, and `spotless:check` is clean.
**On CI, I checked the lanes independently and reached the same conclusion,
with one thing I can pin down further than you could.** Fork run `36321736609`
for head `3628cb15caa` is 75 success, 7 failure, 1 cancelled, 11 skipped.
`unit-test (11, windows-latest)`: the only failure is
`PayPalClientTest.closeWakesRetryWait` (23 tests, 1 failure), the Windows
timing flake that #12444 fixes. In the same job `SeaTunnel : Transforms : V2`
is `SUCCESS [01:40 min]`, and all four touched classes are green on Windows:
`SQLTransformTest` 28, `DateTimeFunctionsTest` 21, `VectorFunctionTest` 9,
`ZetaSQLEngineTest` 7, all 0 failures. That is worth noting on its own, since
it shows the `Locale.setDefault("tr-TR")` tests behave the same on a second
platform.
`transform-v2-it-part-1`: 219 tests, 1 failure,
`TestFilterRowKindIT.testFilterRowKindMultiTable{TestContainer}[2]`, `expected:
<0> but was: <1>`. This is not an inference. It is **#12116**, open since
2026-09-05: "Multi-table row-count rules flaky on Flink: AssertSinkWriter uses
static JVM-wide counters evaluated per-subtask close()". That is exactly this
assertion and exactly this mechanism. I also confirmed
`filter_row_kind_exclude_insert_multi_table.conf` declares only a
`FilterRowKind` transform, and grepping the whole lane log for `SQLTransform`,
`ZetaSQL` and `sql_transform` returns zero hits, so none of the changed code
runs there.
The other five lanes (`all-connectors-it-5` on 8 and 11,
`all-connectors-it-7`, `amazonSqs-connector-it`, `engine-v2-it`, plus the
cancelled `kudu-connector-it`) are connector and engine integration jobs. The
diff is confined to `seatunnel-transforms-v2`, so they cannot reach it.
**Base refreshed.** You were right that the fork base was behind. It was
`4fd80db52`, four commits back. I have merged current `dev` (`146a1b5c5`),
which picks up #12478 (MinIO readiness in `S3FileWithFilterIT`), #12449
(checkpoint trigger dispatch E2E), #12477 and #12487. None of the four touch
`transform/sql/`, so the local results above still apply unchanged. That should
clear several of the integration lanes on the rerun.
On your maintainability suggestion, a checkstyle or forbidden-API rule
banning bare `toUpperCase()` and `toLowerCase()` in `transforms-v2`: I agree
that is the durable version of this fix, and I would rather do it together with
the remaining `validator`, `calcite` and `nlpmodel` sites than bolt it on here.
Happy to open that as a follow-up once this lands.
--
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]
