Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Jun 19, 2022

What changes were proposed in this pull request?

  1. Move DESCRIBE TABLE parsing tests to DescribeRelationParserSuite.
  2. Put common DESCRIBE TABLE tests into one trait org.apache.spark.sql.execution.command.DescribeTableSuiteBase, and put datasource specific tests to the v1.DescribeTableSuite and v2.DescribeTableSuite.

The changes follow the approach of #30287.

Closes #36671.

Why are the changes needed?

  1. The unification will allow to run common DESCRIBE TABLE tests for both DSv1/Hive DSv1 and DSv2
  2. 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 the modified test suites:

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

and new test suites:

$ build/sbt "sql/test:testOnly *DescribeTableParserSuite"
$ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *DescribeTableSuite"

@github-actions github-actions bot added the SQL label Jun 19, 2022
@MaxGekk MaxGekk changed the title [WIP][SPARK-37888][SQL][TESTS] Unify v1 and v2 DESCRIBE TABLE tests [SPARK-37888][SQL][TESTS] Unify v1 and v2 DESCRIBE TABLE tests Jun 20, 2022
@MaxGekk MaxGekk marked this pull request as ready for review June 20, 2022 15:54
@MaxGekk MaxGekk requested a review from cloud-fan June 20, 2022 15:54
@cloud-fan
Copy link
Contributor

A major problem in #35265 is how to unify the v1 and v2 DESCRIBE TABLE command. I'm OK to unify the tests first, and let's unify the commands in a later PR.

@MaxGekk
Copy link
Member Author

MaxGekk commented Jun 21, 2022

Merging to master. Thank you, @cloud-fan for review.

@MaxGekk MaxGekk closed this in 42d33e1 Jun 21, 2022
Row("Database", "ns", ""),
Row("Table", "table", ""),
Row("Last Access", "UNKNOWN", ""),
Row("Created By", "Spark 3.4.0-SNAPSHOT", ""),
Copy link
Contributor

Choose a reason for hiding this comment

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

we should filter out this row as well, otherwise we need to keep updating this test when making a new release. cc @MaxGekk

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in another PR for DESCRIBE TABLE tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Next time, let's address comments immediately. We missed fixing it in the next PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants