pratham76 commented on code in PR #59213:
URL: https://github.com/apache/spark/pull/59213#discussion_r4188005782
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/StringFunctionsBenchmark.scala:
##########
@@ -53,6 +53,18 @@ object StringFunctionsBenchmark extends SqlBasedBenchmark {
doRegexpReplaceBenchmark()
}
benchmark.run()
+
+ val splitBenchmark = new Benchmark("split", N, output = output)
+ splitBenchmark.addCase("split('*-*', '-')", M) { _ =>
+ df.selectExpr("split(subject, '-')").noop()
+ }
+ splitBenchmark.addCase("split('*-*', '[-]')", M) { _ =>
+ df.selectExpr("split(subject, '[-]')").noop()
+ }
+ splitBenchmark.addCase("split('*-*' COLLATE UTF8_LCASE, '-')", M) { _ =>
+ df.selectExpr("split(subject COLLATE UTF8_LCASE, '-')").noop()
+ }
+ splitBenchmark.run()
Review Comment:
This PR bundles three independent things: (1) the core Pattern cache fix,
(2) replicating JDK's single-char fast-path heuristic (splitsWithoutRegex,
UTF8String.java), and (3) this new benchmark. Could we split the fast-path
replication and the benchmark into follow-ups so the core cache fix is easier
to review/backport on its own?"
##########
common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java:
##########
@@ -1720,25 +1742,54 @@ public UTF8String[] splitSQL(UTF8String delimiter, int
limit) {
// as the separator to split string. Java String's split, however, only
accept
// regex as the pattern to split, thus we can quote the delimiter to
escape special
// characters in the string.
- return split(Pattern.quote(delimiter.toString()), limit);
+ return split(Pattern.quote(delimiter.toString()), null, limit);
}
}
- private UTF8String[] split(String delimiter, int limit) {
+ // `compiled` is `delimiter` already compiled, or null to let String.split
compile it.
+ private UTF8String[] split(String delimiter, Pattern compiled, int limit) {
// Java String's split method supports "ignore empty string" behavior when
the limit is 0
// whereas other languages do not. To avoid this java specific behavior,
we fall back to
// -1 when the limit is 0.
if (limit == 0) {
limit = -1;
}
- String[] splits = toString().split(delimiter, limit);
+ // String.split splits around a single-char delimiter with indexOf rather
than a regex, which
+ // beats matching even a pre-compiled Pattern, so only use `compiled`
where it would not.
+ String[] splits =
+ compiled == null || (compiled.flags() == 0 &&
splitsWithoutRegex(delimiter)) ?
+ toString().split(delimiter, limit) : compiled.split(toString(), limit);
Review Comment:
compiled.flags() is recomputed on every call. It's cheap (int field read)
but since we're already caching the Pattern object one level up in StringSplit,
worth a comment noting this is intentionally re-checked per row rather than
cached, so a future reader doesn't assume it's an oversight.
##########
common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java:
##########
@@ -1720,25 +1742,54 @@ public UTF8String[] splitSQL(UTF8String delimiter, int
limit) {
// as the separator to split string. Java String's split, however, only
accept
// regex as the pattern to split, thus we can quote the delimiter to
escape special
// characters in the string.
- return split(Pattern.quote(delimiter.toString()), limit);
+ return split(Pattern.quote(delimiter.toString()), null, limit);
}
}
- private UTF8String[] split(String delimiter, int limit) {
+ // `compiled` is `delimiter` already compiled, or null to let String.split
compile it.
+ private UTF8String[] split(String delimiter, Pattern compiled, int limit) {
// Java String's split method supports "ignore empty string" behavior when
the limit is 0
// whereas other languages do not. To avoid this java specific behavior,
we fall back to
// -1 when the limit is 0.
if (limit == 0) {
limit = -1;
}
- String[] splits = toString().split(delimiter, limit);
+ // String.split splits around a single-char delimiter with indexOf rather
than a regex, which
+ // beats matching even a pre-compiled Pattern, so only use `compiled`
where it would not.
+ String[] splits =
+ compiled == null || (compiled.flags() == 0 &&
splitsWithoutRegex(delimiter)) ?
+ toString().split(delimiter, limit) : compiled.split(toString(), limit);
UTF8String[] res = new UTF8String[splits.length];
for (int i = 0; i < res.length; i++) {
res[i] = fromString(splits[i]);
}
return res;
}
+ /**
+ * Mirrors the check String.split uses to split around `regex` with indexOf
instead of compiling
+ * it: a single char that is not a regex metacharacter, or a backslash
followed by a char that
+ * is not an ASCII letter or digit, and in both cases not a surrogate. This
only picks the faster
+ * of two equivalent calls: otherwise String.split is
Pattern.compile(regex).split(this, limit).
+ */
+ private static boolean splitsWithoutRegex(String regex) {
Review Comment:
This duplicates JDK's internal String.split single-char fast-path detection
(metacharacter set, backslash-escape check, surrogate guard) by hand. If a
future JDK changes that heuristic, this silently diverges (perf-only risk, not
correctness, since Pattern.split and String.split are always equivalent — but
still a maintenance footgun). Could use a one-line comment pointing at the JDK
source/version this was copied from, so it's easier to re-check on JDK upgrades.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/RegexpExpressionsSuite.scala:
##########
@@ -627,6 +629,26 @@ class RegexpExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
StringSplit(Literal("\"quote"), Literal("\"quote"), Literal(-1)) :: Nil)
}
+ test("SPARK-59961: split refreshes its cached pattern when the regex
changes") {
Review Comment:
Good test for cache-invalidation correctness, but it only exercises the
default collation + default legacySplitTruncate=false path. Could we add a case
covering legacySplitTruncate=true and non-UTF8_BINARY collation together with a
regex change.
--
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]
