Skip to content

[SPARK-37444][SQL] ALTER NAMESPACE ... SET LOCATION should handle empty location consistently across v1 and v2 command #34686

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.

Sign up for GitHub

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

Closed
imback82 wants to merge 3 commits into from
Closed

[SPARK-37444][SQL] ALTER NAMESPACE ... SET LOCATION should handle empty location consistently across v1 and v2 command #34686

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28ce116
initial commit
imback82 Nov 23, 2021
8c0a8c9
address pr comment
imback82 Nov 24, 2021
068473f
address PR comment
imback82 Nov 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import org.apache.spark.internal.Logging
import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.{FunctionIdentifier, SQLConfHelper, TableIdentifier}
import org.apache.spark.sql.catalyst.analysis._
import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogStorageFormat}
import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogStorageFormat, CatalogUtils}
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.aggregate.{First, Last}
import org.apache.spark.sql.catalyst.parser.SqlBaseParser._
Expand Down Expand Up @@ -3132,7 +3132,7 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
withOrigin(ctx) {
SetNamespaceLocation(
UnresolvedNamespace(visitMultipartIdentifier(ctx.multipartIdentifier)),
visitLocationSpec(ctx.locationSpec))
CatalogUtils.stringToURI(visitLocationSpec(ctx.locationSpec)))
}
}

Expand Down
4 changes: 3 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.spark.sql.catalyst.plans.logical

import java.net.URI

import org.apache.spark.sql.catalyst.analysis.{AnalysisContext, FieldName, NamedRelation, PartitionSpec, UnresolvedException}
import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec
import org.apache.spark.sql.catalyst.catalog.FunctionResource
Expand Down Expand Up @@ -349,7 +351,7 @@ case class SetNamespaceProperties(
*/
case class SetNamespaceLocation(
namespace: LogicalPlan,
location: String) extends UnaryCommand {
location: URI) extends UnaryCommand {

@cloud-fan cloud-fan Nov 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it's a bit weird to have URI in the logical plan, can we keep it as String type and just put the qualified path string?

@cloud-fan cloud-fan Nov 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and can you refer to the code for qualifying the path string in v1 commands? Let's make sure v1 and v2 are truly consistent.

@imback82 imback82 Nov 24, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. This is where qualified path is generated for v1:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

Line 285 in c1029dc

name = dbName, locationUri = makeQualifiedDBPath(dbDefinition.locationUri)))

override def child: LogicalPlan = namespace
override protected def withNewChildInternal(newChild: LogicalPlan): SetNamespaceLocation =
copy(namespace = newChild)
Expand Down
9 changes: 5 additions & 4 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import java.util.Locale

import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.analysis._
import org.apache.spark.sql.catalyst.catalog.BucketSpec
import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogUtils}
import org.apache.spark.sql.catalyst.expressions.{EqualTo, Hex, Literal}
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.connector.catalog.TableChange.ColumnPosition.{after, first}
Expand Down Expand Up @@ -1833,20 +1833,21 @@ class DDLParserSuite extends AnalysisTest {
}

test("set namespace location") {
val loc = CatalogUtils.stringToURI("/home/user/db")
comparePlans(
parsePlan("ALTER DATABASE a.b.c SET LOCATION '/home/user/db'"),
SetNamespaceLocation(
UnresolvedNamespace(Seq("a", "b", "c")), "/home/user/db"))
UnresolvedNamespace(Seq("a", "b", "c")), loc))

comparePlans(
parsePlan("ALTER SCHEMA a.b.c SET LOCATION '/home/user/db'"),
SetNamespaceLocation(
UnresolvedNamespace(Seq("a", "b", "c")), "/home/user/db"))
UnresolvedNamespace(Seq("a", "b", "c")), loc))

comparePlans(
parsePlan("ALTER NAMESPACE a.b.c SET LOCATION '/home/user/db'"),
SetNamespaceLocation(
UnresolvedNamespace(Seq("a", "b", "c")), "/home/user/db"))
UnresolvedNamespace(Seq("a", "b", "c")), loc))
}

test("analyze table statistics") {
Expand Down
5 changes: 3 additions & 2 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.spark.sql.execution.command

import java.net.URI
import java.util.Locale
import java.util.concurrent.TimeUnit._

Expand Down Expand Up @@ -147,13 +148,13 @@ case class AlterDatabasePropertiesCommand(
* ALTER (DATABASE|SCHEMA) database_name SET LOCATION path
* }}}
*/
case class AlterDatabaseSetLocationCommand(databaseName: String, location: String)
case class AlterDatabaseSetLocationCommand(databaseName: String, location: URI)
extends LeafRunnableCommand {

override def run(sparkSession: SparkSession): Seq[Row] = {
val catalog = sparkSession.sessionState.catalog
val oldDb = catalog.getDatabaseMetadata(databaseName)
catalog.alterDatabase(oldDb.copy(locationUri = CatalogUtils.stringToURI(location)))
catalog.alterDatabase(oldDb.copy(locationUri = location))

Seq.empty[Row]
}
Expand Down
3 changes: 2 additions & 1 deletion ...e/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import scala.collection.mutable

import org.apache.spark.sql.{SparkSession, Strategy}
import org.apache.spark.sql.catalyst.analysis.{ResolvedDBObjectName, ResolvedNamespace, ResolvedPartitionSpec, ResolvedTable}
import org.apache.spark.sql.catalyst.catalog.CatalogUtils
import org.apache.spark.sql.catalyst.expressions
import org.apache.spark.sql.catalyst.expressions.{And, Attribute, DynamicPruning, EmptyRow, Expression, Literal, NamedExpression, PredicateHelper, SubqueryExpression}
import org.apache.spark.sql.catalyst.planning.PhysicalOperation
Expand Down Expand Up @@ -314,7 +315,7 @@ class DataSourceV2Strategy(session: SparkSession) extends Strategy with Predicat
AlterNamespaceSetPropertiesExec(
catalog.asNamespaceCatalog,
ns,
Map(SupportsNamespaces.PROP_LOCATION -> location)) :: Nil
Map(SupportsNamespaces.PROP_LOCATION -> CatalogUtils.URIToString(location))) :: Nil

case CommentOnNamespace(ResolvedNamespace(catalog, ns), comment) =>
AlterNamespaceSetPropertiesExec(
Expand Down
12 changes: 12 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1301,6 +1301,18 @@ class DataSourceV2SQLSuite
}
}

test("SPARK-37444: ALTER NAMESPACE .. SET LOCATION using v2 catalog with empty location") {
val ns = "testcat.ns1.ns2"
withNamespace(ns) {
sql(s"CREATE NAMESPACE IF NOT EXISTS $ns COMMENT " +
"'test namespace' LOCATION '/tmp/ns_test_1'")

@cloud-fan cloud-fan Nov 24, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shall we do the same thing for CREATE NAMESPACE? We can do it in a separated PR.

@imback82 imback82 Nov 24, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, we should fix this. I will do it in a separate PR. Thanks!

val e = intercept[IllegalArgumentException] {
sql(s"ALTER DATABASE $ns SET LOCATION ''")
}
assert(e.getMessage.contains("Can not create a Path from an empty string"))
}
}

private def testShowNamespaces(
sqlText: String,
expected: Seq[String]): Unit = {
Expand Down