viirya commented on code in PR #58815:
URL: https://github.com/apache/spark/pull/58815#discussion_r4057522412
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -1173,7 +1171,7 @@ case class RegExpInStr(subject: Expression, regexp:
Expression, idx: Expression)
val source = s.toString
val m = getLastMatcher(source, r)
if (m.find) {
- source.codePointCount(0, m.toMatchResult.start()) + 1
+ source.codePointCount(0, m.start()) + 1
Review Comment:
Flagging this one because it's the only site where the swap interacts with
an exception handler, and it's worth recording why it's safe.
`Matcher.start()` and `MatchResult.start()` both throw
`IllegalStateException` when there is no match, but with different messages on
JDK 17 (`"No match available"` vs `"No match found"`). That difference is not
observable here on two counts: the read is guarded by `if (m.find)`, so `first
>= 0` holds and the ISE path is unreachable; and the enclosing `catch` below
matches on type rather than message, so even a thrown ISE would still yield `0`.
I confirmed on both JDKs that a failed `find()` makes `m.start()` and
`m.toMatchResult().start()` throw identically, so the change doesn't shift
which branch runs. No change requested.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala:
##########
@@ -843,9 +842,8 @@ object RegExpExtractBase {
def extractAll(matcher: Matcher, idx: Int, prettyName: String):
GenericArrayData = {
val matchResults = new ArrayBuffer[UTF8String]()
while (matcher.find()) {
- val mr = matcher.toMatchResult
- checkGroupIndex(prettyName, mr.groupCount, idx)
- val group = mr.group(idx)
+ checkGroupIndex(prettyName, matcher.groupCount, idx)
Review Comment:
Minor, description-only: `regexp_count` and `regexp_substr` are
`RuntimeReplaceable` and rewrite to `RegExpExtractAll(..., 0)` and
`RegExpExtract(..., 0)` respectively, so they pick up this improvement
transitively. The PR title and description name only the three direct
functions, which slightly undersells the reach. Both are exercised by
`CollationSQLRegexpSuite`, which passes.
Worth a clause in the description if you touch it again; not worth a rebase
on its own.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
