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 7 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
15 changes: 13 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 @@ -439,6 +439,13 @@ case class MapObjects(
s"boolean ${loopVar.isNull} = ${genInputData.isNull} || ${loopVar.value} == null;"
}

// If lambdaFunction is WrapOption, we will not determine null or not based on the
// value of loopVar.isNull, because WrapOption will return None for null.
val isWrapOption = lambdaFunction match {
Copy link
Contributor

@marmbrus marmbrus Dec 28, 2015

Choose a reason for hiding this comment

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

I'm not sure exactly what is going on here, but this looks like a hack. Ideally this node should not need to know what type its child is to operate correctly.

Copy link
Member Author

@viirya viirya Dec 28, 2015

Choose a reason for hiding this comment

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

Let me explain it.

When we pass in an array with None. It will be encoded as null internally. When we decode it back, WrapOption is called to re-construct it.

The logic of MapObjects is to assign an element as null if its given input element is null. So It will not actually go into WrapOption to re-construct a None back. In order to do that, we need to call lambdaFunction even the element is null.

But we can't simply ignore loopVar.isNull and call all kinds of lambdaFunctions. I tried before but for some lambdaFunctions, a null input value causes problematic results.

In the end I can only check if lambdaFunction is WrapOption or not to make the decision here. Do you have other suggestion other than a hack like this here? Thanks.

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.

@marmbrus I have refactored this part and removed the check of WrapOption now. Please take a look it if it is better now. Thanks.

case _: WrapOption => "true"
case _ => "false"
}

s"""
${genInputData.code}

Expand All @@ -456,11 +463,15 @@ case class MapObjects(
($elementJavaType)${genInputData.value}${itemAccessor(loopIndex)};
$loopNullCheck

if (${loopVar.isNull}) {
if (${loopVar.isNull} && !${isWrapOption}) {
$convertedArray[$loopIndex] = null;
} else {
${genFunction.code}
$convertedArray[$loopIndex] = ${genFunction.value};
if (${genFunction.isNull}) {
$convertedArray[$loopIndex] = null;
} else {
$convertedArray[$loopIndex] = ${genFunction.value};
}
}

$loopIndex += 1;
Expand Down
3 changes: 3 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 @@ -155,6 +155,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