-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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.
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
[SPARK-20728][SQL][FOLLOWUP] Use an actionable exception message #19903
Conversation
LGTM |
@@ -602,7 +602,7 @@ object DataSource extends Logging { | |||
provider1.startsWith("org.apache.spark.sql.hive.orc")) { | |||
throw new AnalysisException( | |||
"Hive-based ORC data source must be used with Hive support enabled. " + | |||
"Please use native ORC data source instead") | |||
"Please use native ORC data source instead by `SET spark.sql.orc.impl=native`") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait .. @dongjoon-hyun can we just say like by setting 'spark.sql.orc.impl' configuration to 'native'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
LGTM |
Thank you, @HyukjinKwon and @cloud-fan . |
@@ -602,7 +602,8 @@ object DataSource extends Logging { | |||
provider1.startsWith("org.apache.spark.sql.hive.orc")) { | |||
throw new AnalysisException( | |||
"Hive-based ORC data source must be used with Hive support enabled. " + | |||
"Please use native ORC data source instead") | |||
"Please use native ORC data source instead by setting 'spark.sql.orc.impl' " + | |||
"configuration to 'native'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the native ORC data source by setting 'spark.sql.orc.impl' to 'native'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
@@ -602,7 +602,8 @@ object DataSource extends Logging { | |||
provider1.startsWith("org.apache.spark.sql.hive.orc")) { | |||
throw new AnalysisException( | |||
"Hive-based ORC data source must be used with Hive support enabled. " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hive built-in ORC data source must be used with Hive support enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
Do you have a test case? |
Yep. The test case will be updated together. |
@cloud-fan , @HyukjinKwon , @gatorsmile . The PR is updated again. |
Test build #84521 has finished for PR 19903 at commit
|
LGTM pending Jenkins on $84528 |
Thanks, @gatorsmile . |
Test build #84526 has finished for PR 19903 at commit
|
Test build #84528 has finished for PR 19903 at commit
|
Retest this please. |
Test build #84532 has finished for PR 19903 at commit
|
retest this please |
@@ -601,8 +601,9 @@ object DataSource extends Logging { | |||
if (provider1.toLowerCase(Locale.ROOT) == "orc" || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested in other PR, we can remove this condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I'll remove this here since it's small.
LGTM with one minor comment. |
Test build #84536 has finished for PR 19903 at commit
|
Test build #84541 has finished for PR 19903 at commit
|
Merged to master. |
Thank you, @HyukjinKwon , @cloud-fan , @gatorsmile , @viirya ! |
What changes were proposed in this pull request?
This is a follow-up of #19871 to improve an exception message.
How was this patch tested?
Pass the Jenkins.