[SPARK-40657] Add support for Java classes in Protobuf functions #38286

rangadi · 2022-10-17T17:13:27Z

What changes were proposed in this pull request?

Adds support for compiled Java classes to Protobuf functions. This is tested with Protobuf v3 classes. V2 vs V3 issues will be handled in a separate PR. The main changes in this PR:

Changes to top level API:
- Adds new version that takes just the class name.
- Changes the order of arguments for existing API with descriptor files (messageName and descFilePath are swapped).
Protobuf utils methods to create descriptor from Java class name.
Many unit tests are update to check both versions : (1) with descriptor file and (2) with Java class name.
Maven build updates to generate Java classes to use in tests.
Miscellaneous changes:
- Adds proto to package name in proto files used in tests.
- A few TODO comments about improvements

Why are the changes needed?

Java compiled classes is a common method for users to provide Protobuf definitions.

Does this PR introduce any user-facing change?

No.
This updates interface, but for a new feature in active development.

How was this patch tested?

Unit tests

rangadi · 2022-10-17T17:15:36Z

cc: @SandishKumarHN, @mposdev21, @gengliangwang

rangadi

Added a few inline comments for reviewers.

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/ProtobufDataToCatalyst.scala

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala

...tobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala

connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufFunctionsSuite.scala

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/ProtobufDataToCatalyst.scala

AmplabJenkins · 2022-10-17T19:45:09Z

Can one of the admins verify this patch?

...tobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala

connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufFunctionsSuite.scala

connector/protobuf/pom.xml

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/SchemaConverters.scala

rangadi · 2022-10-18T17:13:37Z

@SandishKumarHN, @mposdev21 PTAL. All the checks pass and I addressed all the comments.
Once you LGTM, I can ping committers.

connector/protobuf/pom.xml

...tobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala

connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufFunctionsSuite.scala

SandishKumarHN

sbt tests are passing now.

rangadi · 2022-10-19T22:06:16Z

@SandishKumarHN pyspark tests are failing could you take a look? I will look at other errors (though they seem unrelated)
https://github.com/rangadi/spark/actions/runs/3283371018/jobs/5408025513

SandishKumarHN · 2022-10-19T22:40:50Z

@SandishKumarHN pyspark tests are failing could you take a look? I will look at other errors (though they seem unrelated) https://github.com/rangadi/spark/actions/runs/3283371018/jobs/5408025513

@rangadi since this feature is swapping the from_protobuf and to_protobuf params, descFilePath, messageName -> messageName, descFilePath. the same change has to be made in python/pyspark/sql/protobuf/functions.py

rangadi · 2022-10-19T22:47:08Z

@SandishKumarHN thanks. That makes sense. I will fix that.

gengliangwang

LGTM

rangadi · 2022-10-20T14:17:45Z

@gengliangwang, @SandishKumarHN some tests related timezone and timestamp type are failing in Run / Build modules: catalyst, hive-thriftserver

They seem unrelated to this PR. Any idea what might be causing this?

gengliangwang · 2022-10-20T17:37:45Z

@rangadi I check the latest run and it fails the python check
https://github.com/rangadi/spark/runs/9009122825
The catalyst tests passed.

amaliujia · 2022-10-20T18:38:08Z

@gengliangwang, @SandishKumarHN some tests related timezone and timestamp type are failing in Run / Build modules: catalyst, hive-thriftserver

They seem unrelated to this PR. Any idea what might be causing this?

I saw this once yestearday. Tried retrigger the test and it passed.

SandishKumarHN · 2022-10-21T03:10:31Z

@gengliangwang, @SandishKumarHN some tests related timezone and timestamp type are failing in Run / Build modules: catalyst, hive-thriftserver

They seem unrelated to this PR. Any idea what might be causing this?

@rangadi test failures are not related to PR. why don't we rerun the failed test? and I see the below test failing consistently. tried merging with the master?

Start checking the generated codes in pyspark-connect.
RUN: /__w/spark/spark/connector/connect/dev/generate_protos.sh /tmp/tmpiuin_p7y
Different files: ['base_pb2.pyi', 'commands_pb2.pyi', 'expressions_pb2.pyi', 'relations_pb2.pyi', 'types_pb2.pyi']
Generated files for pyspark-connect are out of sync! Please run ./connector/connect/dev/generate_protos.sh
Error: Process completed with exit code 255.

rangadi · 2022-10-21T03:32:22Z

tried merging with the master?

I think it merges with master before running the tests.

HeartSaVioR · 2022-10-21T06:44:27Z

I'm merging this since it only fails with Spark client python generation and @gengliangwang approved it.

HeartSaVioR · 2022-10-21T07:35:10Z

Thanks @rangadi for your contribution! I merged into master.

MaxGekk · 2022-10-28T14:02:46Z

...tobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala

@@ -24,6 +24,7 @@ import org.apache.spark.sql.{RandomDataGenerator, Row}
 import org.apache.spark.sql.catalyst.{CatalystTypeConverters, InternalRow, NoopFilters, OrderedFilters, StructFilters}
 import org.apache.spark.sql.catalyst.expressions.{ExpressionEvalHelper, GenericInternalRow, Literal}
 import org.apache.spark.sql.catalyst.util.{ArrayBasedMapData, GenericArrayData, MapData}
+import org.apache.spark.sql.protobuf.protos.CatalystTypes.BytesMsg


I am struggling in generating this file in IDEA:

Is any way to generate all required dependencies?

@MaxGekk did you import it as maven or sbt project? Both should work though.
With maven, 'generating sources' should fix that (in maven 'Tool Window'). I am also seeing similar issue with it, IntelliJ does not seem to run the generate-test-sources phase.

I tried just now and running ../../build/mvn test in protobuf directory fixed this issue.

### What changes were proposed in this pull request? Adds support for compiled Java classes to Protobuf functions. This is tested with Protobuf v3 classes. V2 vs V3 issues will be handled in a separate PR. The main changes in this PR: - Changes to top level API: - Adds new version that takes just the class name. - Changes the order of arguments for existing API with descriptor files (`messageName` and `descFilePath` are swapped). - Protobuf utils methods to create descriptor from Java class name. - Many unit tests are update to check both versions : (1) with descriptor file and (2) with Java class name. - Maven build updates to generate Java classes to use in tests. - Miscellaneous changes: - Adds `proto` to package name in `proto` files used in tests. - A few TODO comments about improvements ### Why are the changes needed? Java compiled classes is a common method for users to provide Protobuf definitions. ### Does this PR introduce _any_ user-facing change? No. This updates interface, but for a new feature in active development. ### How was this patch tested? - Unit tests Closes apache#38286 from rangadi/protobuf-java. Authored-by: Raghu Angadi <raghu.angadi@databricks.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>

rangadi added 6 commits October 14, 2022 18:01

Read support and mvn build support

9d3568a

First set of tests work

64c0cfe

More test fixes

c91f9a0

Write side and more tests are updated

e631eda

All the tests pass

5fc70cc

comment

d16b1cc

github-actions bot added BUILD PROTOBUF SQL labels Oct 17, 2022

rangadi commented Oct 17, 2022

View reviewed changes

SandishKumarHN reviewed Oct 17, 2022

View reviewed changes

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/ProtobufDataToCatalyst.scala Show resolved Hide resolved

Fix scalastyle check & add a comment

c41eb62

mposdev21 reviewed Oct 17, 2022

View reviewed changes

...tobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala Show resolved Hide resolved

connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufFunctionsSuite.scala Outdated Show resolved Hide resolved

minor

6e6ae9b

SandishKumarHN reviewed Oct 17, 2022

View reviewed changes

connector/protobuf/pom.xml Show resolved Hide resolved

SandishKumarHN reviewed Oct 18, 2022

View reviewed changes

connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/SchemaConverters.scala Show resolved Hide resolved

rangadi added 3 commits October 18, 2022 00:53

Genereate protobufs in sbt

9ae8f4a

Merge remote-tracking branch 'apache/master' into protobuf-java

de2c171

minor

40a8170

SandishKumarHN approved these changes Oct 18, 2022

View reviewed changes

connector/protobuf/pom.xml Show resolved Hide resolved

rangadi commented Oct 18, 2022

View reviewed changes

...tobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala Show resolved Hide resolved

SandishKumarHN reviewed Oct 18, 2022

View reviewed changes

connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufFunctionsSuite.scala Outdated Show resolved Hide resolved

Add protobuf to build_and_test

1b0fa0e

github-actions bot added the INFRA label Oct 18, 2022

fix for sbt resources

8471e78

SandishKumarHN approved these changes Oct 18, 2022

View reviewed changes

Merge remote-tracking branch 'apache/master' into protobuf-java

0205a48

merge master, fix typo in test

b600700

update pyspark

fec1052

github-actions bot added CORE PYTHON labels Oct 20, 2022

gengliangwang approved these changes Oct 20, 2022

View reviewed changes

rangadi added 2 commits October 20, 2022 06:59

typo

2d44680

Merge remote-tracking branch 'apache/master' into protobuf-java

66b6ad9

one more python fix

2025031

SandishKumarHN mentioned this pull request Oct 21, 2022

[SPARK-40654][SQL] Protobuf support for Spark - from_protobuf AND to_protobuf #37972

Closed

HeartSaVioR closed this in fd9e576 Oct 21, 2022

rangadi mentioned this pull request Oct 25, 2022

Protobuf generate V2 and V3 protos and extend tests. #38324

Closed

MaxGekk reviewed Oct 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-40657] Add support for Java classes in Protobuf functions #38286

[SPARK-40657] Add support for Java classes in Protobuf functions #38286

rangadi commented Oct 17, 2022

rangadi commented Oct 17, 2022

rangadi left a comment

AmplabJenkins commented Oct 17, 2022

rangadi commented Oct 18, 2022

SandishKumarHN left a comment •

edited

Loading

rangadi commented Oct 19, 2022

SandishKumarHN commented Oct 19, 2022

rangadi commented Oct 19, 2022

gengliangwang left a comment

rangadi commented Oct 20, 2022

gengliangwang commented Oct 20, 2022

amaliujia commented Oct 20, 2022

SandishKumarHN commented Oct 21, 2022

rangadi commented Oct 21, 2022

HeartSaVioR commented Oct 21, 2022

HeartSaVioR commented Oct 21, 2022

MaxGekk Oct 28, 2022

rangadi Oct 28, 2022

[SPARK-40657] Add support for Java classes in Protobuf functions #38286

[SPARK-40657] Add support for Java classes in Protobuf functions #38286

Conversation

rangadi commented Oct 17, 2022

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

rangadi commented Oct 17, 2022

rangadi left a comment

Choose a reason for hiding this comment

AmplabJenkins commented Oct 17, 2022

rangadi commented Oct 18, 2022

SandishKumarHN left a comment • edited Loading

Choose a reason for hiding this comment

rangadi commented Oct 19, 2022

SandishKumarHN commented Oct 19, 2022

rangadi commented Oct 19, 2022

gengliangwang left a comment

Choose a reason for hiding this comment

rangadi commented Oct 20, 2022

gengliangwang commented Oct 20, 2022

amaliujia commented Oct 20, 2022

SandishKumarHN commented Oct 21, 2022

rangadi commented Oct 21, 2022

HeartSaVioR commented Oct 21, 2022

HeartSaVioR commented Oct 21, 2022

MaxGekk Oct 28, 2022

Choose a reason for hiding this comment

rangadi Oct 28, 2022

Choose a reason for hiding this comment

SandishKumarHN left a comment •

edited

Loading