[SPARK-42406][SQL] Fix check for missing required fields of to_protobuf by rangadi · Pull Request #40080 · apache/spark

rangadi · 2023-02-19T05:14:33Z

What changes were proposed in this pull request?

Protobuf serializer (used in to_protobuf()) should error if non-nullable fields (i.e. protobuf required fields) are present in the schema of the catalyst record being converted to a protobuf.

But isNullable() method used for this check returns opposite (see PR comment in the diff). As a result, Serializer incorrectly requires the fields that are optional. This PR fixes this check (see PR comment in the diff).

This also requires corresponding fix for couple of unit tests. In order use a Protobuf message with a required field, Protobuf version 2 file proto2_messages.proto is added.
Two tests are updated to verify missing required fields results in an error.

Why are the changes needed?

This is need to fix a bug where we were incorrectly enforcing a schema check on optional fields rather than on required fields.

Does this PR introduce any user-facing change?

It fixes a bug, and gives more flexibility for user queries.

How was this patch tested?

Updated unit tests

Add a proto2 file to verify required field.

rangadi · 2023-02-19T05:14:59Z

cc: @gengliangwang, @HeartSaVioR, @SandishKumarHN

rangadi

Added inline comment to help with review.

rangadi · 2023-02-19T05:16:40Z

-
- /** Return true if `fieldDescriptor` is optional. */
- private[protobuf] def isNullable(fieldDescriptor: FieldDescriptor): Boolean =
- !fieldDescriptor.isOptional


This is the bug. extra ! returns false for optional field. But optional fields are nullable. This is fixed at line #103 above.

rangadi · 2023-02-19T05:17:06Z

@@ -0,0 +1,8 @@
+


This a descriptor file for proto2_messages.proto

rangadi · 2023-02-19T05:17:30Z

+ * limitations under the License.
+ */
+
+syntax = "proto2";


Protobuf v2 file is added to test required fields.

rangadi · 2023-02-19T05:18:09Z

 int64 bar = 1;
 }

-/* Field boo missing from SQL root, but available in Protobuf root*/


These two are moved to proto2_messages.proto (and renamed)

SandishKumarHN · 2023-02-19T05:19:19Z

@rangadi with this change, we can conclude that spark-protobuf supports proto2 and proto3 versions right?

SandishKumarHN

LGTM

dongjoon-hyun

Hi, @rangadi . Please remove [3.4] from the PR title. Apache Spark community uses a branch tag like [3.4] only when the PR is open for the release branch, branch-3.4. This PR is mis-using it as a target version.

rangadi · 2023-02-20T05:36:08Z

@dongjoon-hyun, done. Remove 3.4 in the summary.

@rangadi with this change, we can conclude that spark-protobuf supports proto2 and proto3 versions right?

Yes. And we always supported Proto2.

Addressed.

dongjoon-hyun · 2023-02-20T05:38:18Z

Thank you for updates, @rangadi .

cc @hvanhovell

rangadi · 2023-02-21T18:12:22Z

@gengliangwang could you take a quick look and merge in to master and 3.4.
Currently, it looks like you are the only one who can merge Protobuf patches. Please suggest someone else also for next time.

gengliangwang · 2023-02-21T18:54:11Z


- // serializing with extra fails if extra field is missing in SQL Schema
+ val foobarSQLType = DataType.fromDDL(
+ "struct<foo string>" // "bar" is missing.


Shall we test catalyst schema which contains bar only and there is a result returned?

Could you describe a bit more?
Do we want to test not having 'foo' field which is optional?

Added a new test at line #69. PTAL.

gengliangwang · 2023-02-21T18:55:07Z

@rangadi can you update the PR description to follow the PR template?

rangadi · 2023-02-21T20:08:29Z

@rangadi can you update the PR description to follow the PR template?

@gengliangwang update the summary to follow the template. Sorry about that.
Also added a new test. PTAL.

gengliangwang · 2023-02-22T01:33:41Z

Thanks, merging to master/3.4

### What changes were proposed in this pull request? Protobuf serializer (used in `to_protobuf()`) should error if non-nullable fields (i.e. protobuf `required` fields) are present in the schema of the catalyst record being converted to a protobuf. But `isNullable()` method used for this check returns opposite (see PR comment in the diff). As a result, Serializer incorrectly requires the fields that are optional. This PR fixes this check (see PR comment in the diff). This also requires corresponding fix for couple of unit tests. In order use a Protobuf message with a `required` field, Protobuf version 2 file `proto2_messages.proto` is added. Two tests are updated to verify missing required fields results in an error. ### Why are the changes needed? This is need to fix a bug where we were incorrectly enforcing a schema check on optional fields rather than on required fields. ### Does this PR introduce _any_ user-facing change? It fixes a bug, and gives more flexibility for user queries. ### How was this patch tested? - Updated unit tests Closes#40080 from rangadi/fix-required-field-check. Authored-by: Raghu Angadi <raghu.angadi@databricks.com> Signed-off-by: Gengliang Wang <gengliang@apache.org> (cherry picked from commit fb56477) Signed-off-by: Gengliang Wang <gengliang@apache.org>

### What changes were proposed in this pull request? Protobuf serializer (used in `to_protobuf()`) should error if non-nullable fields (i.e. protobuf `required` fields) are present in the schema of the catalyst record being converted to a protobuf. But `isNullable()` method used for this check returns opposite (see PR comment in the diff). As a result, Serializer incorrectly requires the fields that are optional. This PR fixes this check (see PR comment in the diff). This also requires corresponding fix for couple of unit tests. In order use a Protobuf message with a `required` field, Protobuf version 2 file `proto2_messages.proto` is added. Two tests are updated to verify missing required fields results in an error. ### Why are the changes needed? This is need to fix a bug where we were incorrectly enforcing a schema check on optional fields rather than on required fields. ### Does this PR introduce _any_ user-facing change? It fixes a bug, and gives more flexibility for user queries. ### How was this patch tested? - Updated unit tests Closesapache#40080 from rangadi/fix-required-field-check. Authored-by: Raghu Angadi <raghu.angadi@databricks.com> Signed-off-by: Gengliang Wang <gengliang@apache.org> (cherry picked from commit fb56477) Signed-off-by: Gengliang Wang <gengliang@apache.org>

Fix check for required fields.
b37f9a8
Add a proto2 file to verify required field.

github-actionsBot added PROTOBUF SQL labels Feb 19, 2023

rangadi commented Feb 19, 2023
View reviewed changes

SandishKumarHN approved these changes Feb 19, 2023
View reviewed changes

rangadi changed the title ~~[SPARK-42406] Fix check for missing required fields.~~ [SPARK-42406][3.4] Fix check for missing required fields. Feb 19, 2023

dongjoon-hyun previously requested changes Feb 20, 2023
View reviewed changes

rangadi changed the title ~~[SPARK-42406][3.4] Fix check for missing required fields.~~ [SPARK-42406]Fix check for missing required fields. Feb 20, 2023

rangadi requested a review from dongjoon-hyun February 20, 2023 05:36

Try fix for scala style
92d6d3a

gengliangwang reviewed Feb 21, 2023
View reviewed changes

Add a test to verify optional field can be missing in input schema
5ca177b

gengliangwang changed the title ~~[SPARK-42406]Fix check for missing required fields.~~ [SPARK-42406][SQL] Fix check for missing required fields of to_protobufFeb 21, 2023

gengliangwang approved these changes Feb 21, 2023
View reviewed changes

HyukjinKwon approved these changes Feb 22, 2023
View reviewed changes

gengliangwang closed this in fb56477Feb 22, 2023

Uh oh!

Conversation

rangadi commented Feb 19, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

rangadi commented Feb 19, 2023

Uh oh!

rangadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SandishKumarHN commented Feb 19, 2023

Uh oh!

SandishKumarHN left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

rangadi commented Feb 20, 2023

Uh oh!

dongjoon-hyun commented Feb 20, 2023

Uh oh!

rangadi commented Feb 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented Feb 21, 2023

Uh oh!

rangadi commented Feb 21, 2023

Uh oh!

gengliangwang commented Feb 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rangadi commented Feb 19, 2023•
edited
Loading