Skip to content
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

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

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

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
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,28 @@ object ScalaReflection extends ScalaReflection {
val newPath = s"""- array element class: "$clsName"""" +: walkedTypePath
MapObjects(serializerFor(_, elementType, newPath), input, dt)

case dt @ (BooleanType | ByteType | ShortType | IntegerType | LongType |
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this cover all the code paths from an object to a Spark SQL internal type? For instance RowEncoder.serializeFor? Also take a look at CatalystTypeConverters.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am afraid that I cannot understand your question correctly.
This case covers only cases that we are interested in (e.g. generate UnsafeArrayData). Other cases are covered by here.

Is this an answer to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your clarification. Let me check them.

FloatType | DoubleType) =>
val cls = input.dataType.asInstanceOf[ObjectType].cls
if (cls.isAssignableFrom(classOf[Array[Boolean]]) ||
cls.isAssignableFrom(classOf[Array[Byte]]) ||
cls.isAssignableFrom(classOf[Array[Short]]) ||
cls.isAssignableFrom(classOf[Array[Int]]) ||
cls.isAssignableFrom(classOf[Array[Long]]) ||
cls.isAssignableFrom(classOf[Array[Float]]) ||
cls.isAssignableFrom(classOf[Array[Double]])) {
StaticInvoke(
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.

we can simplify it

if (cls.isArray && cls.getComponentType.isPrimitive) ...

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, good catch.

classOf[UnsafeArrayData],
ArrayType(dt, false),
"fromPrimitiveArray",
input :: Nil)
} else {
NewInstance(
classOf[GenericArrayData],
input :: Nil,
dataType = ArrayType(dt, schemaFor(elementType).nullable))
}

case dt =>
NewInstance(
classOf[GenericArrayData],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,18 +119,36 @@ object RowEncoder {
"fromString",
inputObject :: Nil)

case t @ ArrayType(et, _) => et match {
case BooleanType | ByteType | ShortType | IntegerType | LongType | FloatType | DoubleType =>
// TODO: validate input type for primitive array.
NewInstance(
classOf[GenericArrayData],
inputObject :: Nil,
dataType = t)
case _ => MapObjects(
element => serializerFor(ValidateExternalType(element, et), et),
inputObject,
ObjectType(classOf[Object]))
}
case t @ ArrayType(et, cn) =>
val cls = inputObject.dataType.asInstanceOf[ObjectType].cls
Copy link
Contributor

Choose a reason for hiding this comment

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

where do we use the cls?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, removed

et match {
/*
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.

can we do the same thing here? i.e. special handling primitive array. I know we don't have the class information here, bu we can do it in the runtime:

object ArrayData {
  def toArrayData(input: Any): ArrayData = input match {
    case a: Array[Boolean] => UnsafeArrayData.fromPrimitive(a)
    ...
    case other => new GenericArrayData(other)
  }
}

Then we just use StaticInvoke here to call this method

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.

Good idea. It worked well.

case BooleanType | ByteType | ShortType | IntegerType | LongType | FloatType | DoubleType
Copy link
Contributor

Choose a reason for hiding this comment

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

What is going on here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, my mistake

if !cn && (
cls.isAssignableFrom(classOf[Array[Boolean]]) ||
cls.isAssignableFrom(classOf[Array[Byte]]) ||
cls.isAssignableFrom(classOf[Array[Short]]) ||
cls.isAssignableFrom(classOf[Array[Int]]) ||
cls.isAssignableFrom(classOf[Array[Long]]) ||
cls.isAssignableFrom(classOf[Array[Float]]) ||
cls.isAssignableFrom(classOf[Array[Double]])) =>
print(s"1@ET: $et, $cn, $cls\n")
StaticInvoke(
classOf[UnsafeArrayData],
ArrayType(et, false),
"fromPrimitiveArray",
inputObject :: Nil)
*/
case BooleanType | ByteType | ShortType | IntegerType | LongType | FloatType | DoubleType =>
NewInstance(
classOf[GenericArrayData],
inputObject :: Nil,
dataType = t)
case _ => MapObjects(
element => serializerFor(ValidateExternalType(element, et), et),
inputObject,
ObjectType(classOf[Object]))
}

case t @ MapType(kt, vt, valueNullable) =>
val keys =
Expand Down Expand Up @@ -193,7 +211,8 @@ object RowEncoder {
// as java.lang.Object.
case _: DecimalType => ObjectType(classOf[java.lang.Object])
// In order to support both Array and Seq in external row, we make this as java.lang.Object.
case _: ArrayType => ObjectType(classOf[java.lang.Object])
case a @ ArrayType(et, cn) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted it, but it was not the same as the previous one. thanks.

ObjectType(classOf[java.lang.Object])
case _ => externalDataTypeFor(dt)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -993,7 +993,7 @@ case class ValidateExternalType(child: Expression, expected: DataType)
case _: DecimalType =>
Seq(classOf[java.math.BigDecimal], classOf[scala.math.BigDecimal], classOf[Decimal])
.map(cls => s"$obj instanceof ${cls.getName}").mkString(" || ")
case _: ArrayType =>
case a @ ArrayType(et, cn) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

s"$obj instanceof ${classOf[Seq[_]].getName} || $obj.getClass().isArray()"
case _ =>
s"$obj instanceof ${ctx.boxedType(dataType)}"
Expand Down
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 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 handling") {
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)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,32 @@ class RowEncoderSuite extends SparkFunSuite {
assert(encoder.serializer.head.nullable == false)
}

test("RowEncoder should support a primitive array") {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: RowEncoder should support primitive arrays

Copy link
Member Author

Choose a reason for hiding this comment

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

done

val schema = new StructType()
.add("booleanPrimitiveArray", ArrayType(BooleanType, false))
.add("bytePrimitiveArray", ArrayType(ByteType, false))
.add("shortPrimitiveArray", ArrayType(ShortType, false))
.add("intPrimitiveArray", ArrayType(IntegerType, false))
.add("longPrimitiveArray", ArrayType(LongType, false))
.add("floatPrimitiveArray", ArrayType(FloatType, false))
.add("doublePrimitiveArray", ArrayType(DoubleType, false))
val encoder = RowEncoder(schema).resolveAndBind()
val input = Seq(
Array(true, false),
Array(1.toByte, 64.toByte, Byte.MaxValue),
Array(1.toShort, 255.toShort, Short.MaxValue),
Array(1, 10000, Int.MaxValue),
Array(1.toLong, 1000000.toLong, Long.MaxValue),
Array(1.1.toFloat, 123.456.toFloat, Float.MaxValue),
Array(11.1111, 123456.7890123, Double.MaxValue)
)
val row = encoder.toRow(Row.fromSeq(input))
val convertedBack = encoder.fromRow(row)
input.zipWithIndex.map { case (array, index) =>
assert(convertedBack.getSeq(index) === array)
}
}

test("RowEncoder should support array as the external type for ArrayType") {
val schema = new StructType()
.add("array", ArrayType(IntegerType))
Expand Down
18 changes: 18 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,24 @@ class DatasetSuite extends QueryTest with SharedSQLContext {
checkAnswer(agg, ds.groupBy('id % 2).agg(count('id)))
}
}

test("array") {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you think of a better test name which can describe what we are testing here?

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, gave a better name

val arrayByte = Array(1.toByte, 2.toByte, 3.toByte)
val arrayInt = Array(1, 2, 3)
val arrayLong = Array(1.toLong, 2.toLong, 3.toLong)
val arrayDouble = Array(1.1, 2.2, 3.3)
val arrayString = Array("a", "b", "c")
val dsByte = sparkContext.parallelize(Seq(arrayByte), 1).toDS.map(e => e)
val dsInt = sparkContext.parallelize(Seq(arrayInt), 1).toDS.map(e => e)
val dsLong = sparkContext.parallelize(Seq(arrayLong), 1).toDS.map(e => e)
val dsDouble = sparkContext.parallelize(Seq(arrayDouble), 1).toDS.map(e => e)
val dsString = sparkContext.parallelize(Seq(arrayString), 1).toDS.map(e => e)
checkDataset(dsByte, arrayByte)
checkDataset(dsInt, arrayInt)
checkDataset(dsLong, arrayLong)
checkDataset(dsDouble, arrayDouble)
checkDataset(dsString, arrayString)
}
}

case class Generic[T](id: T, value: Double)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* 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 scala.concurrent.duration._

import org.apache.spark.SparkConf
import org.apache.spark.sql.catalyst.util._
import org.apache.spark.util.Benchmark

/**
* Benchmark [[PrimitiveArray]] for DataFrame and Dataset program using primitive array
* To run this:
* 1. replace ignore(...) with test(...)
* 2. build/sbt "sql/test-only *benchmark.PrimitiveArrayDataBenchmark"
*
* Benchmarks in this file are skipped in normal builds.
*/
class PrimitiveArrayBenchmark extends BenchmarkBase {

def writeDatasetArray(iters: Int): Unit = {
import sparkSession.implicits._

val count = 1024 * 1024 * 2

val sc = sparkSession.sparkContext
val primitiveIntArray = Array.fill[Int](count)(65535)
val dsInt = sc.parallelize(Seq(primitiveIntArray), 1).toDS
dsInt.count
Copy link
Contributor

Choose a reason for hiding this comment

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

what this count do? warmup?

Copy link
Member Author

Choose a reason for hiding this comment

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

Want to force to build dataset

val intArray = { i: Int =>
var n = 0
while (n < iters) {
dsInt.map(e => e).collect
Copy link
Contributor

Choose a reason for hiding this comment

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

use n += dsInt.map(e => e).queryExecution.toRDD.collect().length, for 2 reasons:

  1. This PR speeds up the serialization by converts external primitve array to internal unsafe array data directly. However, calling collect directly will converts internal row to external row, which includes deserialization.
  2. we should use the result of the benchmark code, or it may get optimized and removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. done

n += 1
}
}
val primitiveDoubleArray = Array.fill[Double](count)(65535.0)
val dsDouble = sc.parallelize(Seq(primitiveDoubleArray), 1).toDS
dsDouble.count
val doubleArray = { i: Int =>
var n = 0
var sum = 0L
while (n < iters) {
dsDouble.map(e => e).collect
n += 1
}
}

val benchmark = new Benchmark("Write an array in Dataset", count * iters)
benchmark.addCase("Int ")(intArray)
benchmark.addCase("Double")(doubleArray)
benchmark.run
/*
OpenJDK 64-Bit Server VM 1.8.0_91-b14 on Linux 4.4.11-200.fc22.x86_64
Intel Xeon E3-12xx v2 (Ivy Bridge)
Write an array in Dataset: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
Int 280 / 302 30.0 33.4 1.0X
Double 503 / 519 16.7 60.0 0.6X
*/
}

ignore("Write an array in Dataset") {
writeDatasetArray(4)
}
}