MindOfqc commented on PR #13012:
URL: https://github.com/apache/seatunnel/pull/13012#issuecomment-6575889741
Thanks for the full static re-review and for the "ready to merge after
fixes" read. Issue 1 is fixed in `5ca1cd51`; Issues 2 and 3 are accepted as
follow-ups, agreed with your and @SEZ9's framing.
**Issue 1 (Medium, the blocker) — the helper no longer splits the Javadoc
from its method.** `matchesAsciiKeyword` was inserted between the existing
comment block and `hasTopLevelSelectWildcard`, so the public method lost its
documentation. It now sits at the bottom of the class, after `isKeyword`,
alongside the other private helpers, and the original Javadoc is back on the
public method:
```java
/**
* Check if SELECT clause has top-level wildcard (* or table.*). Does
NOT match wildcards inside
* expressions like COUNT(*), col*2, SUM(a*b).
*/
public static boolean hasTopLevelSelectWildcard(String sql) {
```
This is a pure relocation — no character of the helper body changed, and the
file is still 602 lines. The two lines above 100 characters in the file (`43`
and `423`) are pre-existing, not introduced by any commit of this PR.
Verification on the new head:
```
mvn -o -pl seatunnel-connectors-v2/connector-jdbc
-Dtest=SqlWhereConditionHelperTest -Dspotless.check.skip=true test
Tests run: 127, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
```
Spotless still cannot run here (`spotless-maven-plugin 2.29.0` calls
`JCImport.getQualifiedIdentifier()`, which JDK 21 removed), so I hand-checked
AOSP placement and the 100-column limit instead; please let CI confirm
formatting.
**Issue 2 (Low) — I am leaving `hasTopLevelSetOperator` (`:175`) and
`hasComplexClause` (`:365`) on their uppercase copies**, exactly as you
assessed: their indices never slice the original string, so there is no drift
defect, and switching them would widen this PR past the reported bug. Your
second option is cheaper and honest, so I noted the boundary where the ASCII
matcher is used:
```java
// Keywords are matched on the original string so the indices stay
aligned with the
// text that is sliced below; uppercasing is not length-preserving
(e.g. 'ß' -> "SS").
```
That comment is the one that already sits above the changed `SELECT` scan,
and it is the slicing scanners it describes. If you would like the "only the
two slicing scanners use the ASCII matcher" sentence stated explicitly rather
than implied, I will add it as a one-line comment in a follow-up commit.
**Issue 3 (Low) — agreed, and I concur with @SEZ9 on the same point.** The
Unicode wildcard path is pinned by unit tests that assert the exact string the
helper produces; a real-database fixture needs the case-expanding alias inside
the projection plus a matching alias column in the sink DDL, and neither can be
validated here (no Docker). I would rather add it in a small follow-up PR that
can actually show the MySQL IT passing than push an unverifiable e2e change
into this one.
**On the fallback in your Issue 3 from @SEZ9's review**: also agreed to keep
out of this PR.
CI is still the open item, and it is not something I can unblock from my
side: the fork rejects both `push` and `workflow_dispatch` with `422 Actions
has been disabled for this repository.` even though `GET
/repos/MindOfqc/seatunnel/actions/permissions` reports `enabled: true,
allowed_actions: "all"`. The support-case write-up is finished on my side (the
same 422/204/GET evidence plus the element-plus fork contrast), and I have not
touched the head otherwise — no empty commit, no branch sync, consistent with
your "no failure to attribute yet". The current head for the run you would need
is `5ca1cd51`.
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]
