SPARK-9758 #8051
SPARK-9758 #8051
Conversation
Changed MAPR repository to http://repository.mapr.com/maven/
old package org.apache.spark.sql.hive.execution corrected package test.org.apache.spark.sql.hive.execution
resolve compilation issue. Added import statement
|
Hmm, I think the correct thing would be to move the files to the correct directory. All code should be in a sub-package of |
| @@ -248,7 +248,7 @@ | |||
| <repository> | |||
| <id>mapr-repo</id> | |||
| <name>MapR Repository</name> | |||
| <url>http://repository.mapr.com/maven</url> | |||
| <url>http://repository.mapr.com/maven/</url> | |||
There was a problem hiding this comment.
I think this was an unrelated change. You also need to give this PR a proper title. Did you review https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark first?
|
@chenghao-intel @rxin do you know what might have happened here? they were moved in f6f2eeb but it looks like they're in the wrong directory. |
|
It's best for API tests to live in a separate repository, to make sure we don't accidentally change the visibility of public APIs. |
|
I see the point that some tests from outside the It doesn't look like this code was trying to be in test.org.apache.spark since it didn't declare that as the package. But then I don't know how anything ever compiled it since its package doesn't match the dir. Fixing that is fine enough but I'm also wondering whether the other ~10 instances of this in the code are accidents or on purpose. |
|
@ashokblend just checking, does this really not compile? I'm wondering how it works for anyone, since Jenkins and my Hive builds seem OK. @rxin do you find my argument compelling? that the idea of tests from external packages is a good one, but a separate idea? that is, I think this was just an oversight in package and file naming rather than intentional? They don't look like their purpose it to test from an external package. |
|
Jenkins, test this please. |
|
They should all compile with Scala. Some IDEs complain when the files are not in proper places. |
|
@ashokblend can you update the pull request title to make this a little bit more descriptive? |
|
Test build #1593 has finished for PR 8051 at commit
|
|
Test build #40847 has finished for PR 8051 at commit
|
|
I still don't understand why the right fix isn't to move the files to the same directory as all the other tests? These really do not look like some API stability / visibility tests to require a different package name. |
|
@ashokblend are you able to follow up on the requests and comments here? otherwise do you mind closing this pull request? |
…kage? Move `test.org.apache.spark.sql.hive` package tests to apparent intended `org.apache.spark.sql.hive` as they don't intend to test behavior from outside org.apache.spark.* Alternate take, per discussion at #8051 I think this is what vanzin and I had in mind but also CC rxin to cross-check, as this does indeed depend on whether these tests were accidentally in this package or not. Testing from a `test.org.apache.spark` package is legitimate but didn't seem to be the intent here. Author: Sean Owen <sowen@cloudera.com> Closes #8307 from srowen/SPARK-9758. (cherry picked from commit cb2d2e1) Signed-off-by: Sean Owen <sowen@cloudera.com>
…kage? Move `test.org.apache.spark.sql.hive` package tests to apparent intended `org.apache.spark.sql.hive` as they don't intend to test behavior from outside org.apache.spark.* Alternate take, per discussion at #8051 I think this is what vanzin and I had in mind but also CC rxin to cross-check, as this does indeed depend on whether these tests were accidentally in this package or not. Testing from a `test.org.apache.spark` package is legitimate but didn't seem to be the intent here. Author: Sean Owen <sowen@cloudera.com> Closes #8307 from srowen/SPARK-9758.
|
@ashokblend looks like this is resolved through #8307. Would you mind closing this patch? |
…kage? Move `test.org.apache.spark.sql.hive` package tests to apparent intended `org.apache.spark.sql.hive` as they don't intend to test behavior from outside org.apache.spark.* Alternate take, per discussion at apache/spark#8051 I think this is what vanzin and I had in mind but also CC rxin to cross-check, as this does indeed depend on whether these tests were accidentally in this package or not. Testing from a `test.org.apache.spark` package is legitimate but didn't seem to be the intent here. Author: Sean Owen <sowen@cloudera.com> Closes #8307 from srowen/SPARK-9758.
creating pull request for hive test code