rdblue commented on a change in pull request #30554:
URL: https://github.com/apache/spark/pull/30554#discussion_r533591131
##########
File path:
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
##########
@@ -657,14 +659,8 @@ class ResolveSessionCatalog(
comment: Option[String],
storageFormat: CatalogStorageFormat,
external: Boolean): CatalogTable = {
- if (external) {
- if (DDLUtils.isHiveTable(Some(provider))) {
- if (location.isEmpty) {
- throw new AnalysisException(s"CREATE EXTERNAL TABLE must be
accompanied by LOCATION")
- }
- } else {
- throw new AnalysisException(s"Operation not allowed: CREATE EXTERNAL
TABLE ... USING")
- }
+ if (external && location.isEmpty) {
+ throw new AnalysisException(s"CREATE EXTERNAL TABLE must be accompanied
by LOCATION")
Review comment:
The PR description says "This PR allows CREATE EXTERNAL TABLE when
LOCATION is present" but the effect of this change is to disallow `EXTERNAL`
when `LOCATION` is not present.
Since the session catalog may be wrapped by a v2 catalog, this disallows
`EXTERNAL` for some v2 catalog plugin use cases. That violates the v2 behavior
expectations because `EXTERNAL` should be passed to a v2 catalog unless it
would have failed before. I don't think this change is correct.
Also, why does this change need to be included to change the default
provider? If it doesn't need to be included, then it should be in a separate PR.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
