Skip to content

[SPARK-25374][SQL] SafeProjection supports fallback to an interpreted mode #22468

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
maropu wants to merge 6 commits into from
Closed

[SPARK-25374][SQL] SafeProjection supports fallback to an interpreted mode #22468

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c549d3e
Implement InterpretedSafeProjection
maropu Sep 6, 2018
127fd27
Fix
maropu Oct 23, 2018
cec8480
Fix
maropu Oct 24, 2018
0b23adb
Fix
maropu Dec 4, 2018
7ef5f86
Fix
maropu Dec 4, 2018
fbfbbff
Fix
maropu Dec 4, 2018
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
Fix
  • Loading branch information
@maropu
maropu committed Dec 4, 2018
commit 7ef5f866eb02f6638a5be00a602de6c6810ae2a3
3 changes: 3 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ object UnsafeProjection
}
}

/**
* A projection that could turn UnsafeRow into GenericInternalRow
*/
object SafeProjection extends CodeGeneratorWithInterpretedFallback[Seq[Expression], Projection] {

override protected def createCodeGeneratedObject(in: Seq[Expression]): Projection = {
Expand Down
8 changes: 3 additions & 5 deletions ...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/UnsafeRowConverterSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,19 +609,17 @@ class UnsafeRowConverterSuite extends SparkFunSuite with Matchers with PlanTestB
MapType(StringType, IntegerType),
MapType(MapType(IntegerType, StringType), MapType(StringType, ShortType)))

val mapResultRow = convertBackToInternalRow(mapRow, fields4).toSeq(fields4)
val mapExpectedRow = mapRow.toSeq(fields4)
// Since `ArrayBasedMapData` does not override `equals` and `hashCode`,
Copy link
Contributor

@cloud-fan cloud-fan Dec 4, 2018

Choose a reason for hiding this comment

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

maybe we should implement equals and hashCode in ArrayBasedMapData and UnsafeMapData.

Copy link
Contributor

@cloud-fan cloud-fan Dec 4, 2018

Choose a reason for hiding this comment

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

Or we can use ExpressionEvalHelper.checkResult here.

Copy link
Member Author

@maropu maropu Dec 4, 2018

Choose a reason for hiding this comment

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

fixed code to use ExpressionEvalHelper.checkResult.

I don't remember correctly though, we might have some historical reasons about that; ArrayBasedMapData has no hashCode and equals. Probably, somebody might know this... cc: @hvanhovell @viirya

Copy link
Contributor

@hvanhovell hvanhovell Dec 4, 2018

Choose a reason for hiding this comment

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

ArrayBasedMapData/UnsafeMapData does not have equals() or hashCode() implemented because we do not have a good story around map equality. Implementing equals/hashcode for map is only half of the solution, we would also need a comparable binary format.

Copy link
Member Author

@maropu maropu Dec 5, 2018

Choose a reason for hiding this comment

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

Aha, thanks. I remember that its related to SPARK-18134.

// we need to take care of it to compare rows.
// we convert it into the two `Seq`s of keys and values for correct comparisons.
def toComparable(d: Any): Any = d match {
Copy link
Contributor

@cloud-fan cloud-fan Dec 4, 2018

Choose a reason for hiding this comment

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

this does nothing, isn't it?

Copy link
Member Author

@maropu maropu Dec 4, 2018

Choose a reason for hiding this comment

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

Since we cannot compare ArrayBasedMapDatas directly (that is, assert(mapResultRow === mapExpectedRow) fails), I just converted them into the Seqs of keys/values by this method.

case ar: GenericArrayData =>
ar.array.map(toComparable).toSeq
case map: ArrayBasedMapData =>
val keys = map.keyArray.array.map(toComparable).toSeq
val values = map.valueArray.array.map(toComparable).toSeq
(keys, values)
case o => o
}
val mapResultRow = convertBackToInternalRow(mapRow, fields4).toSeq(fields4)
val mapExpectedRow = mapRow.toSeq(fields4)
assert(mapResultRow.map(toComparable) === mapExpectedRow.map(toComparable))

// UDT tests
Expand Down