[SPARK-30673][SQL][TESTS] Test cases in HiveShowCreateTableSuite should create Hive table#27393
[SPARK-30673][SQL][TESTS] Test cases in HiveShowCreateTableSuite should create Hive table#27393viirya wants to merge 4 commits intoapache:masterfrom
Conversation
|
I found that few test cases in HiveShowCreateTableSuite behave differently and actually create datasource table, when I want to sync #24938 with master. So this patch updates them. |
|
Thank you for pinging me, @viirya . |
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveShowCreateTableSuite.scala
Show resolved
Hide resolved
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM (Pending Jenkins).
|
Thanks! @dongjoon-hyun |
|
Test build #117544 has finished for PR 27393 at commit
|
|
Test build #117547 has finished for PR 27393 at commit
|
|
Retest this please. |
HyukjinKwon
changed the title
| withSQLConf( | ||
| SQLConf.LEGACY_CREATE_HIVE_TABLE_BY_DEFAULT_ENABLED.key -> "true") { | ||
| withTable("t1") { | ||
| val createTable = "CREATE TABLE `t1`(`a` STRUCT<`b`: STRING>) USING hive" |
There was a problem hiding this comment.
Hm, doesn't this already creates a Hive table?
There was a problem hiding this comment.
Actually, I asked to cover all cases to avoid future parser changes at the other test cases (partitioned table), but @HyukjinKwon 's comment also makes sense at this case. @viirya . Sorry, you can omit these two cases.
There was a problem hiding this comment.
Oops, I missed that comment. What about we do it in beforeAll o beforeEach? I left a comment because it enables it for each test manually and it looked odd to do it for unnecessary cases as well.
| | c1 INT COMMENT 'bla', | ||
| | c2 STRING | ||
| |) | ||
| |STORED AS PARQUET |
|
Test build #117553 has finished for PR 27393 at commit
|
| test("simple external hive table") { | ||
| withTempDir { dir => | ||
| withSQLConf( | ||
| SQLConf.LEGACY_CREATE_HIVE_TABLE_BY_DEFAULT_ENABLED.key -> "true") { |
There was a problem hiding this comment.
legacy configs may get removed in future versions. Can we update the testing query to add USING hive or STORE AS TEXT?
There was a problem hiding this comment.
There is a test simple hive table. The purpose of the test should be a very simple CREATE TABLE query. My original proposal is just to add legacy config enabled for few tests like this one.
Adding USING hive is ok, I'm just concerned that it will reduce test coverage.
There was a problem hiding this comment.
I think as @HyukjinKwon said we can use beforeAll. WDYT?
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveShowCreateTableSuite.scala
Outdated
Show resolved
Hide resolved
|
Test build #117575 has finished for PR 27393 at commit
|
|
Test build #117576 has finished for PR 27393 at commit
|
|
Thanks! I'm going to merge this. |
|
Merged into master. |
5 participants
What changes were proposed in this pull request?
This patch makes the test cases in HiveShowCreateTableSuite create Hive table instead of data source table.
Why are the changes needed?
Because SparkSQL now creates data source table if no provider is specified in SQL command, some test cases in HiveShowCreateTableSuite don't create Hive table, but data source table.
It is confusing and not good for the purpose of this test suite.
Does this PR introduce any user-facing change?
No, only test case.
How was this patch tested?
Unit test.