[SPARK-48551][SQL] Perf improvement for escapePathName

[yaooqinn]

What changes were proposed in this pull request?

This PR improves perf for escapePathName with algorithms briefly described as:

• If a path contains no special characters, we return the original identity instead of creating a new StringBuilder to append char by char
• If a path contains special characters, we relocate the IDX of the first special character. Then initialize the StringBuilder with [0, IDX) of the original string, and do heximal padding if necessary starting from IDX.
• An optimized char-to-hex function replaces the String.format

Add a fast path for storage paths or their parts that do not require escaping to avoid creating a StringBuilder to append per character.

Why are the changes needed?

performance improvement for hotspots

Does this PR introduce any user-facing change?

no

How was this patch tested?

• new tests in ExternalCatalogUtilsSuite
• Benchmark results (9x faster)

Was this patch authored or co-authored using generative AI tooling?

no

[yaooqinn]

cc @cloud-fan

[JoshRosen]

Do we have any unit tests for escapePathName? Or only end-to-end tests? I wonder whether it might be worthwhile to add some basic unit test cases in a new ExternalCatalogUtilsSuite.

[JoshRosen]

We can probably apply similar perf. optimizations to the unescapePathName on the next line below.

[yaooqinn]

Will address this in a separate PR if you don't mind

[yaooqinn]

Do we have any unit tests for escapePathName?

spark/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

Line 1803 in f9542d0

Seq("a b", "a:b", "a%b", "a,b").foreach { specialChars =>

[JoshRosen]

Do we have any unit tests for escapePathName?

spark/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

Line 1803 in f9542d0

Seq("a b", "a:b", "a%b", "a,b").foreach { specialChars =>

Although it's true that those tests indirectly cover these paths, by "unit test" I was referring to a dedicated test of just escapePathString itself which actually asserts over the form of the output. As it stands now, the test in DDLSuite is asserting over some end-to-end functionality to check that the path escaping logic is actually invoked, but that test would not fail if we made breaking or incompatible changes to the escaping logic itself. For example, if we switched from uppercase to lowercase hexadecimal conversions then that change wouldn't cause the DDLSuite test to fail.

In addition, the more "end-to-end" tests are slow to run.

I think it can still be useful to have relatively more end-to-end tests covering important scenarios (since special characters in paths could cause problems at other layers and we might want more end-to-end coverage for such potential cases), but I think we should augment those tests with dedicated unit tests for the helper functions to assert that their actual output doesn't change.

This will be useful if we choose to optimize the int -> hex conversion (which I'll suggest in another comment).

Edit: for example:

import org.apache.spark.SparkFunSuite

class ExternalCatalogUtilsSuite extends SparkFunSuite {
  test("escapePathName") {
    import ExternalCatalogUtils.escapePathName
    assert(escapePathName("a b") === "a b")
    assert(escapePathName("a:b") === "a%3Ab")
    assert(escapePathName("a%b") === "a%25b")
    assert(escapePathName("a,b") === "a,b")
    assert(escapePathName("a/b") === "a%2Fb")


    for (c <- ExternalCatalogUtils.charToEscape.stream().toArray) {
      // Check parity with old conversion technique:
      assert(
        escapePathName(c.toChar.toString) === "%" + f"${c}%02X",
        s"wrong escaping for ${c}")
    }
  }
}

[yaooqinn]

I add some further optimization now the result become

[info] OpenJDK 64-Bit Server VM 17.0.10+0 on Mac OS X 14.5
[info] Apple M2 Max
[info] Escape Tests:                             Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] Legacy                                             5557           5613          36          0.2        5557.3       1.0X
[info] Kent                                                693            712          36          1.4         692.8       8.0X
[info] Josh                                                878            889          10          1.1         878.4       6.3X

I will add the uts you suggested

[JoshRosen]

+1 for performing the hex conversion this way, since the old string formatting was really really expensive:

[JoshRosen]

This optimized version looks good to me overall. Could you check in the new unit test suite and update the PR description to note the improved test coverage?

Let's give this a bit to see if anyone else has feedback.

[yaooqinn]

Gentle ping @dongjoon-hyun @cloud-fan, can you take a look at this improvement when you have some time? Thanks in advance.

[LuciferYang]

LGTM excepts some comments

[LuciferYang]

Merged into master for Spark 4.0. Thanks @yaooqinn and @JoshRosen