[SPARK-8301][SQL] Improve UTF8String substring/startsWith/endsWith/contains performance #6804

tarekbecker · 2015-06-13T22:37:08Z

Jira: https://issues.apache.org/jira/browse/SPARK-8301

Added the private method startsWith(prefix, offset) to implement startsWith, endsWith and contains without copying the array

I hope that the component SQL is still correct. I copied it from the Jira ticket.

…te function startsWith(prefix, offset) to implement the check for startsWith, endsWith and contains.

rxin · 2015-06-13T22:50:15Z

Jenkins, test this please.

SparkQA · 2015-06-14T00:44:10Z

Test build #34848 has finished for PR 6804 at commit b17909e.

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

rxin · 2015-06-14T03:43:16Z

cc @davies for review.

davies · 2015-06-14T06:11:02Z

@tarekauel Thanks for working on this, could you have some micro benchmark to show the performance difference? It's good to see the improvements by numbers.

davies · 2015-06-14T06:12:35Z

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

        return true;
      }
    }
    return false;
  }

+  private boolean startsWith(final UTF8String prefix, int offset) {


Nit: If we use startsWith(final byte[] prefix, int offset), it will may save some calls of s.getBytes(). (not sure how is the difference).

tarekbecker · 2015-06-14T19:21:25Z

@davies thanks for your feedback.
I added a null check and test cases for null. There are other public methods without null checks. Should I add a null check to them as well?

I did a micro benchmark for starsWith. I created objects for the UTF8String and called the test 1,000,000,000 times.

duration starts with old: 73,586 ms
duration starts with new: 23,246 ms (just 31.6%)

davies · 2015-06-14T21:02:01Z

@tarekauel LGTM. It will be better if you could also add null check for others, thanks for the numbers, good to know the improvements!

tarekbecker · 2015-06-15T01:38:11Z

@davies I added the null checks. If someone compares a UTF8String and null, the null value will come first and the string afterwards. For set I decided to initialise an empty byte array, in order to avoid a crash later.

davies · 2015-06-15T01:45:25Z

@tarekauel I think we could mark set as protected(only used in a few places, include codegen), make sure that the parameters is not null. Could you do it in this PR?

…on of bytes from Nullable to Nonnull

tarekbecker · 2015-06-15T03:45:24Z

@davies I have changed the access modifier and I changed the annotation of bytes to Nonnull from Nullable. I added all non null checks, didn't I?

davies · 2015-06-15T04:03:32Z

@tarekauel Thanks for the change, you need to update all the usage of (new UTF8String().set) to UTF8String.fromString/Bytes, or it can not compile or fail the tests.

SparkQA · 2015-06-15T04:07:06Z

Test build #914 has finished for PR 6804 at commit a5f853a.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

tarekbecker · 2015-06-15T04:13:36Z

@davies done. Intellij found only one reference. Can you trigger the build process again?

davies · 2015-06-15T05:01:28Z

you can git grep UTF8String | grep set

tarekbecker · 2015-06-15T05:09:27Z

There's just one other file sql/core/src/main/scala/org/apache/spark/sql/columnar/ColumnType.scalathis uses already UTF8.fromBytes(...)

davies · 2015-06-15T05:11:32Z

Sorry, it should be git grep stringType | grep set

tarekbecker · 2015-06-15T05:16:24Z

Thanks!

SparkQA · 2015-06-15T06:45:51Z

Test build #915 has finished for PR 6804 at commit e4530d2.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

tarekbecker · 2015-06-15T16:23:14Z

Can someone trigger Jenkins?

rxin · 2015-06-15T18:07:52Z

Jenkins, retest this please.

SparkQA · 2015-06-15T19:52:48Z

Test build #34953 has finished for PR 6804 at commit 3a0040f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

tarekbecker · 2015-06-15T22:27:07Z

@davies I fixed it. The change was correct, wasn't it? Can someone start Jenkins again?

davies · 2015-06-16T02:41:39Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

@@ -437,17 +437,17 @@ case class Cast(child: Expression, dataType: DataType) extends UnaryExpression w

      case (BinaryType, StringType) =>
        defineCodeGen (ctx, ev, c =>
-          s"new ${ctx.stringType}().set($c)")
+          s"new ${ctx.stringType}().fromBytes($c)")


This should be ${ctx.stringType}.fromBytes($c), also others

SparkQA · 2015-06-16T04:15:39Z

Test build #916 has finished for PR 6804 at commit 9f17cc8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

chenghao-intel · 2015-06-17T05:02:11Z

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

-      PlatformDependent.throwException(e);
+  protected UTF8String set(final String str) {
+    if (str == null) {
+      bytes = new byte[0];


This's probably not right, as a null string will be converted into empty string "".
(new UTF8String().set(null).toString() == "") ? Can we revert this change?

It's safe to remove this check, because set is protected now, we can make sure that str is not null.

okay I am going to remove this

chenghao-intel · 2015-06-17T05:25:24Z

@tarekauel Glad to see the big improvement for the performance gain! I have some comments on the API change, e.g. the null value checking, comparison with null, probably we'd better not change its original behavior for UTF8String, seems behavior as java.lang.String does will be reasonable to me, what do you think?

tarekbecker · 2015-06-18T00:46:31Z

@chenghao-intel First of all thanks for all your comments and thoughts. I guess the UTF8String is only used in SPARK SQL, isn't it? Because of that I think the behavior should be similar to a database and not to JAVA. But I don't know if this should be achieved by modifying the SQL layer or this type implementation.

What do you think?

marmbrus · 2015-06-18T01:04:02Z

+1, UTF8String should follow SQL semantics not JVM semantics.

chenghao-intel · 2015-06-18T01:04:28Z

I know the null checking stuff will make our code more safe, but we usually don't do that in the API implementation, as it saves lots of redundant of code, that's almost the general conversion, otherwise we need to make a note in the scaladoc/javadoc.

Expression framework (including the UTF8String) is a quite independent system to SQL, follow the general API design principles will be OK, and I believe it's the responsibility for SQLLayer to make the right calls.

tarekbecker · 2015-06-18T04:19:45Z

Yeah, that's a good comment. I checked the String expressions and it does already the null check, see

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringOperations.scala#L173-L182

So it won't be null and a valid result will be provided, if a value is null. I think the result is even better, because it returns null and not false which is way better.

chenghao-intel · 2015-06-18T13:06:00Z

Probably be Nonnull, this will give more strict check by FindBug. It will be great if you can run FindBug locally after the change.

davies · 2015-06-18T16:45:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

@@ -437,17 +437,17 @@ case class Cast(child: Expression, dataType: DataType) extends UnaryExpression w

      case (BinaryType, StringType) =>
        defineCodeGen (ctx, ev, c =>
-          s"new ${ctx.stringType}().set($c)")
+          s"${ctx.stringType}().fromBytes($c)")


No () here

tarekbecker · 2015-06-21T00:44:16Z

All should be fine now. Can someone trigger Jenkins?

davies · 2015-06-21T00:51:02Z

LGTM, waiting for tests.

SparkQA · 2015-06-21T03:02:13Z

Test build #949 has finished for PR 6804 at commit f5d6b9a.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

[SPARK-8301] removed unnecessary copying of UTF8String. Added a priva…

b17909e

…te function startsWith(prefix, offset) to implement the check for startsWith, endsWith and contains.

davies reviewed Jun 14, 2015
View reviewed changes

[SPARK-8301] null check. Added test cases for null check.

79cb55b

[SPARK-8301] added additional null checks

d2fb05f

[SPARK-8301] changed visibility of set to protected. Changed annotati…

a5f853a

…on of bytes from Nullable to Nonnull

[SPARK-8301] changed call of UTF8String.set to UTF8String.from

e4530d2

[SPARK-8301] changed call of UTF8String.set to UTF8String.from

3a0040f

[SPARK-8301] fixed conversion byte to string in codegen

9f17cc8

davies reviewed Jun 16, 2015
View reviewed changes

[SPARK-8301] removed new

1c327eb

chenghao-intel reviewed Jun 17, 2015
View reviewed changes

tarekbecker added 2 commits June 17, 2015 21:26

[SPARK-8301] removed null checks

9ca0473

[SPARK-8301] removed null checks

6d7b068

davies reviewed Jun 18, 2015
View reviewed changes

fixed parentheses and annotation

f5d6b9a

asfgit closed this in 41ab285 Jun 21, 2015

tarekbecker mentioned this pull request Jul 6, 2015

[SPARK-8269][SQL]string function: initcap #7208

Closed

[SPARK-8301][SQL] Improve UTF8String substring/startsWith/endsWith/contains performance #6804

[SPARK-8301][SQL] Improve UTF8String substring/startsWith/endsWith/contains performance #6804

Conversation

tarekbecker commented Jun 13, 2015

rxin commented Jun 13, 2015

SparkQA commented Jun 14, 2015

rxin commented Jun 14, 2015

davies commented Jun 14, 2015

davies Jun 14, 2015

Choose a reason for hiding this comment

tarekbecker commented Jun 14, 2015

davies commented Jun 14, 2015

tarekbecker commented Jun 15, 2015

davies commented Jun 15, 2015

tarekbecker commented Jun 15, 2015

davies commented Jun 15, 2015

SparkQA commented Jun 15, 2015

tarekbecker commented Jun 15, 2015

davies commented Jun 15, 2015

tarekbecker commented Jun 15, 2015

davies commented Jun 15, 2015

tarekbecker commented Jun 15, 2015

SparkQA commented Jun 15, 2015

tarekbecker commented Jun 15, 2015

rxin commented Jun 15, 2015

SparkQA commented Jun 15, 2015

tarekbecker commented Jun 15, 2015

davies Jun 16, 2015

Choose a reason for hiding this comment

SparkQA commented Jun 16, 2015

chenghao-intel Jun 17, 2015

Choose a reason for hiding this comment

davies Jun 17, 2015

Choose a reason for hiding this comment

tarekbecker Jun 18, 2015

Choose a reason for hiding this comment

chenghao-intel commented Jun 17, 2015

tarekbecker commented Jun 18, 2015

marmbrus commented Jun 18, 2015

chenghao-intel commented Jun 18, 2015

tarekbecker commented Jun 18, 2015

chenghao-intel commented Jun 18, 2015

davies Jun 18, 2015

Choose a reason for hiding this comment

tarekbecker commented Jun 21, 2015

davies commented Jun 21, 2015

SparkQA commented Jun 21, 2015