Skip to content

[SPARK-43646][CONNECT][TESTS] Make both SBT and Maven use spark-proto uber jar to test the connect module #42236

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.

Sign up for GitHub

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

Closed
LuciferYang wants to merge 15 commits into from
Closed

[SPARK-43646][CONNECT][TESTS] Make both SBT and Maven use spark-proto uber jar to test the connect module #42236

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bd88c72
init commit
LuciferYang Jul 31, 2023
55c975a
add checkstyle rule
LuciferYang Jul 31, 2023
93d64dd
add comments
LuciferYang Jul 31, 2023
eaa01e5
add comments
LuciferYang Jul 31, 2023
d9411f9
add TODO
LuciferYang Jul 31, 2023
dc13f71
Merge branch 'upmaster' into protobuf-test
LuciferYang Aug 1, 2023
ac92862
rename
LuciferYang Aug 1, 2023
113251b
format
LuciferYang Aug 1, 2023
2563eb4
remove proto file
LuciferYang Aug 4, 2023
8ac1c79
add comments
LuciferYang Aug 9, 2023
9e0333d
Merge branch 'upmaster' into protobuf-test
LuciferYang Aug 18, 2023
4ba45e0
auto gen
LuciferYang Aug 24, 2023
f14b9ab
Merge branch 'upmaster' into protobuf-test
LuciferYang Aug 24, 2023
db17e5b
Merge branch 'upmaster' into protobuf-test
LuciferYang Aug 27, 2023
5b7f230
Merge branch 'upmaster' into protobuf-test
LuciferYang Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ...ctor/connect/client/jvm/src/test/scala/org/apache/spark/sql/PlanGenerationTestSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3233,14 +3233,15 @@ class PlanGenerationTestSuite
"connect/common/src/test/resources/protobuf-tests/common.desc"

test("from_protobuf messageClassName") {

@LuciferYang LuciferYang Aug 1, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we deem this plan too intricate, we could delete or ignore test cases from_protobuf messageClassName and from_protobuf messageClassName options to temporarily abandon this testing scenario

@rangadi rangadi Aug 8, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is great! Thank you.

binary.select(pbFn.from_protobuf(fn.col("bytes"), classOf[StorageLevel].getName))
binary.select(
pbFn.from_protobuf(fn.col("bytes"), "org.apache.spark.sql.protobuf.protos.TestProtoObj"))

@rangadi rangadi Aug 8, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In order to use this sql function, users have no choice but to relocate the content of the Java PB description files used in their business according to Spark's project rules(replcace all com.google.protobuf.
toorg.sparkproject.spark_protobuf.protobuf.

Moving the discussion about "why do users need to shade their java classes?" here in a comment thread. cc: @LuciferYang, @advancedxy, @HyukjinKwon.

There is no other option. Spark at runtime includes unshaded Protobuf library (2.5.x) in its class path. This is was as of a few months back. Don't know if that changed. Its been this way for years. There is no way to support protobuf Java classes for current versions of Protobuf library without shading.

I agree it is not convenient for the customers. That is why descriptor file is documented as the primary API (both for Scala and python).
We will soon support Python classes that will help Python customers.
FWIW I have a small gitbuf project to help customers create shaded Java classes : https://github.com/rangadi/shaded-protobuf-classes

@LuciferYang LuciferYang Aug 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After #41153, protobuf 2.5 is no longer in classpath :)

@LuciferYang LuciferYang Aug 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rangadi I'm sorry that I neglected the historical context of adding these functions. Spark 3.5 no longer depends on Protobuf 2.5, and these functions are currently marked as @Experimental. In this situation, is it possible for us to make some changes?

@rangadi rangadi Aug 8, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That requires removing shading of protobuf classes. It needs more discussion. In general, including non-shaded jars is problematic (that is how original protobuf 2.5 version jar remained unchanged for many years).

@advancedxy advancedxy Aug 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is no other option. Spark at runtime includes unshaded Protobuf library (2.5.x) in its class path.

Yeah, I noticed there's protobu-2.5 jar in the spark-3.4 client tar.

For Spark 3.5, do you think it's better to publishing both shading and unshaded version of spark-protobuf jar? If both published, end customers could choose which jar to use and we can documented it clearly.

}

test("from_protobuf messageClassName options") {
binary.select(
pbFn.from_protobuf(
fn.col("bytes"),
classOf[StorageLevel].getName,
"org.apache.spark.sql.protobuf.protos.TestProtoObj",
Map("recursive.fields.max.depth" -> "2").asJava))
}

Expand Down
2 changes: 1 addition & 1 deletion ...mon/src/test/resources/query-tests/explain-results/from_protobuf_messageClassName.explain
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
Project [from_protobuf(bytes#0, org.apache.spark.connect.proto.StorageLevel, None) AS from_protobuf(bytes)#0]
Project [from_protobuf(bytes#0, org.apache.spark.sql.protobuf.protos.TestProtoObj, None) AS from_protobuf(bytes)#0]
+- LocalRelation <empty>, [id#0L, bytes#0]
2 changes: 1 addition & 1 deletion ...test/resources/query-tests/explain-results/from_protobuf_messageClassName_options.explain
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
Project [from_protobuf(bytes#0, org.apache.spark.connect.proto.StorageLevel, None, (recursive.fields.max.depth,2)) AS from_protobuf(bytes)#0]
Project [from_protobuf(bytes#0, org.apache.spark.sql.protobuf.protos.TestProtoObj, None, (recursive.fields.max.depth,2)) AS from_protobuf(bytes)#0]
+- LocalRelation <empty>, [id#0L, bytes#0]
2 changes: 1 addition & 1 deletion ...connect/common/src/test/resources/query-tests/queries/from_protobuf_messageClassName.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
}
}, {
"literal": {
"string": "org.apache.spark.connect.proto.StorageLevel"
"string": "org.apache.spark.sql.protobuf.protos.TestProtoObj"
}
}]
}
Expand Down
Binary file modified ...ct/common/src/test/resources/query-tests/queries/from_protobuf_messageClassName.proto.bin
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion ...common/src/test/resources/query-tests/queries/from_protobuf_messageClassName_options.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
}
}, {
"literal": {
"string": "org.apache.spark.connect.proto.StorageLevel"
"string": "org.apache.spark.sql.protobuf.protos.TestProtoObj"
}
}, {
"unresolvedFunction": {
Expand Down
Binary file modified ...n/src/test/resources/query-tests/queries/from_protobuf_messageClassName_options.proto.bin
View file
Open in desktop
Binary file not shown.
57 changes: 57 additions & 0 deletions connector/connect/server/src/test/java/org/apache/spark/sql/protobuf/protos/TestProto.java
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading