[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-25 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/17060


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread dilipbiswal 
Github user dilipbiswal commented on a diff in the pull request:

https://github.com/apache/spark/pull/17060#discussion_r103071356
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -241,7 +243,8 @@ class SQLQueryTestSuite extends QueryTest with 
SharedSQLContext {
   private def listTestCases(): Seq[TestCase] = {
 listFilesRecursively(new File(inputFilePath)).map { file =>
   val resultFile = file.getAbsolutePath.replace(inputFilePath, 
goldenFilePath) + ".out"
-  TestCase(file.getName, file.getAbsolutePath, resultFile)
+  val absPath = file.getAbsolutePath
+  TestCase(absPath.stripPrefix(inputFilePath + File.separator), 
absPath, resultFile)
--- End diff --

@srowen Sure. Although i think we are safe, i would go ahead and make that 
change. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread srowen 
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/17060#discussion_r103071094
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -98,7 +98,9 @@ class SQLQueryTestSuite extends QueryTest with 
SharedSQLContext {
 
   /** List of test cases to ignore, in lower cases. */
   private val blackList = Set(
-"blacklist.sql"  // Do NOT remove this one. It is here to test the 
blacklist functionality.
+"blacklist.sql",  // Do NOT remove this one. It is here to test the 
blacklist functionality.
+".ds_store"   // A meta-file that may be created on Mac by Finder 
App.
--- End diff --

That's right, you'd have to lower-case both. I think that's clearer.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread dilipbiswal 
Github user dilipbiswal commented on a diff in the pull request:

https://github.com/apache/spark/pull/17060#discussion_r103070909
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -98,7 +98,9 @@ class SQLQueryTestSuite extends QueryTest with 
SharedSQLContext {
 
   /** List of test cases to ignore, in lower cases. */
   private val blackList = Set(
-"blacklist.sql"  // Do NOT remove this one. It is here to test the 
blacklist functionality.
+"blacklist.sql",  // Do NOT remove this one. It is here to test the 
blacklist functionality.
+".ds_store"   // A meta-file that may be created on Mac by Finder 
App.
--- End diff --

@srowen I did think about it. If we want to keep the mixed case here in 
this list, then we have to change the code later to lowercase both the sides 
and then compare ? Would you prefer to change the comparison to the following ?
```
if (blackList.exists(t => 
testCase.name.toLowerCase.contains(t.toLowerCase))) {
...
}
```


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread srowen 
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/17060#discussion_r103070856
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -241,7 +243,8 @@ class SQLQueryTestSuite extends QueryTest with 
SharedSQLContext {
   private def listTestCases(): Seq[TestCase] = {
 listFilesRecursively(new File(inputFilePath)).map { file =>
   val resultFile = file.getAbsolutePath.replace(inputFilePath, 
goldenFilePath) + ".out"
-  TestCase(file.getName, file.getAbsolutePath, resultFile)
+  val absPath = file.getAbsolutePath
+  TestCase(absPath.stripPrefix(inputFilePath + File.separator), 
absPath, resultFile)
--- End diff --

Does this get into problems where inputFilePath might already have a 
trailing separator and then isn't a prefix? maybe it can't happen, but maybe 
it's easier to strip leading separators from the final result, directly, if 
that's what you mean to do.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread dilipbiswal 
Github user dilipbiswal commented on a diff in the pull request:

https://github.com/apache/spark/pull/17060#discussion_r103070711
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -241,7 +243,8 @@ class SQLQueryTestSuite extends QueryTest with 
SharedSQLContext {
   private def listTestCases(): Seq[TestCase] = {
 listFilesRecursively(new File(inputFilePath)).map { file =>
   val resultFile = file.getAbsolutePath.replace(inputFilePath, 
goldenFilePath) + ".out"
-  TestCase(file.getName, file.getAbsolutePath, resultFile)
+  val absPath = file.getAbsolutePath
+  TestCase(absPath.stripPrefix(inputFilePath + File.separator), 
absPath, resultFile)
--- End diff --

@srowen I am adding a separator to make sure the test case name does not 
have a leading separator character. Example: 
```
file = 
/home/spark/sql/core/src/test/resources/sql-tests/inputs/subquery/exists-subquery/exists-basic.sql
inputFilePath = /home/spark/sql/core/src/test/resources/sql-tests/inputs
```
So i wanted the test case name to be 
`subquery/exists-subquery/exists-basic.sql` and not 
`/subquery/exists-subquery/exists-basic.sql`


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread dilipbiswal 
Github user dilipbiswal commented on a diff in the pull request:

https://github.com/apache/spark/pull/17060#discussion_r103070622
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -121,7 +123,7 @@ class SQLQueryTestSuite extends QueryTest with 
SharedSQLContext {
   }
 
   private def createScalaTestCase(testCase: TestCase): Unit = {
-if (blackList.contains(testCase.name.toLowerCase)) {
+if (blackList.exists(testCase.name.toLowerCase.contains(_))) {
--- End diff --

@srowen Thanks !! I will change.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread srowen 
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/17060#discussion_r103068302
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -121,7 +123,7 @@ class SQLQueryTestSuite extends QueryTest with 
SharedSQLContext {
   }
 
   private def createScalaTestCase(testCase: TestCase): Unit = {
-if (blackList.contains(testCase.name.toLowerCase)) {
+if (blackList.exists(testCase.name.toLowerCase.contains(_))) {
--- End diff --

Nit, you can remove the `(_)` here


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread srowen 
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/17060#discussion_r103068318
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -241,7 +243,8 @@ class SQLQueryTestSuite extends QueryTest with 
SharedSQLContext {
   private def listTestCases(): Seq[TestCase] = {
 listFilesRecursively(new File(inputFilePath)).map { file =>
   val resultFile = file.getAbsolutePath.replace(inputFilePath, 
goldenFilePath) + ".out"
-  TestCase(file.getName, file.getAbsolutePath, resultFile)
+  val absPath = file.getAbsolutePath
+  TestCase(absPath.stripPrefix(inputFilePath + File.separator), 
absPath, resultFile)
--- End diff --

Why add the separator here?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread srowen 
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/17060#discussion_r103068296
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -98,7 +98,9 @@ class SQLQueryTestSuite extends QueryTest with 
SharedSQLContext {
 
   /** List of test cases to ignore, in lower cases. */
   private val blackList = Set(
-"blacklist.sql"  // Do NOT remove this one. It is here to test the 
blacklist functionality.
+"blacklist.sql",  // Do NOT remove this one. It is here to test the 
blacklist functionality.
+".ds_store"   // A meta-file that may be created on Mac by Finder 
App.
--- End diff --

.DS_Store right? I know you compare lower-case later but might as well 
write it as it appears


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #17060: [SQL] Duplicate test exception in SQLQueryTestSui...

2017-02-24 Thread dilipbiswal 
GitHub user dilipbiswal opened a pull request:

https://github.com/apache/spark/pull/17060

[SQL] Duplicate test exception in SQLQueryTestSuite due to meta 
files(.DS_Store) on Mac

## What changes were proposed in this pull request?
After adding the tests for subquery, we now have multiple level of 
directories under "sql-tests/inputs".  Some times on Mac while using Finder 
application it creates the meta data files called ".DS_Store". When these files 
are present at different levels in directory hierarchy, we get duplicate test 
exception while running the tests  as we just use the file name as the test 
case name. In this PR, we use the relative file path from the base directory 
along with the test file as the test name. Here is the truncated output of the 
test run after the change. Also after this change, we can have the same test 
file name under different directory like exists/basic.sql , in/basic.sql.

```SQL
info] SQLQueryTestSuite:
[info] - arithmetic.sql (5 seconds, 235 milliseconds)
[info] - array.sql (536 milliseconds)
[info] - blacklist.sql !!! IGNORED !!!
[info] - cast.sql (550 milliseconds)



[info] - union.sql (315 milliseconds)
[info] - subquery/.DS_Store !!! IGNORED !!!
[info] - subquery/exists-subquery/.DS_Store !!! IGNORED !!!
[info] - subquery/exists-subquery/exists-aggregate.sql (2 seconds, 451 
milliseconds)


[info] - subquery/in-subquery/in-group-by.sql (12 seconds, 264 milliseconds)


[info] - subquery/scalar-subquery/scalar-subquery-predicate.sql (7 seconds, 
769 milliseconds)
[info] - subquery/scalar-subquery/scalar-subquery-select.sql (4 seconds, 
119 milliseconds)
```
Since this is a simple change, i haven't created a JIRA for it.  
## How was this patch tested?
Manually verified. This is change to test infrastructure



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dilipbiswal/spark sqlquerytestsuite

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/17060.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 #17060


commit 5f2a6a82244a623fed142555f55dd34060426e5a
Author: Dilip Biswal 
Date:   2017-02-24T22:28:56Z

Duplicate test exception




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org