Skip to content

[SPARK-29033][SQL][WIP] Always use UnsafeRow-based version of CreateNamedStruct #25745

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
JoshRosen wants to merge 3 commits into from
Closed

[SPARK-29033][SQL][WIP] Always use UnsafeRow-based version of CreateNamedStruct #25745

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b07b57
Change CreateNamedStruct to use CreateNamedStructUnsafe implementatio…
JoshRosen Sep 10, 2019
426d856
Remove CreateNamedStructLike
JoshRosen Sep 10, 2019
d3925bc
Update ExpressionEvalHelper.checkResult to properly compare different…
JoshRosen Sep 10, 2019
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
Update ExpressionEvalHelper.checkResult to properly compare different… 
… InternalRow implementations
  • Loading branch information
@JoshRosen
JoshRosen committed Sep 10, 2019
commit d3925bc02604fdb7223091c066018b2142c38753
4 changes: 4 additions & 0 deletions ...alyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks with PlanTestBa
case (result: Float, expected: Float) =>
if (expected.isNaN) result.isNaN else expected == result
case (result: Row, expected: InternalRow) => result.toSeq == expected.toSeq(result.schema)
case (result: Seq[InternalRow], expected: Seq[InternalRow]) =>
Copy link
Contributor Author

@JoshRosen JoshRosen Sep 10, 2019

Choose a reason for hiding this comment

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

This change was needed because we can't do direct equals() comparison between UnsafeRow and other row classes. After this PR's changes, the "SPARK-14793: split wide struct creation into blocks due to JVM code size limit" test case in CodeGenerationSuite was failing because the new code was producing UnsafeRow but the test code was comparing against GenericInternalRow. In the old code, this comparison between sequences of rows was happening in the default case _ => below, but that case doesn't work when the InternalRow implementations are mismatched.

I'm not sure whether this change-of-internal-row-format will have adverse consequences in other parts of the code.

result.size == expected.size && result.zip(expected).forall { case (r, e) =>
checkResult(r, e, exprDataType, exprNullable)
}
case _ =>
result == expected
}
Expand Down