Skip to content

Conversation

@maropu
Copy link
Member

@maropu maropu commented Mar 6, 2018

What changes were proposed in this pull request?

This pr added interpreted execution for GetExternalRowField.

How was this patch tested?

Added tests in ObjectExpressionsSuite.

@SparkQA
Copy link

SparkQA commented Mar 6, 2018

Test build #87995 has finished for PR 20746 at commit 62a9814.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Member Author

maropu commented Mar 6, 2018

retest this please


// If an input row or a field are null, a runtime exception will be thrown
val errMsg1 = intercept[RuntimeException] {
evaluate(getRowField, InternalRow.fromSeq(Seq(null)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor comment: This only tests the interpreted code path. Maybe we should add something to the ExpressionEvalHelper harness to test all paths in case of failure. Let me know if you want to pick this up, we can also spin that off into a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, sound nice to me. Would it be better to add the helper function first before this pr?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just merge this, and add the helper in a follow-up.

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.

ok, thanks! I'll make a pr later for that.

@hvanhovell
Copy link
Contributor

@maropu look pretty good. I left one comment, otherwise this is gtg.

@SparkQA
Copy link

SparkQA commented Mar 6, 2018

Test build #87997 has finished for PR 20746 at commit 62a9814.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@hvanhovell
Copy link
Contributor

Merging to master. Thanks!

@asfgit asfgit closed this in e8a259d Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants