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

imback82 · 2021-11-23T02:21:00Z

What changes were proposed in this pull request?

Currently, there is an inconsistency when handling an empty location for ALTER NAMESPACE .. SET LOCATION between v1 and v2 command. In v1 command, an empty string location will result in the IllegalArgumentException exception thrown whereas v2 uses the empty string as it is.

This PR proposes to make the behavior consistent by following the v1 command behavior.

Why are the changes needed?

To make the behavior consistent and the reason for following v1 behavior is that "Spark should be responsible to qualify the user-specified path using its spark/hadoop configs, before passing the path to v2 sources": #34610 (comment)

Does this PR introduce any user-facing change?

Yes, now the empty string location will result in the IllegalArgumentException exception thrown even for v2 catalogs.

How was this patch tested?

Added a new test

SparkQA · 2021-11-23T03:39:58Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49997/

SparkQA · 2021-11-23T04:23:48Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49997/

imback82 · 2021-11-23T04:41:04Z

cc: @cloud-fan (GA test failure is from org.apache.spark.sql.jdbc.DB2KrbIntegrationSuite)

SparkQA · 2021-11-23T07:48:28Z

Test build #145525 has finished for PR 34686 at commit 28ce116.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AlterDatabaseSetLocationCommand(databaseName: String, location: URI)

cloud-fan · 2021-11-23T10:28:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

@@ -349,7 +351,7 @@ case class SetNamespaceProperties(
 */
 case class SetNamespaceLocation(
    namespace: LogicalPlan,
-    location: String) extends UnaryCommand {
+    location: URI) extends UnaryCommand {


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?

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.

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)))

imback82 · 2021-11-24T05:51:42Z

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

+    val fs = hadoopPath.getFileSystem(hadoopConf)
+    fs.makeQualified(hadoopPath).toUri
+  }
+


These are copied from SessionCatalog.scala

imback82 · 2021-11-24T05:52:14Z

...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala

@@ -311,10 +313,12 @@ class DataSourceV2Strategy(session: SparkSession) extends Strategy with Predicat
      AlterNamespaceSetPropertiesExec(catalog.asNamespaceCatalog, ns, properties) :: Nil

    case SetNamespaceLocation(ResolvedNamespace(catalog, ns), location) =>
+      val nsPath = CatalogUtils.makeQualifiedNamespacePath(
+        CatalogUtils.stringToURI(location), conf.warehousePath, session.sharedState.hadoopConf)


This will now qualify the namespace location for v2 command.

session.conf will contain the qualified warehouse path, but using SQLConf is fine since it will be qualified anyway (same as v1):

spark/sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala

Line 89 in c1029dc

SharedState.setWarehousePathConf(confClone, hadoopConfClone, qualified)

SparkQA · 2021-11-24T06:52:21Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50037/

SparkQA · 2021-11-24T07:36:05Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50037/

cloud-fan · 2021-11-24T08:13:56Z

...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala

@@ -44,7 +45,8 @@ import org.apache.spark.sql.util.CaseInsensitiveStringMap
 import org.apache.spark.storage.StorageLevel
 import org.apache.spark.unsafe.types.UTF8String

-class DataSourceV2Strategy(session: SparkSession) extends Strategy with PredicateHelper {
+class DataSourceV2Strategy(session: SparkSession) extends Strategy
+    with PredicateHelper with SQLConfHelper {


We don't need SQLConfHelper. We can get the conf by session.sessionState.conf

cloud-fan · 2021-11-24T08:15:21Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

+    val ns = "testcat.ns1.ns2"
+    withNamespace(ns) {
+      sql(s"CREATE NAMESPACE IF NOT EXISTS $ns COMMENT " +
+        "'test namespace' LOCATION '/tmp/ns_test_1'")


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

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

SparkQA · 2021-11-24T09:44:47Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50042/

SparkQA · 2021-11-24T10:38:31Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50042/

SparkQA · 2021-11-24T10:55:41Z

Test build #145565 has finished for PR 34686 at commit 8c0a8c9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AlterDatabaseSetLocationCommand(databaseName: String, location: String)
class DataSourceV2Strategy(session: SparkSession) extends Strategy

SparkQA · 2021-11-24T13:57:26Z

Test build #145570 has finished for PR 34686 at commit 068473f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DataSourceV2Strategy(session: SparkSession) extends Strategy with PredicateHelper

cloud-fan · 2021-11-24T16:32:57Z

thanks, merging to master!

… command by default ### What changes were proposed in this pull request? This PR proposes to use V2 commands as default as outlined in [SPARK-36588](https://issues.apache.org/jira/browse/SPARK-36588), and this PR migrates `ALTER NAMESPACE ... SET LOCATION` to use v2 command by default. Note that the work to make v1/v2 commands consistent was done in #34686 and tests covering both v1/v2 were done in #34610. ### Why are the changes needed? It's been a while since we introduced the v2 commands, and it seems reasonable to use v2 commands by default even for the session catalog, with a legacy config to fall back to the v1 commands. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing *AlterNamespaceSetLocationSuite tests cover this PR's change. Closes #34708 from imback82/v2_alter_ns_location. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

initial commit

28ce116

github-actions bot added the SQL label Nov 23, 2021

imback82 mentioned this pull request Nov 23, 2021

[SPARK-34332][SQL][TEST] Unify v1 and v2 ALTER NAMESPACE .. SET LOCATION tests #34610

Closed

cloud-fan reviewed Nov 23, 2021

View reviewed changes

address pr comment

8c0a8c9

imback82 commented Nov 24, 2021

View reviewed changes

cloud-fan reviewed Nov 24, 2021

View reviewed changes

cloud-fan approved these changes Nov 24, 2021

View reviewed changes

address PR comment

068473f

cloud-fan closed this in 24561ca Nov 24, 2021

imback82 mentioned this pull request Nov 25, 2021

[SPARK-37311][SQL] Migrate ALTER NAMESPACE ... SET LOCATION to use V2 command by default #34708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

imback82 commented Nov 23, 2021

SparkQA commented Nov 23, 2021

SparkQA commented Nov 23, 2021

imback82 commented Nov 23, 2021

SparkQA commented Nov 23, 2021

cloud-fan Nov 23, 2021

cloud-fan Nov 23, 2021

imback82 Nov 24, 2021 •

edited

imback82 Nov 24, 2021

imback82 Nov 24, 2021

imback82 Nov 24, 2021

SparkQA commented Nov 24, 2021

SparkQA commented Nov 24, 2021

cloud-fan Nov 24, 2021

imback82 Nov 24, 2021

cloud-fan Nov 24, 2021

imback82 Nov 24, 2021

SparkQA commented Nov 24, 2021

SparkQA commented Nov 24, 2021

SparkQA commented Nov 24, 2021

SparkQA commented Nov 24, 2021

cloud-fan commented Nov 24, 2021

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

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

Conversation

imback82 commented Nov 23, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Nov 23, 2021

SparkQA commented Nov 23, 2021

imback82 commented Nov 23, 2021

SparkQA commented Nov 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imback82 Nov 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 24, 2021

SparkQA commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 24, 2021

SparkQA commented Nov 24, 2021

SparkQA commented Nov 24, 2021

SparkQA commented Nov 24, 2021

cloud-fan commented Nov 24, 2021

imback82 Nov 24, 2021 •

edited