Skip to content

[SPARK-12439][SQL] Fix toCatalystArray and MapObjects #10391

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
viirya wants to merge 10 commits into from
Closed

[SPARK-12439][SQL] Fix toCatalystArray and MapObjects #10391

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
788a7c6
Fix toCatalystArray and MapObjects.
viirya Dec 19, 2015
180048c
Move test case.
viirya Dec 22, 2015
a726fd8
Remove unnecessary classes.
viirya Dec 22, 2015
090b293
Fix MapObjects.
viirya Dec 23, 2015
7389e15
Remove unnecessary import.
viirya Dec 23, 2015
ae55e01
Add check if genFunction returns an Option.
viirya Dec 23, 2015
f5b9d50
Check WrapOption.
viirya Dec 23, 2015
7728124
Do not check WrapOption. Instead, we should wrap an If expression out…
viirya Dec 29, 2015
e0d34a5
Merge remote-tracking branch 'upstream/master' into fix-catalystarray
viirya Dec 31, 2015
e541e04
Rebase updates.
viirya Dec 31, 2015
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
13 changes: 8 additions & 5 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,11 +406,14 @@ object ScalaReflection extends ScalaReflection {
def toCatalystArray(input: Expression, elementType: `Type`): Expression = {
val externalDataType = dataTypeFor(elementType)
val Schema(catalystType, nullable) = silentSchemaFor(elementType)
if (isNativeType(catalystType)) {
NewInstance(
classOf[GenericArrayData],
input :: Nil,
dataType = ArrayType(catalystType, nullable))
if (isNativeType(externalDataType)) {
expressions.If(
IsNull(input),
expressions.Literal.create(null, ArrayType(catalystType, nullable)),
NewInstance(
classOf[GenericArrayData],
input :: Nil,
dataType = ArrayType(catalystType, nullable)))
Copy link
Member Author

@viirya viirya Dec 23, 2015

Choose a reason for hiding this comment

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

The part of fixing is from pr #10401. Without this, the following fixing to MapObjects will cause null exception. Can we merge #10401 first then I do rebase here?

Copy link
Member Author

@viirya viirya Dec 29, 2015

Choose a reason for hiding this comment

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

Waiting #10443 to getting merged too.

} else {
val clsName = getClassNameFromType(elementType)
val newPath = s"""- array element class: "$clsName"""" +: walkedTypePath
Expand Down
4 changes: 2 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,10 +456,10 @@ case class MapObjects(
($elementJavaType)${genInputData.value}${itemAccessor(loopIndex)};
$loopNullCheck

if (${loopVar.isNull}) {
${genFunction.code}
if (${genFunction.isNull}) {
Copy link
Member Author

@viirya viirya Dec 23, 2015

Choose a reason for hiding this comment

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

Even loopVar.isNull is true, we should still call genFunction because it may have special handling for null case, for example WrapOption will produce a None for null value. We should depend on genFunction.isNull to decide $convertedArray[$loopIndex] is null or not.

$convertedArray[$loopIndex] = null;
} else {
${genFunction.code}
$convertedArray[$loopIndex] = ${genFunction.value};
}

Expand Down
4 changes: 4 additions & 0 deletions ...talyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import com.google.common.collect.MapMaker

import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.Encoders
import org.apache.spark.sql.catalyst.ScalaReflection
import org.apache.spark.sql.catalyst.expressions.AttributeReference
import org.apache.spark.sql.catalyst.util.ArrayData
import org.apache.spark.sql.catalyst.{OptionalData, PrimitiveData}
Expand Down Expand Up @@ -155,6 +156,9 @@ class ExpressionEncoderSuite extends SparkFunSuite {

productTest(OptionalData(None, None, None, None, None, None, None, None))

encodeDecodeTest(Seq(Some(1), None), "Option in array")
encodeDecodeTest(Map(1 -> Some(10L), 2 -> Some(20L), 3 -> None), "Option in map")

productTest(BoxedData(1, 1L, 1.0, 1.0f, 1.toShort, 1.toByte, true))

productTest(BoxedData(null, null, null, null, null, null, null))
Expand Down