[SPARK-28023][SQL] Add trim logic in UTF8String's toInt/toLong to make it consistent with other string-numeric casting #26622

yaooqinn · 2019-11-21T06:26:11Z

What changes were proposed in this pull request?

Modify UTF8String.toInt/toLong to support trim spaces for both sides before converting it to byte/short/int/long.

With this kind of "cheap" trim can help improve performance for casting string to integrals. The idea is from #24872 (comment)

Why are the changes needed?

make the behavior consistent.

Does this PR introduce any user-facing change?

yes, cast string to an integral type, and binary comparison between string and integrals will trim spaces first. their behavior will be consistent with float and double.

How was this patch tested?

add ut.
benchmark tests
the benchmark is modified based on [SPARK-28023][SQL] Trim the string when cast string type to Boolean/Numeric types #24872 (comment)

/*
 * Licensed to the Apache Software Foundation (ASF) under one or more
 * contributor license agreements.  See the NOTICE file distributed with
 * this work for additional information regarding copyright ownership.
 * The ASF licenses this file to You under the Apache License, Version 2.0
 * (the "License"); you may not use this file except in compliance with
 * the License.  You may obtain a copy of the License at
 *
 *    http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package org.apache.spark.sql.execution.benchmark

import org.apache.spark.benchmark.Benchmark

/**
 * Benchmark trim the string when casting string type to Boolean/Numeric types.
 * To run this benchmark:
 * {{{
 *   1. without sbt:
 *      bin/spark-submit --class <this class> --jars <spark core test jar> <spark sql test jar>
 *   2. build/sbt "sql/test:runMain <this class>"
 *   3. generate result: SPARK_GENERATE_BENCHMARK_FILES=1 build/sbt "sql/test:runMain <this class>"
 *      Results will be written to "benchmarks/CastBenchmark-results.txt".
 * }}}
 */
object CastBenchmark extends SqlBasedBenchmark {
This conversation was marked as resolved by yaooqinn

  override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
    val title = "Cast String to Integral"
    runBenchmark(title) {
      withTempPath { dir =>
        val N = 500L << 14
        val df = spark.range(N)
        val types = Seq("int", "long")
        (1 to 5).by(2).foreach { i =>
          df.selectExpr(s"concat(id, '${" " * i}') as str")
            .write.mode("overwrite").parquet(dir + i.toString)
        }

        val benchmark = new Benchmark(title, N, minNumIters = 5, output = output)
        Seq(true, false).foreach { trim =>
          types.foreach { t =>
            val str = if (trim) "trim(str)" else "str"
            val expr = s"cast($str as $t) as c_$t"
            (1 to 5).by(2).foreach { i =>
              benchmark.addCase(expr + s" - with $i spaces") { _ =>
                spark.read.parquet(dir + i.toString).selectExpr(expr).collect()
              }
            }
          }
        }
        benchmark.run()
      }
    }
  }
}

benchmark result.

normal trim v.s. trim in toInt/toLong

================================================================================================
Cast String to Integral
================================================================================================

Java HotSpot(TM) 64-Bit Server VM 1.8.0_231-b11 on Mac OS X 10.15.1
Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
Cast String to Integral:                  Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
cast(trim(str) as int) as c_int - with 1 spaces          10220          12994        1337          0.8        1247.5       1.0X
cast(trim(str) as int) as c_int - with 3 spaces           4763           8356         357          1.7         581.4       2.1X
cast(trim(str) as int) as c_int - with 5 spaces           4791           8042         NaN          1.7         584.9       2.1X
cast(trim(str) as long) as c_long - with 1 spaces           4014           6755         NaN          2.0         490.0       2.5X
cast(trim(str) as long) as c_long - with 3 spaces           4737           6938         NaN          1.7         578.2       2.2X
cast(trim(str) as long) as c_long - with 5 spaces           4478           6919        1404          1.8         546.6       2.3X
cast(str as int) as c_int - with 1 spaces           4443           6222         NaN          1.8         542.3       2.3X
cast(str as int) as c_int - with 3 spaces           3659           3842         170          2.2         446.7       2.8X
cast(str as int) as c_int - with 5 spaces           4372           7996         NaN          1.9         533.7       2.3X
cast(str as long) as c_long - with 1 spaces           3866           5838         NaN          2.1         471.9       2.6X
cast(str as long) as c_long - with 3 spaces           3793           5449         NaN          2.2         463.0       2.7X
cast(str as long) as c_long - with 5 spaces           4947           5961        1198          1.7         603.9       2.1X

yaooqinn · 2019-11-21T06:27:28Z

master branch non casting logic modified, cast result will be null

================================================================================================
Benchmark trim the string
================================================================================================

Java HotSpot(TM) 64-Bit Server VM 1.8.0_231-b11 on Mac OS X 10.15.1
Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
Benchmark trim the string:                Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
cast(str as long) as c_long                        3969           4401         402          1.0         968.9       1.0X
cast(str as double) as c_double                    3048           4448        1985          1.3         744.1       1.3X
cast(str as decimal) as c_decimal                 10042          11716         NaN          0.4        2451.8       0.4X

yaooqinn · 2019-11-21T06:32:50Z

scala> java.lang.Double.valueOf(" 234 ")
res1: Double = 234.0


scala> " 234 ".toDouble
res3: Double = 234.0

Double and float 's supported without trim both codegen and non-codegen

yaooqinn · 2019-11-21T06:57:09Z

sql/core/benchmarks/CastBenchmark-results.txt

-cast(str as int) as c_int                          3169           3530         610          1.3         773.6       1.0X
-cast(str as long) as c_long                        1812           1881          60          2.3         442.4       1.7X
+cast(str as int) as c_int                          2208           4341        1848          1.9         539.0       1.0X
+cast(str as long) as c_long                        2039           3450        2146          2.0         497.8       1.1X


@cloud-fan the result seems not as we expected, I'd increase the cardinality and do another test round. Can you help me to see if I missed something?

cloud-fan · 2019-11-21T07:03:21Z

sql/core/benchmarks/CastBenchmark-results.txt

+Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
+Benchmark trim the string:                Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
+------------------------------------------------------------------------------------------------------------------------
+cast(str as int) as c_int                          2208           4341        1848          1.9         539.0       1.0X


what's the result of master branch?

cloud-fan · 2019-11-21T07:05:17Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+   *
+   * @return this string with no spaces at the start or end
+   */
+  public UTF8String nonCopyTrim() {


another way is to embed the trim logic into toInt.

I will check this

yaooqinn · 2019-11-21T07:10:30Z

sql/core/benchmarks/CastBenchmark-results.txt

-cast(str as int) as c_int                          2478           3669        1046          1.7         604.9       1.0X
-cast(str as long) as c_long                        1439           1548          94          2.8         351.4       1.7X
+cast(str as int) as c_int                          3169           3530         610          1.3         773.6       1.0X
+cast(str as long) as c_long                        1812           1881          60          2.3         442.4       1.7X


@cloud-fan this is master branch to original trim result.

yaooqinn · 2019-11-21T07:15:11Z

sql/core/benchmarks/CastBenchmark-results.txt

-cast(str as int) as c_int                          7105           8190         945          1.2         867.3       1.0X
-cast(str as long) as c_long                        7520           8670        1629          1.1         918.0       0.9X
+cast(str as int) as c_int                          6263           8132         NaN          1.3         764.6       1.0X
+cast(str as long) as c_long                        8199           9737         NaN          1.0        1000.9       0.8X


cardinality * 2, (-) is trim (+) is non-copy trim, the cost for copyMemory an int or long value is trial, I guess.

SparkQA · 2019-11-21T08:05:01Z

Test build #114207 has finished for PR 26622 at commit 05ab098.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-21T08:05:02Z

Test build #114211 has finished for PR 26622 at commit 4fa535b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-21T08:05:02Z

Test build #114213 has finished for PR 26622 at commit cce5a55.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-21T08:05:02Z

Test build #114210 has finished for PR 26622 at commit ee94f98.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2019-11-21T08:14:42Z

Java HotSpot(TM) 64-Bit Server VM 1.8.0_231-b11 on Mac OS X 10.15.1
Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
Benchmark trim the string:                Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
Cast String to Numeric:                   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
### nomal trim
cast(str as int) as c_int                          6263           8132         NaN          1.3         764.6       1.0X
cast(str as long) as c_long                        8199           9737         NaN          1.0        1000.9       0.8X
### inside trim
cast(str as int) as c_int                          4015           6735         412          2.0         490.1       1.0X
cast(str as long) as c_long                        8251           8597         302          1.0        1007.1       0.5X

cc @cloud-fan

cloud-fan · 2019-11-21T08:16:59Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/CastBenchmark.scala

+        val N = 500L << 14
+        val df = spark.range(N)
+        val types = Seq("int", "long")
+        df.selectExpr(s"concat('${" " * 5}', id, '${" " * 5}') as str")


can we only benchmark spaces at right? The parsing logic will return immediately if the first char is a space, so not very useful to benchmark it.

yaooqinn · 2019-11-21T08:57:43Z

normal trim v.s. inside trim

The result shows 10~20% perfomance improvement @cloud-fan

[info] Java HotSpot(TM) 64-Bit Server VM 1.8.0_231-b11 on Mac OS X 10.15.1
[info] Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
[info] Cast String to Numeric:                   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] cast(trim(str) as int) as c_int                    4259           5270        1110          1.9         519.9       1.0X
[info] cast(trim(str) as long) as c_long                  4081           5663        1372          2.0         498.2       1.0X
[info] cast(str as int) as c_int                          4071           4254         176          2.0         496.9       1.0X
[info] cast(str as long) as c_long                        4121           5087        1272          2.0         503.1       1.0X
[info]

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

yaooqinn · 2019-11-21T09:02:36Z

normal trim v.s . no trim

[info] Java HotSpot(TM) 64-Bit Server VM 1.8.0_231-b11 on Mac OS X 10.15.1
[info] Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
[info] Cast String to Integral:                  Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] cast(trim(str) as int) as c_int                    4174           6090        1434          2.0         509.6       1.0X
[info] cast(trim(str) as long) as c_long                  5940           6829        1113          1.4         725.1       0.7X
[info] cast(str as int) as c_int                          2932           4191        1242          2.8         357.9       1.4X
[info] cast(str as long) as c_long                        3202           4767         NaN          2.6         390.8       1.3X
[info]

yaooqinn · 2019-11-21T09:23:09Z

normal trim v.s. inside trim in toInt and toLong

[info] Java HotSpot(TM) 64-Bit Server VM 1.8.0_231-b11 on Mac OS X 10.15.1
[info] Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
[info] Cast String to Integral:                  Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] cast(trim(str) as int) as c_int                    4026           7674         NaN          2.0         491.5       1.0X
[info] cast(trim(str) as long) as c_long                  4022           7887        1345          2.0         491.0       1.0X
[info] cast(str as int) as c_int                          4390           6453        1009          1.9         535.9       0.9X
[info] cast(str as long) as c_long                        3759           5388        1413          2.2         458.8       1.1X
[info]

yaooqinn · 2019-11-21T09:31:30Z

@cloud-fan if this works, we may need to fix Decimal separately

cloud-fan · 2019-11-21T13:59:07Z

I think the overhead is acceptable to make the behavior consistent. what do you think? @gatorsmile @srowen @dongjoon-hyun @MaxGekk @wangyum

wangyum · 2019-11-21T14:27:44Z

+1 To keep the behavior consistent.

yaooqinn · 2019-11-21T14:54:12Z

Shall we handle control characters for integral type here as approximate numerics can do? #26626 (comment)

SparkQA · 2019-11-21T15:09:29Z

Test build #114242 has finished for PR 26622 at commit 09f38f7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-21T17:53:58Z

Test build #114246 has finished for PR 26622 at commit 76639b7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-11-22T02:42:18Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

-    }
+    if (this.numBytes == 0) return false;
+    int offset = 0;
+    while (offset < this.numBytes && getByte(offset) == ' ') offset++;


let's handle control characters to be consistent with casting to float/double

cloud-fan · 2019-11-22T02:43:14Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+    int end = this.numBytes - 1;
+    while (end > offset && getByte(end) == ' ') end--;
+
+    int numBytes = end - offset + 1;


if it's only used once, we can inline it.

cloud-fan · 2019-11-22T03:19:05Z

@yaooqinn let's add a migration guide. I think pretty close now.

SparkQA · 2019-11-22T05:53:47Z

Test build #114267 has finished for PR 26622 at commit f37f467.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2019-11-22T06:23:24Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java


-    byte b = getByte(0);
+    int end = this.numBytes - 1;
+    while (end > offset && getByte(end) <= ' ') end--;


You don't need to trim from the right explicitly here. Just break inside the loop https://github.com/apache/spark/pull/26622/files#diff-d2b5337b91f684b9e7fd5cc101e93fc8R1104 if b == ' '

Hm, I guess not, how do you know the ' ' is in the middle or end?

SparkQA · 2019-11-22T08:05:01Z

Test build #114275 has finished for PR 26622 at commit d5c2a40.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2019-11-22T08:06:18Z

retest this please

SparkQA · 2019-11-22T10:42:44Z

Test build #114284 has finished for PR 26622 at commit d5c2a40.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-11-22T11:32:45Z

thanks, merging to master!

yaooqinn · 2019-11-22T11:35:06Z

thanks for merging

…t handle non-ASCII characters correctly ### What changes were proposed in this pull request? The trim logic in Cast expression introduced in #26622 trim non-ASCII characters unexpectly. Before this patch ![image](https://user-images.githubusercontent.com/1312321/89513154-caad9b80-d806-11ea-9ebe-17c9e7d1b5b3.png) After this patch ![image](https://user-images.githubusercontent.com/1312321/89513196-d731f400-d806-11ea-959c-6a7dc29dcd49.png) ### Why are the changes needed? The behavior described above doesn't make sense, and also doesn't consistent with the behavior when cast a string to double/float, as well as doesn't consistent with the behavior of Hive ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? Added more UT Closes #29375 from WangGuangxin/cast-bugfix. Authored-by: wangguangxin.cn <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…t handle non-ASCII characters correctly The trim logic in Cast expression introduced in apache#26622 trim non-ASCII characters unexpectly. Before this patch ![image](https://user-images.githubusercontent.com/1312321/89513154-caad9b80-d806-11ea-9ebe-17c9e7d1b5b3.png) After this patch ![image](https://user-images.githubusercontent.com/1312321/89513196-d731f400-d806-11ea-959c-6a7dc29dcd49.png) The behavior described above doesn't make sense, and also doesn't consistent with the behavior when cast a string to double/float, as well as doesn't consistent with the behavior of Hive Yes Added more UT Closes apache#29375 from WangGuangxin/cast-bugfix. Authored-by: wangguangxin.cn <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

… did't handle non-ASCII characters correctly ### What changes were proposed in this pull request? This is a backport of #29375 The trim logic in Cast expression introduced in #26622 trim non-ASCII characters unexpectly. Before this patch ![image](https://user-images.githubusercontent.com/1312321/89513154-caad9b80-d806-11ea-9ebe-17c9e7d1b5b3.png) After this patch ![image](https://user-images.githubusercontent.com/1312321/89513196-d731f400-d806-11ea-959c-6a7dc29dcd49.png) ### Why are the changes needed? The behavior described above doesn't make sense, and also doesn't consistent with the behavior when cast a string to double/float, as well as doesn't consistent with the behavior of Hive ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? Added more UT Closes #29393 from WangGuangxin/cast-bugfix-branch-3.0. Authored-by: wangguangxin.cn <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

yaooqinn added 2 commits November 21, 2019 14:22

[WIP][SPARK-28023][Test] Cheap UTF8String Trim

48d5a9f

benchmark without trim

05ab098

yaooqinn added 3 commits November 21, 2019 14:38

benchmark without trim

c769c40

benchmark with copy trim

ee94f98

benchmark with non copy trim

4fa535b

yaooqinn commented Nov 21, 2019

View reviewed changes

benchmark with non copy trim r2

2261433

cloud-fan reviewed Nov 21, 2019

View reviewed changes

yaooqinn commented Nov 21, 2019

View reviewed changes

benchmark with copy trim r2

cce5a55

yaooqinn commented Nov 21, 2019

View reviewed changes

inside trim

4fd810a

cloud-fan reviewed Nov 21, 2019

View reviewed changes

yaooqinn added 2 commits November 21, 2019 16:25

rm left spaces

cf2c243

trim v.s inside trim

98892fe

cloud-fan reviewed Nov 21, 2019

View reviewed changes

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java Show resolved Hide resolved

method local

d0bade1

benchmark

5b75d37

clean benchmark

76639b7

yaooqinn changed the title ~~[WIP][SPARK-28023][Test] Cheap UTF8String Trim~~ [SPARK-28023][SQL] Add trim logic in UTF8String's toInt/toLong to make it consistent with other string-numeric casting Nov 21, 2019

dongjoon-hyun added the SQL label Nov 21, 2019

cloud-fan reviewed Nov 22, 2019

View reviewed changes

control chars

f37f467

cloud-fan approved these changes Nov 22, 2019

View reviewed changes

m guide

d5c2a40

MaxGekk reviewed Nov 22, 2019

View reviewed changes

cloud-fan closed this in 2dd6807 Nov 22, 2019

yaooqinn deleted the cheapstringtrim branch November 22, 2019 11:34

WangGuangxin mentioned this pull request Aug 6, 2020

[SPARK-32559][SQL]Fix the trim logic in UTF8String.toInt/toLong did't handle non-ASCII characters correctly #29375

Closed

WangGuangxin mentioned this pull request Aug 9, 2020

[SPARK-32559][SQL][3.0] Fix the trim logic in UTF8String.toInt/toLong did't handle non-ASCII characters correctly #29393

Closed

sarutak mentioned this pull request Jul 11, 2021

[SPARK-36081][SPARK-36066][SQL] Update the document about the behavior change of trimming characters for cast #33287

Closed

[SPARK-28023][SQL] Add trim logic in UTF8String's toInt/toLong to make it consistent with other string-numeric casting #26622

[SPARK-28023][SQL] Add trim logic in UTF8String's toInt/toLong to make it consistent with other string-numeric casting #26622

Uh oh!

Conversation

yaooqinn commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

benchmark result.

Uh oh!

yaooqinn commented Nov 21, 2019

master branch non casting logic modified, cast result will be null

Uh oh!

yaooqinn commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

yaooqinn commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Nov 21, 2019

normal trim v.s. inside trim

Uh oh!

Uh oh!

yaooqinn commented Nov 21, 2019

normal trim v.s . no trim

Uh oh!

yaooqinn commented Nov 21, 2019

normal trim v.s. inside trim in toInt and toLong

Uh oh!

yaooqinn commented Nov 21, 2019

Uh oh!

cloud-fan commented Nov 21, 2019

Uh oh!

wangyum commented Nov 21, 2019

Uh oh!

yaooqinn commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yaooqinn commented Nov 21, 2019 •

edited

Loading