-
Couldn't load subscription status.
- Fork 28.9k
[SPARK-27831][FOLLOW-UP][SQL][TEST] Should not use maven to add Hive test jars #25690
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
Conversation
…ncy" This reverts commit d53b61c
|
Test build #110166 has finished for PR 25690 at commit
|
|
retest this please |
|
Test build #110178 has finished for PR 25690 at commit
|
|
|
||
| test("Commands using SerDe provided in --jars") { | ||
| val jarFile = HiveTestUtils.getHiveHcatalogCoreJar.getCanonicalPath | ||
| val jarFile = "../hive/src/test/resources/" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding these JARs to the source tree has some LICENSE and NOTICE implications. The Hive NOTICE text from these JARs would have to go in NOTICE (and probably NOTICE-binary as we published test JARs). They'd have to be listed in LICENSE and LICENSE-binary too.
This is possible, but is it equally possible to just download these like we do with other Hive jars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, this doesn't look like a complete revert of that commit. In this case, please be clear on that.
For example, this PR didn't revert the following from hive-thriftserver/pom.xml.
|
IMO, if you still depends on a some code in the previous commit, this is just a clean-up follow-up. If you use |
|
thank you @srowen @dongjoon-hyun I see. Will update it later. |
|
(Let's update and elaborate the PR description as well) |
|
Test build #110216 has finished for PR 25690 at commit
|
|
Test build #110223 has finished for PR 25690 at commit
|
|
retest this please |
|
Test build #110228 has finished for PR 25690 at commit
|
|
I think the approach is valid, although I know there's a strong preference to avoid putting JAR files in the source tree. This is possibly a valid case for doing so, as it's test code and kind of requires the JAR file to exist locally. That said, I wonder if we can just reuse the code that is used elsewhere to download Hive JARs here? it might need a little refactoring, but it already exists, to use the ASF mirrors, etc. |
|
Test build #110276 has finished for PR 25690 at commit
|
|
Test build #110277 has finished for PR 25690 at commit
|
|
Test build #110279 has finished for PR 25690 at commit
|
|
retest this please |
|
Test build #111294 has started for PR 25690 at commit |
|
retest this please |
|
Test build #111301 has finished for PR 25690 at commit
|
|
retest this please |
|
Test build #111314 has finished for PR 25690 at commit
|
|
@srowen @HyukjinKwon @dongjoon-hyun The test failure fixed by #25775. Which one should be merged first? |
|
Test build #4886 has started for PR 25690 at commit |
|
Thank you @srowen |
| } | ||
|
|
||
| private[hive] object HiveTestJars { | ||
| private val repository = SQLConf.ADDITIONAL_REMOTE_REPOSITORIES.defaultValueString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also verify that the default value is valid.
|
Test build #111486 has finished for PR 25690 at commit
|
|
retest this please |
|
Test build #111502 has finished for PR 25690 at commit
|
|
retest this please |
|
Test build #111527 has finished for PR 25690 at commit
|
|
@srowen @dongjoon-hyun @HyukjinKwon Do you have any comment? |
|
Thank you all. |
|
Merged to master. |
What changes were proposed in this pull request?
This PR moves Hive test jars(
hive-contrib-*.jarandhive-hcatalog-core-*.jar) from maven dependency to local file.Why are the changes needed?
--jarscan't be tested sincehive-contrib-*.jarandhive-hcatalog-core-*.jarare already in classpath.Does this PR introduce any user-facing change?
No.
How was this patch tested?
manual test