rwzdk commented on PR #13012:
URL: https://github.com/apache/seatunnel/pull/13012#issuecomment-6440927152
@SEPURI-SAI-KRISHNA thanks — checked it, and you are right about the shape
of the site but the conclusion is less clean than it looks, so I have not
changed line 279 in this PR. Details, in case they are useful for #12996 too:
I extracted the strip-and-detect part of `hasTopLevelSelectWildcard`
(`SqlWhereConditionHelper.java:276-301`) into a standalone probe and ran 245
column lists over the case-expanding characters (`ff fi fl ffi ffl ſt st ß ʼn Dž dž ΐ ΰ İ
ᵺ`) in each keyword slot, comparing the current rule against the
`regionMatches(true, 0, "DISTINCT ", 0, 9)` rule. Only three inputs behave
differently, and every one needs the odd character **inside the keyword
itself**:
- `diſtinct Q*` and `distinct Q*` — `toUpperCase()` gives `DISTINCT Q*` (11
chars) so the current rule matches and then slices the 10-char original at
index 9, leaving `*` alone: it reports a top-level wildcard that is not there.
With `regionMatches` the keyword is not recognised at all, so the answer flips
to `false`.
- `distİnct *` (U+0130) — the opposite direction, and the one that argues
against the proposed fix. `String.toUpperCase()` keeps `İ` as `İ`, so the
current rule does **not** match. `regionMatches` with `ignoreCase` also falls
back to `Character.toLowerCase` on both sides, and `toLowerCase('İ') ==
toLowerCase('I') == 'i'`, so the replacement **does** match and slices `*` out
of the list — a new false positive introduced by the fix.
So `regionMatches` is not a pure improvement at this site: it removes one
mis-slice and adds another, and both only fire on text where `DISTINCT` is
spelled with a ligature or a Turkish dotted capital — which no dialect behind
`connector-jdbc` tokenises as the keyword, so I cannot build a red test on an
input a real JDBC source could produce. Following "no failing test, no change",
I would rather leave 279 alone here than land a behaviour swap I cannot pin
down.
Your "safe" list I confirmed independently rather than taking on faith: the
`ALL ` branch cannot be reached by an expansion at all — matching a
four-character `ALL ` prefix from fewer than four characters would need one
character to uppercase into part of `ALL `, and none does (`ffl` gives `FFL`, `fl`
gives `FL`), which is why the probe found zero disagreements there;
`hasTopLevelSetOperator` (174) and `hasComplexClause` (339) only ever index
into the uppercased string, so they never mix coordinate systems; and
`normalizeWhereCondition` (554) folds down rather than up and nothing expands
to `w`/`h`/`e`/`r`.
The part I do think is worth doing, and where our two PRs actually meet: all
of these sites want the same primitive — a keyword match that is
ASCII-case-insensitive **and** returns the number of characters it consumed, so
the slice length comes from the same comparison that decided the match. That
subsumes both the `ß`/`ſt` index bugs (#12998, this PR) and the `Locale.ROOT`
bugs (#12995, your PR), and it would also make the `İ` asymmetry above explicit
instead of accidental. If you want to put that helper in #12996 I am happy to
rebase this PR onto it and call it from the two sites I changed.
On the process points: agreed about the conflict — whoever lands second
resolves, and my two sites delete two of your seven `toUpperCase()` calls, so a
rebase is straightforward either way. And CI: as an external contributor I
cannot start the upstream workflows or enable Actions on the fork, so a
committer has to trigger them; noted on spotless needing JDK 17.
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]
