Skip to content

[SQL][SPARK-17490] Optimize SerializeFromObject() for a primitive array #15044

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

Closed
kiszk wants to merge 13 commits into from
Closed

[SQL][SPARK-17490] Optimize SerializeFromObject() for a primitive array #15044

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
170e7de
initial commit
kiszk Sep 10, 2016
327b7dd
fix test failures
kiszk Sep 11, 2016
e98cb1e
rebase
kiszk Oct 5, 2016
3d7ea2c
update benchmark program
kiszk Oct 6, 2016
5d5ccd6
create UnsafeArrayData from a primitive array in CatalystTypeConverters
kiszk Oct 8, 2016
edfbce3
create UnsafeArrayData from a primitive array in RowEncoder.serializeFor
kiszk Oct 8, 2016
8b79146
fix typo
kiszk Oct 14, 2016
b5473e3
revert changes for ValdateExternType
kiszk Nov 3, 2016
c5378f9
addressed review comments
kiszk Nov 4, 2016
d9e5b4f
addressed review comments
kiszk Nov 5, 2016
d507cfc
addressed comments
kiszk Nov 6, 2016
c217395
addressed review comments
kiszk Nov 7, 2016
4c679b5
addressed review comments
kiszk Nov 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
create UnsafeArrayData from a primitive array in CatalystTypeConverters
  • Loading branch information
@kiszk
kiszk committed Nov 2, 2016
commit 5d5ccd6785ddcac1ef4be82aef9e8dcdbcbfab65
14 changes: 14 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,20 @@ object CatalystTypeConverters {

override def toCatalystImpl(scalaValue: Any): ArrayData = {
scalaValue match {
case a: Array[Boolean] =>
UnsafeArrayData.fromPrimitiveArray(a)
case a: Array[Byte] =>
UnsafeArrayData.fromPrimitiveArray(a)
case a: Array[Short] =>
UnsafeArrayData.fromPrimitiveArray(a)
case a: Array[Int] =>
UnsafeArrayData.fromPrimitiveArray(a)
case a: Array[Long] =>
UnsafeArrayData.fromPrimitiveArray(a)
case a: Array[Float] =>
UnsafeArrayData.fromPrimitiveArray(a)
case a: Array[Double] =>
UnsafeArrayData.fromPrimitiveArray(a)
Copy link
Contributor

@cloud-fan cloud-fan Nov 4, 2016

Choose a reason for hiding this comment

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

do we need to do this? The CatalystTypeConverter will be removed and replaced by encoder eventually. Does your benchmark cover this branch?

Copy link
Member Author

@kiszk kiszk Nov 4, 2016

Choose a reason for hiding this comment

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

My benchmark does not cover this branch.
This test causes an failure without this branch. Should we drop this test and this branch?

Copy link
Contributor

@cloud-fan cloud-fan Nov 5, 2016

Choose a reason for hiding this comment

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

yea let's remove them, CatalystTypeConverter will not be in critical path and we don't need to optimize it.

case a: Array[_] =>
new GenericArrayData(a.map(elementConverter.toCatalyst))
case s: Seq[_] =>
Expand Down
37 changes: 37 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/CatalystTypeConvertersSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package org.apache.spark.sql.catalyst

import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.Row
import org.apache.spark.sql.catalyst.expressions.UnsafeArrayData
import org.apache.spark.sql.catalyst.util.GenericArrayData
import org.apache.spark.sql.types._

class CatalystTypeConvertersSuite extends SparkFunSuite {
Expand Down Expand Up @@ -61,4 +63,39 @@ class CatalystTypeConvertersSuite extends SparkFunSuite {
test("option handling in createToCatalystConverter") {
assert(CatalystTypeConverters.createToCatalystConverter(IntegerType)(Some(123)) === 123)
}

test("primitive array handing") {
Copy link
Contributor

@hvanhovell hvanhovell Oct 13, 2016

Choose a reason for hiding this comment

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

Nit: handing => handling

val intArray = Array(1, 100, 10000)
val intUnsafeArray = UnsafeArrayData.fromPrimitiveArray(intArray)
val intArrayType = ArrayType(IntegerType, false)
assert(CatalystTypeConverters.createToScalaConverter(intArrayType)(intUnsafeArray) === intArray)
assert(CatalystTypeConverters.createToCatalystConverter(intArrayType)(intArray)
== intUnsafeArray)

val doubleArray = Array(1.1, 111.1, 11111.1)
val doubleUnsafeArray = UnsafeArrayData.fromPrimitiveArray(doubleArray)
val doubleArrayType = ArrayType(DoubleType, false)
assert(CatalystTypeConverters.createToScalaConverter(doubleArrayType)(doubleUnsafeArray)
=== doubleArray)
assert(CatalystTypeConverters.createToCatalystConverter(doubleArrayType)(doubleArray)
== doubleUnsafeArray)
}

test("An array with null handing") {
Copy link
Contributor

@hvanhovell hvanhovell Oct 13, 2016

Choose a reason for hiding this comment

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

Nit: handing => handling

Copy link
Member Author

@kiszk kiszk Oct 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for a good catch. Addressed both of them.

val intArray = Array(1, null, 100, null, 10000)
val intGenericArray = new GenericArrayData(intArray)
val intArrayType = ArrayType(IntegerType, true)
assert(CatalystTypeConverters.createToScalaConverter(intArrayType)(intGenericArray)
=== intArray)
assert(CatalystTypeConverters.createToCatalystConverter(intArrayType)(intArray)
== intGenericArray)

val doubleArray = Array(1.1, null, 111.1, null, 11111.1)
val doubleGenericArray = new GenericArrayData(doubleArray)
val doubleArrayType = ArrayType(DoubleType, true)
assert(CatalystTypeConverters.createToScalaConverter(doubleArrayType)(doubleGenericArray)
=== doubleArray)
assert(CatalystTypeConverters.createToCatalystConverter(doubleArrayType)(doubleArray)
== doubleGenericArray)
}
}