MindOfqc commented on PR #13012:
URL: https://github.com/apache/seatunnel/pull/13012#issuecomment-6569099974
Thanks for the detailed re-review at `4895cefa0bde` — the six points are all
fair, and each one below is answered with what I changed or why it stays open.
**Issue 2 and Issue 5 are fixed together in `b70ce8e9`.**
`SqlWhereConditionHelper` now matches every keyword through one private helper:
```java
private static boolean matchesAsciiKeyword(String sql, int offset, String
keyword)
```
It folds case only for characters below `0x80`, so `U+017F` (`ſ`) is no
longer read as `S` and `U+FB05` (`ſt`) is no longer read as `ST`. It replaces
all four comparisons: `cleaned.regionMatches(true, i, "SELECT", 0, 6)` (was
line 274), `sql.regionMatches(true, i, "FROM", 0, 4)` (was line 561), and the
`DISTINCT `/`ALL ` skip, which no longer validates a prefix on a
`toUpperCase(Locale.ROOT)` copy and then slices the original with the copy's
offset — so the comment at lines 241-242 now holds for the whole method, which
is what you asked for.
Both behaviours were pinned with a failing assertion first, before the
change:
```
testHasTopLevelSelectWildcard_KeywordLettersStayAscii
expected: <false> but was: <true> ("ſelect * from t")
testHasTopLevelSelectWildcard_DistinctSkipStaysAscii
expected: <false> but was: <true> ("select diſtinct t.* from t")
```
The second one is exactly the mis-slice you described: the uppercase copy of
`diſtinct` is one character longer than the token, so `substring(9)` ate the `t`
and left a bare `.*`-shaped remainder that the `table.*` check accepted. After
the fix the ligature is not a keyword letter, the token stays intact, and
neither case is reported as a top-level wildcard.
Regression run: `mvn -o -pl seatunnel-connectors-v2/connector-jdbc
-Dtest=SqlWhereConditionHelperTest -Dspotless.check.skip=true test` → `Tests
run: 127, Failures: 0, Errors: 0` (125 as you counted, plus the two new
assertions). I hand-formatted to the AOSP/100-column rules because spotless
cannot run on JDK 21 here (`spotless-maven-plugin 2.29.0` calls the removed
`JCImport.getQualifiedIdentifier()`), so please let CI confirm formatting.
**Issue 4 is fixed in `431b9d81`.** The `where_condition` rows in
`docs/en/connectors/source/Jdbc.md` and `docs/zh/connectors/source/Jdbc.md` now
say that a column referenced only by the filter is appended before the main
`FROM`, and that the append is skipped for UNION/INTERSECT/EXCEPT, GROUP
BY/HAVING, DISTINCT, LIMIT, window-function and CTE queries.
**Issue 1 is correct as filed — that fixture cannot reproduce #12998.**
`select * from source.unicode_column_table` reaches the helper as pure ASCII,
so the pre-fix copy was length-preserving and the old code behaved identically;
it also duplicates `jdbc_mysql_source_and_sink_with_multiple_tables.conf`. I am
taking your second option: move the case-expanding identifier into the
projection (`select c_varchar as `größe`, t.* from source.unicode_column_table
t`) and add the matching alias column to the sink DDL, so
`hasTopLevelSelectWildcard` really is pinned against the duplicate-column
signature you traced. I did not fold that edit into this push: Docker is
unavailable on this machine, so I cannot run the MySQL IT or even confirm the
sink DDL accepts the new column, and I would rather post it with a run than
push an unverified e2e change.
**Issue 6 is also correct, and it is the next commit.** Splitting
`testMysqlJdbcWhereConditionWildcardAndInsertedColumn` into two `@TestTemplate`
methods (each `clearUnicodeSinkTable()` → `executeJob` →
`assertUnicodeTableProjection()`) is mechanical, and it becomes strictly more
useful once the wildcard fixture carries the Unicode projection, since the two
branches then have independent signals. Same reason it is not in this push: no
local MySQL to confirm the split methods still pass.
**Issue 3 — agreed, and I would keep it out of this PR.** The after-`SELECT`
fallback is reachable independently of case expansion (`select a from(select
...) x` fails the whitespace terminator at lines 540-541), and turning it into
"log and return `sql` unchanged" changes behaviour for every query whose main
`FROM` the scanner cannot locate, not just Unicode ones. That deserves its own
issue and its own review; this PR stays focused on removing the trigger. Happy
to open that follow-up if you want it tracked here rather than in a new ticket.
New head: `431b9d81`. Both commits touch only `connector-jdbc` sources, its
unit test, and the two Jdbc docs, so the exact-head `Build` still needs a
maintainer-triggered run — the fork-side `workflow_dispatch` remains
repository-level blocked, and I have left the code and head alone otherwise as
you asked.
Prepared with an AI coding agent.
--
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]
