Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-33084][CORE][SQL] Add jar support ivy path #29966
[SPARK-33084][CORE][SQL] Add jar support ivy path #29966
Changes from 41 commits
afaf7bd
51daf9a
3579de0
d6e8caf
169e1f8
0e589ec
b3e3211
9161340
0e3c1ec
300ca56
63e877b
733e62c
b60ba1e
883b9d3
208afc2
ba9ea29
10b3737
7f878c2
d2c1950
2200076
5a9cc30
875d8a7
e921245
614a865
8c5cb7c
f460974
1f7dc01
050c410
ff611a6
03aca3b
653b919
6e48275
bdc5035
9c22882
9c88f8d
8220e5a
49ac62c
b69a62e
273a5ac
ebe1c9c
6034fb2
e22e398
afea73f
13000f2
bce3d40
d53f302
57c351d
8c53b83
4048c5b
aa53482
2ffb431
8c18cdf
6bd41cd
fbc236c
90491d5
75ff3ce
4c44dae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@AngersZhuuuu, out of curiosity, is the Ivy URI the standard form documented somewhere? or something specific to Spark that you came up with?
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.
From hive https://issues.apache.org/jira/browse/HIVE-9664, since it download jar use
ivy
then use schema asivy
? I think this useful for a lot of companies that have standard package management, so I implemented it in SparkThere 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.
What if two added jars have the same dependency with different versions? e.g.,
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.
Done
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.
Could you add tests to check if this warning message is shown only once by using
LogAppender
?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.
Sure
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.
Nit: Add Scaladoc for this new parameter
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.
Done and
transitive
moved to front ofexclusions
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.
What happens if we remove this default value? If it doesn't make many changes, let's remove this default value.
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 remove this default value and then add this in place where use this method.
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.
Remove now and you can see the change to judge where need to revert back.
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.
hive has the same behaviour with this?
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.
No, but we can have this warning
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.
If so, what's the hive behaviour? it will throw an exception instead selecting the last one?
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.
Select the last one
https://github.com/apache/hive/blob/aed7c86cdd59f0b2a4979633fbd191d451f2fd75/ql/src/java/org/apache/hadoop/hive/ql/util/DependencyResolver.java#L121-L127
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.
Ah, ok.
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.
What if an invalid param is given in hive, e.g.,
invalidParam=xxxx
? It is just ignored? Could you add tests?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.
Done
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.
Seems like we should at least warn on invalid param?