Skip to content

Conversation

@rxin
Copy link
Contributor

@rxin rxin commented Jul 31, 2018

What changes were proposed in this pull request?

Previously TVF resolution could throw IllegalArgumentException if the data type is null type. This patch replaces that exception with AnalysisException, enriched with positional information, to improve error message reporting and to be more consistent with rest of Spark SQL.

How was this patch tested?

Updated the test case in table-valued-functions.sql.out, which is how I identified this problem in the first place.

@holdensmagicalunicorn
Copy link

@rxin, thanks! I am a bot who has found some folks who might be able to help with the review:@hvanhovell, @cloud-fan and @gatorsmile

@rxin
Copy link
Contributor Author

rxin commented Jul 31, 2018

cc @gatorsmile @ericl who originally wrote this.

@ericl
Copy link
Contributor

ericl commented Jul 31, 2018

LGTM

@SparkQA
Copy link

SparkQA commented Jul 31, 2018

Test build #93834 has finished for PR 21934 at commit 514fd77.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor Author

rxin commented Jul 31, 2018

Jenkins, retest this please.

java.lang.IllegalArgumentException
Invalid arguments for resolved function: 1, null
org.apache.spark.sql.AnalysisException
error: table-valued function range with alternatives:
Copy link
Contributor

@dilipbiswal dilipbiswal Jul 31, 2018

Choose a reason for hiding this comment

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

Nit: @rxin will it be better to display the function name within single quotes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably could go either way

Copy link
Contributor

Choose a reason for hiding this comment

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

@rxin Ok.

@gatorsmile
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Aug 1, 2018

Test build #93849 has finished for PR 21934 at commit 514fd77.

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

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks! Merged to master.

@asfgit asfgit closed this in 1f7e22c Aug 1, 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.

6 participants