[SPARK-41015][SQL][PROTOBUF] UnitTest null check for data generator #38515

SandishKumarHN · 2022-11-05T00:12:09Z

What changes were proposed in this pull request?

null check for data generator after type conversion.

Why are the changes needed?

To fix a test failure.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

I have tested all the random numbers manually, current unit tests.

SandishKumarHN · 2022-11-05T00:12:53Z

@rangadi Because some random numbers do not get converted to catalyst type, a null check for the data generator is required.
generator() returns an empty byte array for seed 399 and 776

AmplabJenkins · 2022-11-05T12:50:11Z

Can one of the admins verify this patch?

rangadi · 2022-11-05T16:37:50Z

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

+        data != null &&
+        (data.asInstanceOf[Row].get(0) == defaultValue ||
+          (data.asInstanceOf[Row].get(0).isInstanceOf[Array[Byte]] &&
+          data.asInstanceOf[Row].get(0).asInstanceOf[Array[Byte]].isEmpty)))


Isn't ByteString.empty().toByteArray at line 122 already empty value?

rangadi · 2022-11-05T17:28:22Z

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

-        data = generator()                                  // from_protobuf() returns null in v3.
+      // Do not use default values, since from_protobuf() returns null in v3.
+      while (
+        data != null &&


This should just be data == null || data.asInstanceOf[Row].get(0) == defaultValue?

I meant, we don't need the array check.

@rangadi

data.asInstanceOf[Row].get(0) == ByteString.empty().toByteArray

data.asInstanceOf[Row].get(0) == Array.emptyByteArray

data.asInstanceOf[Row].get(0) == ByteString.EMPTY.toByteArray

data.asInstanceOf[Row].get(0) == "".getBytes

data.asInstanceOf[Row].get(0).isInstanceOf[Array[Byte]] && data.asInstanceOf[Row].get(0).asInstanceOf[Array[Byte]].isEmpty

Except for (5), none of them worked. I'm printing under the conditions listed above.

I see. Equals does not check the content on array.
Optional:
We could recduce data.asInstanceOf calls with val data = generator().asInstanceOf[Row].
Also could replace

data.asInstanceOf[Row].get(0).isInstanceOf[Array[Byte]]
with
dt == BinaryType

SandishKumarHN · 2022-11-05T19:03:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

@@ -3330,7 +3330,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
  def descrioptorParseError(descFilePath: String, cause: Throwable): Throwable = {
    new AnalysisException(
      errorClass = "CANNOT_PARSE_PROTOBUF_DESCRIPTOR",
-      messageParameters = Map.empty("descFilePath" -> descFilePath),
+      messageParameters = Map("descFilePath" -> descFilePath),


@MaxGekk fixing this one #38344 (comment) here

MaxGekk · 2022-11-06T06:12:12Z

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

+
+    checkError(
+      exception = e,
+      errorClass = "CANNOT_PARSE_PROTOBUF_DESCRIPTOR",


@SandishKumarHN Thank you for the added test.

MaxGekk · 2022-11-06T06:12:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

@@ -3344,7 +3344,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
  def failedParsingDescriptorError(descFilePath: String, cause: Throwable): Throwable = {
    new AnalysisException(
      errorClass = "CANNOT_CONSTRUCT_PROTOBUF_DESCRIPTOR",


Could you add a test for this error class.

@MaxGekk added

rangadi · 2022-11-07T18:00:25Z

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

-        data = generator()                                  // from_protobuf() returns null in v3.
+      // Do not use default values, since from_protobuf() returns null in v3.
+      while (
+        data != null &&


I see. Equals does not check the content on array.
Optional:
We could recduce data.asInstanceOf calls with val data = generator().asInstanceOf[Row].
Also could replace

data.asInstanceOf[Row].get(0).isInstanceOf[Array[Byte]]
with
dt == BinaryType

rangadi · 2022-11-07T18:04:00Z

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

+  test("raise cannot construct protobuf descriptor error") {
+    val testFileDesc = testFile("basicmessage_noimports.desc").replace("file:/", "/")
+    val e = intercept[AnalysisException] {
+      ProtobufUtils.parseFileDescriptorSet(testFileDesc)


Thanks for adding the test.
Do we need to invoke this? Any query would raise this, right?

@rangadi Yes, but the query would catch this error and throw a different error right?

Users don't call this. Analysis exception thrown to the user if they try to use this.

@rangadi Check it out once again I've made a few minor changes.

rangadi · 2022-11-07T18:06:00Z

connector/protobuf/src/test/resources/protobuf/basicmessage_noimports.desc

@@ -0,0 +1,18 @@
+


Where is this used?
Did you mean to another test?

@rangadi yes, it is used in the unit test above, how else can we raise the CANNOT_CONSTRUCT_PROTOBUF_DESCRIPTOR exception?

Right. I didn't see the second test carefully.

rangadi · 2022-11-07T18:49:09Z

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

+  test("raise cannot construct protobuf descriptor error") {
+    val basicMessageDesc = ProtobufUtils.buildDescriptor(testFileDesc, "BasicMessage")
+
+    val basicMessage = DynamicMessage


Btw, you don't need the message. Could use empty byte array while initializing df.

correct, fixed.

rangadi · 2022-11-07T19:05:11Z

@MaxGekk, please merge once this this looks good to you.

MaxGekk · 2022-11-07T20:28:08Z

@SandishKumarHN @rangadi I will review and merge it (if everything is ok, and test passed) tomorrow.

MaxGekk · 2022-11-08T10:18:11Z

+1, LGTM. Merging to master.
Thank you, @SandishKumarHN and @rangadi for review.

### What changes were proposed in this pull request? null check for data generator after type conversion. ### Why are the changes needed? To fix a test failure. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? I have tested all the random numbers manually, current unit tests. Closes apache#38515 from SandishKumarHN/SPARK-41015-UTests. Authored-by: SandishKumarHN <sanysandish@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

github-actions bot added PROTOBUF SQL labels Nov 5, 2022

null check for data generator

bcc333f

SandishKumarHN force-pushed the SPARK-41015-UTests branch from 69be828 to bcc333f Compare November 5, 2022 05:36

rangadi reviewed Nov 5, 2022

View reviewed changes

bug fix Map.empty to Map with Utest

d4e26b9

SandishKumarHN commented Nov 5, 2022

View reviewed changes

MaxGekk requested changes Nov 6, 2022

View reviewed changes

review changes Utest for cannt construct proto desc

f11b04d

rangadi reviewed Nov 7, 2022

View reviewed changes

review changes nit, utests changes

c9abb2d

rangadi approved these changes Nov 7, 2022

View reviewed changes

review changes utest, empty protobuf

44e9209

MaxGekk approved these changes Nov 8, 2022

View reviewed changes

MaxGekk closed this in 75643f4 Nov 8, 2022

MaxGekk mentioned this pull request Oct 24, 2023

[SPARK-45640][SQL][TESTS] Fix flaky ProtobufCatalystDataConversionSuite #43493

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-41015][SQL][PROTOBUF] UnitTest null check for data generator #38515

[SPARK-41015][SQL][PROTOBUF] UnitTest null check for data generator #38515

SandishKumarHN commented Nov 5, 2022 •

edited by MaxGekk

Loading

SandishKumarHN commented Nov 5, 2022 •

edited

Loading

AmplabJenkins commented Nov 5, 2022

rangadi Nov 5, 2022

rangadi Nov 5, 2022

rangadi Nov 5, 2022

SandishKumarHN Nov 5, 2022

rangadi Nov 7, 2022

SandishKumarHN Nov 5, 2022

MaxGekk Nov 6, 2022

MaxGekk Nov 6, 2022

SandishKumarHN Nov 6, 2022

rangadi Nov 7, 2022

rangadi Nov 7, 2022

SandishKumarHN Nov 7, 2022

rangadi Nov 7, 2022

SandishKumarHN Nov 7, 2022

rangadi Nov 7, 2022

SandishKumarHN Nov 7, 2022

rangadi Nov 7, 2022

rangadi Nov 7, 2022

SandishKumarHN Nov 7, 2022

rangadi commented Nov 7, 2022

MaxGekk commented Nov 7, 2022

MaxGekk commented Nov 8, 2022

[SPARK-41015][SQL][PROTOBUF] UnitTest null check for data generator #38515

[SPARK-41015][SQL][PROTOBUF] UnitTest null check for data generator #38515

Conversation

SandishKumarHN commented Nov 5, 2022 • edited by MaxGekk Loading

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?

SandishKumarHN commented Nov 5, 2022 • edited Loading

AmplabJenkins commented Nov 5, 2022

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

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

rangadi commented Nov 7, 2022

MaxGekk commented Nov 7, 2022

MaxGekk commented Nov 8, 2022

SandishKumarHN commented Nov 5, 2022 •

edited by MaxGekk

Loading

SandishKumarHN commented Nov 5, 2022 •

edited

Loading