Skip to content

Conversation

@HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR makes the following dependencies shaded:

com.google.android:annotations
com.google.api.grpc:proto-google-common-proto
io.perfmark:perfmark-api
org.codehaus.mojo:animal-sniffer-annotations
com.google.errorprone:error_prone_annotations
com.google.j2objc:j2objc-annotations

Before #38109, it worked because related dependences pulled together but now we don't as Spark Connect would be a single jar. This issue has existed from the very first place.

Why are the changes needed?

Otherwise, the tests fails if you build Spark Connect with Maven. SBT does not have the issue because it does the assemply with all dependencies.

Does this PR introduce any user-facing change?

No, the codes are not released out yet.

How was this patch tested?

Manually tested via Maven:

./build/mvn clean package
./python/run-tests --testnames 'pyspark.sql.tests.test_connect_basic'

@HyukjinKwon
Copy link
Member Author

cc @LuciferYang FYI

@HyukjinKwon
Copy link
Member Author

cc @amaliujia too FYI

@HyukjinKwon
Copy link
Member Author

@amaliujia
Copy link
Contributor

question:

how do you decide which package needs a shading?

@HyukjinKwon
Copy link
Member Author

Oh, these were jars missing, andI manually added them (see also #38109 (comment))

@HyukjinKwon
Copy link
Member Author

Probably there should be a better way I believe but I am going to merge it first to fix the broken test cases with Maven.

@HyukjinKwon
Copy link
Member Author

Merged to master.

</relocation>

<relocation>
<pattern>com.google.android</pattern>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HyukjinKwon Sorry for the late reply

The relocation rule may be incorrect

I think it should be

<pattern>android.annotation</pattern>
<shadedPattern>${spark.shade.packageName}.connect.android.annotation</shadedPattern>

and unzip the assembly jar, the contents are as follows:

ls

META-INF	android		com		google		grpc		org		spark

Not all contents are placed in ${spark.shade.packageName}.connect, which may require further check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey sure, please go ahead with a followup. I just rushed to merge to fix up the broken tests with Maven :-).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK~ Maybe do this tomorrow, a little tired today

@HyukjinKwon HyukjinKwon deleted the SPARK-40677 branch January 15, 2024 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants