[SPARK-37195][SQL][TESTS] Unify v1 and v2 SHOW TBLPROPERTIES tests #34476

Peng-Lei · 2021-11-03T09:49:21Z

What changes were proposed in this pull request?

Move SHOW TBLPROPERTIES parsing tests to ShowTblPropertiesParserSuite.
Define the class command.ShowTblPropertiesSuiteBase that is parent of v1.ShowTblPropertiesSuiteBase and v2.ShowTblPropertiesSuite.
Define the class v1.ShowTblPropertiesSuiteBase that is parent of v1.ShowTblPropertiesSuite and hive.execution.command.ShowTblPropertiesSuite.
move test case from DataSourceV2SQLSessionCatalogSuite to command.ShowTblPropertiesSuiteBase to run for V2/V1/HIVE V1
move some test case from HiveCommandSuite to command.ShowTblPropertiesSuiteBase
move some test case from HiveCommandSuite to v1.ShowTblPropertiesSuiteBase to run for V1/HIVE V1
Add two test case for v1.ShowTblPropertiesSuite for view.

The changes follow the approach of #30287 #34305

Why are the changes needed?

The unification will allow to run common SHOW TBLPROPERTIES 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?

By running v1/v2 and Hive v1 ShowTblPropertiesSuite:

$ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *ShowTblPropertiesSuite"

Peng-Lei · 2021-11-03T09:57:53Z

I'm not sure that
L1229 should move to ShowTblPropertiesParserSuite, because it is test Resolve not Parser and we should move some method more if do it. @MaxGekk @imback82 @cloud-fan

SparkQA · 2021-11-03T10:47:06Z

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

SparkQA · 2021-11-03T11:32:00Z

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

MaxGekk

L1229 should move to ShowTblPropertiesParserSuite, because it is test Resolve not Parser

@Peng-Lei If you have such tests, please, place them to a separate test suite but not to *ParserSuite. Actually, I planned to do so if I would work on unifying of v1 and v2 CREATE TABLE tests.

SparkQA · 2021-11-03T15:03:58Z

Test build #144876 has finished for PR 34476 at commit b66d351.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ShowTblPropertiesParserSuite extends AnalysisTest
trait ShowTblPropertiesSuiteBase extends QueryTest with DDLCommandTestUtils
class ShowTblPropertiesSuite extends ShowTblPropertiesSuiteBase with CommandSuiteBase
class ShowTblPropertiesSuite extends command.ShowTblPropertiesSuiteBase with CommandSuiteBase
class ShowTblPropertiesSuite extends v1.ShowTblPropertiesSuiteBase with CommandSuiteBase

Peng-Lei · 2021-11-04T06:25:56Z

If you have such tests, please, place them to a separate test suite but not to *ParserSuite. Actually, I planned to do so if I would work on unifying of v1 and v2 CREATE TABLE tests.

@MaxGekk , Could you continue to look at it, and I made the following modifications

I define the PlanResolutionSuiteBase that contains the properties and methods of PlanResolutionSuite.
Define the v1.ShowTblPropertiesPlanResolutionSuite and v2.ShowTblPropertiesPlanResolutionSuite extends the PlanResolutionSuiteBase, so that they can use method of parent class.
In order to guarantee PlanResolutionSuite is right that after extract method/properties to properties , I change that PlanResolutionSuite extends PlanResolutionSuiteBase.
PlanResolutionSuiteBase may not be abstract enough, but enough for this PR.

cloud-fan · 2021-11-04T06:34:36Z

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

Let's leave this suite unchanged.

Today we have a unified framework to resolve database objects in commands, and we don't need to test plan resolution for each command. We will probably clean up this test suite to only test DML commands.

Today we have a unified framework to resolve database objects in commands, and we don't need to test plan resolution for each command.

Thank you for reminding me, you're quite right. I will undo the last modification

SparkQA · 2021-11-04T07:31:41Z

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

SparkQA · 2021-11-04T08:31:11Z

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

SparkQA · 2021-11-04T11:40:11Z

Test build #144895 has finished for PR 34476 at commit 2806dfd.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class PlanResolutionSuite extends PlanResolutionSuiteBase
class PlanResolutionSuiteBase extends AnalysisTest
class ShowTblPropertiesParserSuite extends AnalysisTest
trait ShowTblPropertiesSuiteBase extends QueryTest with DDLCommandTestUtils
class ShowTblPropertiesPlanResolutionSuite extends PlanResolutionSuiteBase
class ShowTblPropertiesSuite extends ShowTblPropertiesSuiteBase with CommandSuiteBase
class ShowTblPropertiesPlanResolutionSuite extends PlanResolutionSuiteBase
class ShowTblPropertiesSuite extends command.ShowTblPropertiesSuiteBase with CommandSuiteBase
class ShowTblPropertiesSuite extends v1.ShowTblPropertiesSuiteBase with CommandSuiteBase

SparkQA · 2021-11-11T08:00:53Z

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

cloud-fan · 2021-11-11T08:29:47Z

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

is it possible to unify the test? seems this PR just move the old tests to either v1 or v2 test suite, but did not unify them.

Hold on. Let me think more and learn more about #30287 #34305

SparkQA · 2021-11-11T08:59:47Z

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

SparkQA · 2021-11-11T12:16:57Z

Test build #145091 has finished for PR 34476 at commit 95a2266.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ShowTblPropertiesParserSuite extends AnalysisTest
trait ShowTblPropertiesSuiteBase extends QueryTest with DDLCommandTestUtils
class ShowTblPropertiesSuite extends ShowTblPropertiesSuiteBase with CommandSuiteBase
class ShowTblPropertiesSuite extends command.ShowTblPropertiesSuiteBase with CommandSuiteBase
class ShowTblPropertiesSuite extends v1.ShowTblPropertiesSuiteBase with CommandSuiteBase

cloud-fan · 2021-11-15T08:03:16Z

are you going to unify the tests? @Peng-Lei

Peng-Lei · 2021-11-15T08:05:29Z

are you going to unify the tests? @Peng-Lei

Yeah. I am going work on this. Sorry. I clicked the wrong request for review. @cloud-fan

Peng-Lei · 2021-11-16T11:37:32Z

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

+      spark.sql(s"CREATE TABLE $tbl (id bigint, data string) $defaultUsing " +
+        s"TBLPROPERTIES ('user'='$user', 'status'='$status')")
+
+      val properties = sql(s"SHOW TBLPROPERTIES $tbl").filter("key != 'transient_lastDdlTime'")


In order to be compatible Hive table, so add the filter action.

Peng-Lei · 2021-11-16T11:39:51Z

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

+
+      val properties = sql(s"SHOW TBLPROPERTIES $tbl ('$nonExistingKey')")
+      val expected = Seq(Row(nonExistingKey,
+        s"Table ns1.tbl does not have property: $nonExistingKey"))


The table name without catalog name in V1 different with V2.

can we change the test code so that it can be moved to the base class? e.g.

val res = properties.collect() assert(res.length == 1) assert(res.head.getString(0) == nonExistingKey) assert(res.head.getString(1).contains("does not have property: $nonExistingKey"))

Peng-Lei · 2021-11-16T11:40:53Z

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

+    withNamespaceAndTable("ns1", "tbl") { tbl =>
+      spark.sql(s"CREATE TABLE $tbl (id bigint, data string) $defaultUsing " +
+        s"TBLPROPERTIES ('user'='andrew', 'status'='new')")
+      withSQLConf(SQLConf.LEGACY_KEEP_COMMAND_OUTPUT_SCHEMA.key -> "true") {


In V1. We can set LEGACY_KEEP_COMMAND_OUTPUT_SCHEMA to control the output.

Peng-Lei · 2021-11-16T11:47:13Z

update as follow:

move test case from DataSourceV2SQLSessionCatalogSuite to command.ShowTblPropertiesSuiteBase to run for V2/V1/HIVE V1
move some test case from HiveCommandSuite to command.ShowTblPropertiesSuiteBase
move some test case from HiveCommandSuite to v1.ShowTblPropertiesSuiteBase to run for V1/HIVE V1
Add two test case for v1.ShowTblPropertiesSuite for view.

SparkQA · 2021-11-16T12:22:03Z

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

SparkQA · 2021-11-16T13:20:14Z

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

cloud-fan · 2021-11-16T15:27:24Z

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

+        Row("user", user))
+
+      assert(properties.schema === schema)
+      assert(expected === properties.collect().sortBy(_.toString))


nit: we can use checkAnswer to check result

cloud-fan · 2021-11-16T15:28:19Z

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

+
+      val properties = sql(s"SHOW TBLPROPERTIES $tbl ('status')")
+      val expected = Seq(Row("status", status))
+      assert(expected === properties.collect())


cloud-fan · 2021-11-16T15:31:50Z

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

+class ShowTblPropertiesSuite extends ShowTblPropertiesSuiteBase with CommandSuiteBase {
+  override def commandVersion: String = super[ShowTblPropertiesSuiteBase].commandVersion
+
+  test("SHOW TBLPROPERTIES FOR VIEW") {


The reason to have these 2 tests here is because hive can return more properties?

if so can we add a comment to explain it?

The reason to have these 2 tests here is because hive can return more properties?

Yeah. An additional attribute transient_lastDdlTime is added for hive. I try to move the test case to v1.base with filter the properties transient_lastDdlTime. So they can run for V1/HIVE V1

cloud-fan

LGTM except for some minor comments

SparkQA · 2021-11-16T16:20:05Z

Test build #145277 has finished for PR 34476 at commit a6db08e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ShowTblPropertiesParserSuite extends AnalysisTest
trait ShowTblPropertiesSuiteBase extends QueryTest with DDLCommandTestUtils
class ShowTblPropertiesSuite extends ShowTblPropertiesSuiteBase with CommandSuiteBase
class ShowTblPropertiesSuite extends command.ShowTblPropertiesSuiteBase with CommandSuiteBase
class ShowTblPropertiesSuite extends v1.ShowTblPropertiesSuiteBase with CommandSuiteBase

Peng-Lei · 2021-11-17T07:53:33Z

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

+        Row("user", user))
+
+      assert(properties.schema === schema)
+      checkAnswer(properties.sort("key"), expected)


v2: sort the result. v1: without the sort. So the test case sort the result before the compare

v1 should sort as well. We can fix this in a followup.

SparkQA · 2021-11-17T09:18:54Z

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

SparkQA · 2021-11-17T09:56:56Z

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

SparkQA · 2021-11-17T13:21:36Z

Test build #145325 has finished for PR 34476 at commit 95e30cf.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ShowTblPropertiesSuite extends ShowTblPropertiesSuiteBase with CommandSuiteBase

cloud-fan · 2021-11-17T13:26:07Z

thanks, merging to master!

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

MaxGekk reviewed Nov 3, 2021

View reviewed changes

Peng-Lei force-pushed the SPARK-37195 branch from b66d351 to 2806dfd Compare November 4, 2021 06:13

cloud-fan reviewed Nov 4, 2021

View reviewed changes

Peng-Lei force-pushed the SPARK-37195 branch from 2806dfd to 95a2266 Compare November 11, 2021 06:59

cloud-fan reviewed Nov 11, 2021

View reviewed changes

Peng-Lei requested review from MaxGekk and cloud-fan November 15, 2021 07:24

add draft

a6db08e

Peng-Lei force-pushed the SPARK-37195 branch from 95a2266 to a6db08e Compare November 16, 2021 11:23

Peng-Lei commented Nov 16, 2021

View reviewed changes

cloud-fan reviewed Nov 16, 2021

View reviewed changes

cloud-fan approved these changes Nov 16, 2021

View reviewed changes

address comments

95e30cf

Peng-Lei commented Nov 17, 2021

View reviewed changes

cloud-fan approved these changes Nov 17, 2021

View reviewed changes

cloud-fan closed this in 06586f6 Nov 17, 2021

[SPARK-37195][SQL][TESTS] Unify v1 and v2 SHOW TBLPROPERTIES tests #34476

[SPARK-37195][SQL][TESTS] Unify v1 and v2 SHOW TBLPROPERTIES tests #34476

Uh oh!

Conversation

Peng-Lei commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Peng-Lei commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 3, 2021

Uh oh!

SparkQA commented Nov 3, 2021

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 3, 2021

Uh oh!

Peng-Lei commented Nov 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 4, 2021

Uh oh!

SparkQA commented Nov 4, 2021

Uh oh!

SparkQA commented Nov 4, 2021

Uh oh!

SparkQA commented Nov 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 11, 2021

Uh oh!

SparkQA commented Nov 11, 2021

Uh oh!

cloud-fan commented Nov 15, 2021

Uh oh!

Peng-Lei commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Peng-Lei commented Nov 16, 2021

Uh oh!

SparkQA commented Nov 16, 2021

Uh oh!

SparkQA commented Nov 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 16, 2021

Peng-Lei commented Nov 3, 2021 •

edited

Loading

Peng-Lei commented Nov 3, 2021 •

edited

Loading

Peng-Lei commented Nov 15, 2021 •

edited

Loading