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

imback82 · 2021-11-15T20:39:23Z

What changes were proposed in this pull request?

Move ALTER NAMESPACE ... SET LOCATION parsing tests to AlterNamespaceSetLocationParserSuite.
Put common ALTER NAMESPACE ... SET LOCATION tests into one trait org.apache.spark.sql.execution.command.AlterNamespaceSetLocationSuiteBase, and put datasource specific tests to the v1.AlterNamespaceSetLocationSuite and v2.AlterNamespaceSetLocationSuite.

The changes follow the approach of #30287.

Why are the changes needed?

The unification will allow to run common ALTER NAMESPACE ... SET LOCATION tests for both DSv1/Hive DSv1 and DSv2
We can detect missing features and differences between DSv1 and DSv2 implementations.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing unit tests and new tests.

imback82 · 2021-11-15T20:40:15Z

cc @cloud-fan @MaxGekk Thanks in advance!

imback82 · 2021-11-15T20:41:20Z

...rc/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetLocationSuite.scala

+      withTempDir { tmpDir =>
+        sql(s"ALTER NAMESPACE $catalog.$ns SET LOCATION '${tmpDir.toURI}'")
+        val sessionCatalog = spark.sessionState.catalog
+        val uriInCatalog = sessionCatalog.getDatabaseMetadata(ns).locationUri


@cloud-fan This check is used in v1 session catalog (existing code), but this can be unified with v2 test if we use DESCRIBE NAMESPACE, WDYT?

yea let's unify it, so that we can move this test to the base trait.

SparkQA · 2021-11-15T21:22:36Z

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

SparkQA · 2021-11-15T22:06:29Z

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

SparkQA · 2021-11-16T01:39:16Z

Test build #145250 has finished for PR 34610 at commit 4629166.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

...rc/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetLocationSuite.scala

.../test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetLocationSuite.scala

...c/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetLocationSuiteBase.scala

imback82 · 2021-11-21T05:25:37Z

...rc/test/scala/org/apache/spark/sql/execution/command/v2/AlterNamespaceSetLocationSuite.scala

+    runBasicTest()
+  }
+
+  test("Empty location string is allowed") {


This is another difference. v1 doesn't allow empty string because it converts the string to URI.

SparkQA · 2021-11-21T06:28:17Z

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

SparkQA · 2021-11-21T07:11:47Z

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

SparkQA · 2021-11-21T10:50:45Z

Test build #145481 has finished for PR 34610 at commit 6df9efd.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2021-11-22T08:10:58Z

...c/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetLocationSuiteBase.scala

+    assert(message.contains(s"$notFoundMsgPrefix '$ns' not found"))
+  }
+
+  // Hive catalog does not support "ALTER NAMESPACE ... SET LOCATION", thus


maybe we can have a

protected def namespaceKeyword = "NAMESPACE"

and in the hive test we override it

override def namespaceKeyword = "DATABASE"

Then we can define the test in base trait

test(...) { ... sql(s"ALTER $namepaceKeyword ...") }

or we just allow using NAMESPACE in hive context as well.

I meant Hive doesn't support altering location:

spark/sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

Lines 356 to 360 in 04671bd

override def alterDatabase(database: CatalogDatabase): Unit = withHiveState {

if (!getDatabase(database.name).locationUri.equals(database.locationUri)) {

// SPARK-29260: Enable supported versions once it support altering database location.

if (!(version.equals(hive.v3_0) || version.equals(hive.v3_1))) {

throw QueryCompilationErrors.alterDatabaseLocationUnsupportedError(version.fullVersion)

I see, thanks for the explanation!

…ty location consistently across v1 and v2 command ### 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 Closes #34686 from imback82/empty_location_fix. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…et_loation_tests2

imback82 · 2021-11-25T04:30:06Z

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

    }
  }

-  test("SPARK-37444: ALTER NAMESPACE .. SET LOCATION using v2 catalog with empty location") {


This is now being tested in test("Empty location string")

SparkQA · 2021-11-25T06:56:44Z

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

SparkQA · 2021-11-25T07:41:38Z

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

cloud-fan · 2021-11-25T07:47:27Z

thanks, merging to master!

SparkQA · 2021-11-25T10:02:30Z

Test build #145617 has finished for PR 34610 at commit caacba6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

… 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 <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…NAMESPACE .. SET LOCATION ### What changes were proposed in this pull request? This is a follow up to #34610 to remove unnecessary test for `ALTER NAMESPACE .. SET LOCATION`. The test being removed is already covered by https://github.com/apache/spark/blob/7b000116575cdc8e42653a694d9ad5273346fa61/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetLocationSuiteBase.scala#L65 ### Why are the changes needed? To remove unnecessary test. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Removing test. Closes #34854 from imback82/followup-SPARK-34332. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? This pr aims to upgrade Arrow from 14.0.2 to 15.0.0, this version fixes the compatibility issue with Netty 4.1.104.Final(GH-39265). Additionally, since the `arrow-vector` module uses `eclipse-collections` to replace `netty-common` as a compile-level dependency, Apache Spark has added a dependency on `eclipse-collections` after upgrading to use Arrow 15.0.0. ### Why are the changes needed? The new version brings the following major changes: Bug Fixes GH-34610 - [Java] Fix valueCount and field name when loading/transferring NullVector GH-38242 - [Java] Fix incorrect internal struct accounting for DenseUnionVector#getBufferSizeFor GH-38254 - [Java] Add reusable buffer getters to char/binary vectors GH-38366 - [Java] Fix Murmur hash on buffers less than 4 bytes GH-38387 - [Java] Fix JDK8 compilation issue with TestAllTypes GH-38614 - [Java] Add VarBinary and VarCharWriter helper methods to more writers GH-38725 - [Java] decompression in Lz4CompressionCodec.java does not set writer index New Features and Improvements GH-38511 - [Java] Add getTransferPair(Field, BufferAllocator, CallBack) for StructVector and MapVector GH-14936 - [Java] Remove netty dependency from arrow-vector GH-38990 - [Java] Upgrade to flatc version 23.5.26 GH-39265 - [Java] Make it run well with the netty newest version 4.1.104 The full release notes as follows: - https://arrow.apache.org/release/15.0.0.html ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass GitHub Actions ### Was this patch authored or co-authored using generative AI tooling? No Closes #44797 from LuciferYang/SPARK-46718. Authored-by: yangjie01 <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

imback82 added 2 commits November 12, 2021 16:58

initial commit

a17986e

move empty location string

4629166

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

imback82 commented Nov 15, 2021

View reviewed changes

jerqi reviewed Nov 16, 2021

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Nov 16, 2021

View reviewed changes

...rc/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetLocationSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Nov 16, 2021

View reviewed changes

.../test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetLocationSuite.scala Show resolved Hide resolved

address PR comments

6df9efd

imback82 commented Nov 21, 2021

View reviewed changes

cloud-fan reviewed Nov 22, 2021

View reviewed changes

imback82 mentioned this pull request Nov 23, 2021

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

Closed

imback82 added 4 commits November 24, 2021 20:05

Merge remote-tracking branch 'upstream/master' into alter_namespace_s…

b1874f1

…et_loation_tests2

move empty locatio test

b32d397

Merge branch 'master' into alter_namespace_set_loation_tests2

c93eb8c

clean up test

caacba6

imback82 commented Nov 25, 2021

View reviewed changes

cloud-fan approved these changes Nov 25, 2021

View reviewed changes

cloud-fan closed this in f175362 Nov 25, 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

imback82 changed the title ~~[SPARK-34332][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET LOCATION tests~~ [SPARK-34332][SQL][TEST] Unify v1 and v2 ALTER NAMESPACE .. SET LOCATION tests Dec 9, 2021

imback82 mentioned this pull request Dec 9, 2021

[SPARK-34332][SQL][TEST][FOLLOWUP] Remove unnecessary test for ALTER NAMESPACE .. SET LOCATION #34854

Closed

LuciferYang mentioned this pull request Jan 23, 2024

[SPARK-46718][BUILD] Upgrade Arrow to 15.0.0 #44797

Closed

	override def alterDatabase(database: CatalogDatabase): Unit = withHiveState {
	if (!getDatabase(database.name).locationUri.equals(database.locationUri)) {
	// SPARK-29260: Enable supported versions once it support altering database location.
	if (!(version.equals(hive.v3_0) \|\| version.equals(hive.v3_1))) {
	throw QueryCompilationErrors.alterDatabaseLocationUnsupportedError(version.fullVersion)

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

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

Uh oh!

Conversation

imback82 commented Nov 15, 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?

Uh oh!

imback82 commented Nov 15, 2021

Uh oh!

imback82 Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 15, 2021

Uh oh!

SparkQA commented Nov 15, 2021

Uh oh!

SparkQA commented Nov 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imback82 Nov 21, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 21, 2021

Uh oh!

SparkQA commented Nov 21, 2021

Uh oh!

SparkQA commented Nov 21, 2021

Uh oh!

cloud-fan Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

imback82 Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imback82 Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 25, 2021

Uh oh!

SparkQA commented Nov 25, 2021

Uh oh!

cloud-fan commented Nov 25, 2021

Uh oh!

SparkQA commented Nov 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cloud-fan Nov 25, 2021 •

edited

Loading