Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-23762][SQL] UTF8StringBuffer uses MemoryBlock #20871

Closed
wants to merge 3 commits into from
Closed

[SPARK-23762][SQL] UTF8StringBuffer uses MemoryBlock #20871

wants to merge 3 commits into from

Conversation

kiszk
Copy link
Member

@kiszk kiszk commented Mar 21, 2018 •
edited
Loading

What changes were proposed in this pull request?

This PR tries to use MemoryBlock in UTF8StringBuffer. In general, there are two advantages to use MemoryBlock.

  1. Has clean API calls rather than using a Java array or PlatformMemory
  2. Improve runtime performance of memory access instead of using Object.

How was this patch tested?

Added UTF8StringBufferSuite

@SparkQA
Copy link

SparkQA commented Mar 21, 2018

Test build #88462 has finished for PR 20871 at commit d6aaef7.

  • This patch fails RAT tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 21, 2018

Test build #88469 has finished for PR 20871 at commit 7db100f.

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

@kiszk kiszk mentioned this pull request Mar 26, 2018
Closed
@SparkQA
Copy link

SparkQA commented Apr 4, 2018

Test build #88894 has finished for PR 20871 at commit 0d0f271.

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

@kiszk kiszk changed the title [WIP][SPARK-23762][SQL] UTF8StringBuffer uses MemoryBlock [SPARK-23762][SQL] UTF8StringBuffer uses MemoryBlock Apr 6, 2018
@kiszk
Copy link
Member Author

kiszk commented Apr 6, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 6, 2018

Test build #88964 has finished for PR 20871 at commit ba81497.

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

@kiszk
Copy link
Member Author

kiszk commented Apr 6, 2018

retest this please

viirya
viirya reviewed Apr 6, 2018
View reviewed changes
...alyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/UTF8StringBuilder.java Outdated
buffer = tmp;
}
}

private int totalSize() {
return cursor - Platform.BYTE_ARRAY_OFFSET;
return length;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since totalSize is privately used only, and now it is simply length, I think we can remove it and just use length.

@SparkQA
Copy link

SparkQA commented Apr 6, 2018

Test build #88963 has finished for PR 20871 at commit ba81497.

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

@SparkQA
Copy link

SparkQA commented Apr 6, 2018

Test build #88967 has finished for PR 20871 at commit ba81497.

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

viirya
viirya reviewed Apr 6, 2018
View reviewed changes
...src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/UTF8StringBufferSuite.scala Outdated
import org.apache.spark.SparkFunSuite
import org.apache.spark.unsafe.types.UTF8String

class UTF8StringBufferSuite extends SparkFunSuite {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UTF8StringBufferSuite -> UTF8StringBuilderSuite?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

@SparkQA
Copy link

SparkQA commented Apr 6, 2018

Test build #88972 has finished for PR 20871 at commit 1d75cf3.

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

@SparkQA
Copy link

SparkQA commented Apr 6, 2018

Test build #88988 has finished for PR 20871 at commit a7665fd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class UTF8StringBuilderSuite extends SparkFunSuite

@kiszk kiszk mentioned this pull request Apr 7, 2018
Closed
@kiszk
Copy link
Member Author

kiszk commented Apr 7, 2018

ping @cloud-fan

@viirya
Copy link
Member

viirya commented Apr 7, 2018

LGTM

@viirya
Copy link
Member

viirya commented Apr 7, 2018

Btw, it is better to check if any lint-java issue because this involves java change.

@kiszk
Copy link
Member Author

kiszk commented Apr 7, 2018

Thank you for your suggestion. I installed checkstyle plug-in into Intellij today.

@kiszk
Copy link
Member Author

kiszk commented Apr 10, 2018

ping @cloud-fan

@kiszk
Copy link
Member Author

kiszk commented Apr 10, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 10, 2018

Test build #89148 has finished for PR 20871 at commit a7665fd.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class UTF8StringBuilderSuite extends SparkFunSuite

@kiszk
Copy link
Member Author

kiszk commented Apr 11, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 11, 2018

Test build #89171 has finished for PR 20871 at commit a7665fd.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class UTF8StringBuilderSuite extends SparkFunSuite

@kiszk
Copy link
Member Author

kiszk commented Apr 11, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 11, 2018

Test build #89191 has finished for PR 20871 at commit a7665fd.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class UTF8StringBuilderSuite extends SparkFunSuite

@kiszk
Copy link
Member Author

kiszk commented Apr 11, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 11, 2018

Test build #89196 has finished for PR 20871 at commit a7665fd.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class UTF8StringBuilderSuite extends SparkFunSuite

@kiszk
Copy link
Member Author

kiszk commented Apr 11, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 11, 2018

Test build #89211 has finished for PR 20871 at commit a7665fd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class UTF8StringBuilderSuite extends SparkFunSuite

@kiszk
Copy link
Member Author

kiszk commented Apr 12, 2018

ping @cloud-fan

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@asfgit asfgit closed this in 0b19122 Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viirya viirya viirya left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kiszk @SparkQA @viirya @cloud-fan