Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-14441] [SQL] Consolidate DDL tests #12347

Closed
wants to merge 2 commits into from
Closed

[SPARK-14441] [SQL] Consolidate DDL tests #12347

wants to merge 2 commits into from

Conversation

bomeng
Copy link
Contributor

@bomeng bomeng commented Apr 13, 2016

What changes were proposed in this pull request?

Today we have DDLSuite, DDLCommandSuite, HiveDDLCommandSuite and HiveDDLSuite. In this PR, I am trying to consolidate the files as much as possible. Two files are left for now, since it is good to put Hive related test suite in the Hive package.

Along with the combination, I also did some modification of current codes mainly in order to improve the codings, here is the summary:

  1. Use .contains(...) instead == Some(...) for Options, this method is introduced in Scala 2.11 and it is recommended method to use for this purpose;
  2. Make map consistent to use ->, instead of 2 elements tuple;
  3. Use isEmpty() instead of == None
  4. Add private to the parser and narrow the scope of implicit (put inside one of the tests);
  5. Modify the names of some tests to be unique, since they are in one file now;

How was this patch tested?

I did not change the logic of any tests, just move around and improve the codes, so existing test cases should remain same.

@@ -113,10 +244,10 @@ class HiveDDLCommandSuite extends PlanTest {

val (desc, exists) = extractTableDesc(s2)
assert(exists)
assert(desc.identifier.database == Some("mydb"))
assert(desc.identifier.database.contains("mydb"))
Copy link
Member

Choose a reason for hiding this comment

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

All these similar changes will break the Scala 2.10 build

@gatorsmile
Copy link
Member

Use .contains(...) instead == Some(...) for Options, this method is introduced in Scala 2.11 and it is recommended method to use for this purpose;
This should not be used in Spark code. It will break the 2.10 build.

See my PR: #12201

@gatorsmile
Copy link
Member

To be honest, I do not know why we need to merge these test case files. Their purposes are different. One is to verify the parsers; another is to verify the execution of DDL/commands. In the future, we might add more test cases to both files. The files will grow bigger and bigger.

cc @yhuai @andrewor14

@SparkQA
Copy link

SparkQA commented Apr 13, 2016

Test build #55680 has finished for PR 12347 at commit 06c8948.

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

@andrewor14
Copy link
Contributor

@gatorsmile that's a good point. I think a better goal of the issue is to make the tests consistent and make sure the functionality of the test suites are clearly isolated. I don't think just literally merging all the test files together is a great idea.

@bomeng also we should wait for all the other DDLs tasks have finished before we start working on this one. Otherwise there will be a lot of conflicts and the later PRs will still put them in the wrong places. I would like to do this myself after we finish all the tasks. There are a lot of inconsistencies that need to be resolved. It would be best if you can work on the other DDL tasks instead.

@bomeng
Copy link
Contributor Author

bomeng commented Apr 13, 2016

Ok, not a problem. Thanks.

@rxin
Copy link
Contributor

rxin commented Apr 23, 2016

Yea the current tests are pretty scattered and they are way too big to merge into a single one. Can you just close this pull request? Thanks.

@bomeng
Copy link
Contributor Author

bomeng commented Apr 25, 2016

closing this PR. thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants