You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/07/30 14:03:50 UTC
[GitHub] [iceberg] HeartSaVioR opened a new pull request #1276: Spark: Provide better error message on using SparkSessionCatalog without replacing default spark catalog
HeartSaVioR opened a new pull request #1276:
URL: https://github.com/apache/iceberg/pull/1276
If end users provide SparkSessionCatalog when defining non-default catalog (other than `spark_catalog`), Iceberg will throw NPE in runtime without proper message explaining what is misconfigured. This patch provides better error message on such case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] HeartSaVioR commented on a change in pull request #1276: Spark: Provide better error message on using SparkSessionCatalog without replacing default spark catalog
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on a change in pull request #1276:
URL: https://github.com/apache/iceberg/pull/1276#discussion_r463896412
##########
File path: spark3/src/main/java/org/apache/iceberg/spark/SparkSessionCatalog.java
##########
@@ -280,4 +282,10 @@ private boolean useIceberg(String provider) {
return false;
}
+
+ private T getSessionCatalog() {
+ Preconditions.checkNotNull(sessionCatalog, "Delegated SessionCatalog is not provided by Spark. " +
+ "Please make sure you are replacing Spark's default catalog.");
Review comment:
Ah right. I agree that's not something Spark is doing wrong and the sentence may leads misunderstanding. Thanks for the suggestion!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] merrily01 commented on pull request #1276: Spark: Provide better error message on using SparkSessionCatalog without replacing default spark catalog
Posted by GitBox <gi...@apache.org>.
merrily01 commented on pull request #1276:
URL: https://github.com/apache/iceberg/pull/1276#issuecomment-667947300
Good job~
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] HeartSaVioR commented on pull request #1276: Spark: Provide better error message on using SparkSessionCatalog without replacing default spark catalog
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on pull request #1276:
URL: https://github.com/apache/iceberg/pull/1276#issuecomment-667616085
Thanks for reviewing and merging!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1276: Spark: Provide better error message on using SparkSessionCatalog without replacing default spark catalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1276:
URL: https://github.com/apache/iceberg/pull/1276#issuecomment-667578669
Thanks, @HeartSaVioR! I merged this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1276: Spark: Provide better error message on using SparkSessionCatalog without replacing default spark catalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1276:
URL: https://github.com/apache/iceberg/pull/1276#issuecomment-667406328
Good catch! I suggested a couple of edits to the error message, but overall this is a good idea.
The test failure is checkstyle:
```
[ant:checkstyle] [ERROR] /home/travis/build/apache/iceberg/spark3/src/main/java/org/apache/iceberg/spark/SparkSessionCatalog.java:24: Extra separation in import group before 'org.apache.iceberg.relocated.com.google.common.base.Preconditions' [ImportOrder]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #1276: Spark: Provide better error message on using SparkSessionCatalog without replacing default spark catalog
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1276:
URL: https://github.com/apache/iceberg/pull/1276
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1276: Spark: Provide better error message on using SparkSessionCatalog without replacing default spark catalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1276:
URL: https://github.com/apache/iceberg/pull/1276#discussion_r463866197
##########
File path: spark3/src/main/java/org/apache/iceberg/spark/SparkSessionCatalog.java
##########
@@ -280,4 +282,10 @@ private boolean useIceberg(String provider) {
return false;
}
+
+ private T getSessionCatalog() {
+ Preconditions.checkNotNull(sessionCatalog, "Delegated SessionCatalog is not provided by Spark. " +
+ "Please make sure you are replacing Spark's default catalog.");
Review comment:
How about adding the name of the session catalog, like this: "Please make sure your are replacing Spark's default catalog, named 'spark_catalog'".
Also, the first line makes it sound like Spark did something wrong when this is probably a misconfiguration. For that, what about changing it to "Delegated SessionCatalog is missing" rather that "not provided by Spark"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org