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

rangadi · 2022-10-20T18:33:41Z

This is a temporary PR on, made on top of Java-support PR #37738.

Outline:

Generates descriptor files and java classes for both V2 and V3
Updates the unit tests to check all variations (V2 & V3 x Java, decriptor file).
Adds a test with default values and required fields for V2.
Maven build is updated. But not clear how do the same in SBT.

rangadi · 2022-10-20T18:34:03Z

FYI: @mposdev21, @SandishKumarHN

mposdev21

Overall, looks good to me except for a few things I noted.

mposdev21 · 2022-10-21T00:40:52Z

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

+    // TODO: Avoid carrying the file name. Read the contents of descriptor file only once
+    //       at the start. Rest of the runs should reuse the buffer. Otherwise, it could
+    //       cause inconsistencies if the file contents are changed the user after a few days.
+    //       Same for the write side in [[CatalystDataToProtobuf]].


I am not sure I understand the comment completely. When buildDescriptor is called, the contents of the file is read and a descriptor is created. The first time messageDescriptor is evaluated, the file contents are read. Could you clarify ?

I had added this in my Java class pr #38286.
The file contents are read on executors too when the ProtobufDataToCatalyst() class is initialized there. Notice @transient private lazy val messageDescriptor in the class.

mposdev21 · 2022-10-21T00:43:12Z

connector/protobuf/src/test/protobuf/v2/catalyst_types.proto

+ */
+// protoc --java_out=connector/protobuf/src/test/resources/protobuf/ connector/protobuf/src/test/resources/protobuf/catalyst_types.proto
+// protoc --descriptor_set_out=connector/protobuf/src/test/resources/protobuf/catalyst_types.desc --java_out=connector/protobuf/src/test/resources/protobuf/org/apache/spark/sql/protobuf/ connector/protobuf/src/test/resources/protobuf/catalyst_types.proto
+


Do we still want to keep these comments ?

Removed. Also added pyspark_test.proto to this directory.

mposdev21 · 2022-10-21T00:48:43Z

connector/protobuf/src/test/protobuf/v2/functions_suite.proto

+  required int32 id = 2;
+  optional int32 api_quota = 3 [default = 100]; // Default 100 qps.
+  optional string location = 4 [default = "Unknown"];
+}


Can we also add Enum ? I tested with a default value which seemed to work. But then the following text in the spec:

"For enums, the default value is the first value listed in the enum's type definition."

If I interpret this to mean that if we don't specify a default value and the field is left uninitialized, it should pick the. first default value. That interpretation does not work because hasDefaultValue fails and hence the test would fail if you assert that it has the first value.

mposdev21 · 2022-10-21T00:49:48Z

connector/protobuf/src/test/protobuf/v3/functions_suite.proto

@@ -119,7 +116,7 @@ message SimpleMessageEnum {
  string key = 1;
  string value = 2;
  enum NestedEnum {
-    ESTED_NOTHING = 0;
+    ESTED_NOTHING = 0; // TODO: Fix the name.


Should we fix this in v2 and v3 ? Seems like a small change.

mposdev21 · 2022-10-21T00:51:52Z

connector/protobuf/pom.xml

+            </configuration>
+          </execution>
+        </executions>
+      </plugin>


It looks like we still have the desc files under resources/protobuf ?

mposdev21 · 2022-10-21T01:07:43Z

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

+      .addRfloatValue(10903.0f)
+      .addRfloatValue(10902.0f)
+      .addRnestedEnum(NestedEnum.ESTED_NOTHING)
+      .addRnestedEnum(NestedEnum.NESTED_FIRST)
      .build()



Here SimpleMessageRepeated is a v3 class which is used to build the protobuf message. It might make sense to add one test case to use v2.SimpleMessageRepeated (or anything else) to build the message and do the test.

mposdev21 · 2022-10-21T01:10:03Z

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

-    val repeatedMessageDesc = ProtobufUtils.buildDescriptor(testFileDesc, "RepeatedMessage")
-    val basicMessageDesc = ProtobufUtils.buildDescriptor(testFileDesc, "BasicMessage")
+    val repeatedMessageDesc = ProtobufUtils.buildDescriptor(descPathV3, "RepeatedMessage")
+    val basicMessageDesc = ProtobufUtils.buildDescriptor(descPathV3, "BasicMessage")


I guess. you are picking the descriptors (v2 or v3) for building the message randomly here. Is that correct ?

LuciferYang · 2022-10-21T06:06:59Z

A newbie question: why we need support protobuf v2?

AmplabJenkins · 2022-10-21T15:03:38Z

Can one of the admins verify this patch?

rangadi · 2022-10-25T06:50:24Z

A newbie question: why we need support protobuf v2?

@LuciferYang many users still use v2 protobufs. It is not always easy to migrate the protobufs to v3 or even necessary. Note that v2 protobufs are still genrated with protoc 3.x.x. The v2 protos used in tests are built with 3.2.1. The syntax version (v2 or v3) is different from version used to parse and generate Java classes or descriptor sets.

github-actions · 2023-02-12T00:21:53Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

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

rangadi marked this pull request as draft October 20, 2022 18:36

mposdev21 reviewed Oct 21, 2022

View reviewed changes

Rebased to master and reapplied the diffs in single commit

9864c63

rangadi force-pushed the protoc-desc branch from a5eaedc to 9864c63 Compare October 25, 2022 06:31

rangadi added 2 commits October 24, 2022 23:40

update proto files

e33c167

add pyspark_test.proto to v2

a68a1bc

rangadi changed the title ~~[Temp] Protobuf generate V2 and V3 protos and extend tests.~~ Protobuf generate V2 and V3 protos and extend tests. Nov 3, 2022

github-actions bot added the Stale label Feb 12, 2023

github-actions bot closed this Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rangadi commented Oct 20, 2022

rangadi commented Oct 20, 2022

mposdev21 left a comment

mposdev21 Oct 21, 2022

rangadi Oct 25, 2022

mposdev21 Oct 21, 2022

rangadi Oct 25, 2022

mposdev21 Oct 21, 2022

mposdev21 Oct 21, 2022

mposdev21 Oct 21, 2022

rangadi Oct 25, 2022

mposdev21 Oct 21, 2022

mposdev21 Oct 21, 2022

rangadi Oct 25, 2022

LuciferYang commented Oct 21, 2022

AmplabJenkins commented Oct 21, 2022

rangadi commented Oct 25, 2022

github-actions bot commented Feb 12, 2023

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

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

Conversation

rangadi commented Oct 20, 2022

rangadi commented Oct 20, 2022

mposdev21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuciferYang commented Oct 21, 2022

AmplabJenkins commented Oct 21, 2022

rangadi commented Oct 25, 2022

github-actions bot commented Feb 12, 2023