You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by bomeng <gi...@git.apache.org> on 2016/04/13 04:32:45 UTC
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
GitHub user bomeng opened a pull request:
https://github.com/apache/spark/pull/12347
[SPARK-14441] [SQL] Consolidate DDL tests
## 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 `==` 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bomeng/spark SPARK-14441
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/12347.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #12347
----
commit af298b63693586e7c0598ccd98ba3f9e6a7634f7
Author: bomeng <bm...@us.ibm.com>
Date: 2016-04-13T02:19:27Z
Consolidate DDL tests
commit 06c8948b20cacccead019e42e3a07b89f727893a
Author: bomeng <bm...@us.ibm.com>
Date: 2016-04-13T02:21:52Z
remove extra line
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-213683729
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-209200845
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 functionalities of 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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-209217278
**[Test build #55680 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/55680/consoleFull)** for PR 12347 at commit [`06c8948`](https://github.com/apache/spark/commit/06c8948b20cacccead019e42e3a07b89f727893a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-209622770
Ok, not a problem. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-209200210
```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: https://github.com/apache/spark/pull/12201
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-214518666
closing this PR. thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-209200120
**[Test build #55680 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/55680/consoleFull)** for PR 12347 at commit [`06c8948`](https://github.com/apache/spark/commit/06c8948b20cacccead019e42e3a07b89f727893a).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-209217405
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/55680/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-209217404
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng closed the pull request at:
https://github.com/apache/spark/pull/12347
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/12347#discussion_r59485621
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveDDLCommandSuite.scala ---
@@ -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"))
--- End diff --
All these similar changes will break the Scala 2.10 build
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-14441] [SQL] Consolidate DDL tests
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/12347#issuecomment-209600458
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org