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

panbingkun · 2023-10-24T02:17:51Z

What changes were proposed in this pull request?

The pr aims to fix flaky ProtobufCatalystDataConversionSuite.

As can be seen from the GA test below
https://github.com/panbingkun/spark/actions/runs/6612141762/job/17982780917

When data.get(0) is null, data.get(0).asInstanceOf[Array[Byte]].isEmpty will be thrown java.lang.NullPointerException.

Why are the changes needed?

Fix flaky ProtobufCatalystDataConversionSuite.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA.
Manually test:

./build/sbt "protobuf/testOnly org.apache.spark.sql.protobuf.ProtobufCatalystDataConversionSuite"

Was this patch authored or co-authored using generative AI tooling?

No.

panbingkun · 2023-10-24T02:18:44Z

cc @LuciferYang @HyukjinKwon

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

MaxGekk · 2023-10-24T11:21:12Z

@SandishKumarHN Could you take a look at the PR, please. Seems similar changes as in #38515 . Also cc @rangadi

SandishKumarHN

LGTM

rangadi · 2023-10-24T19:19:37Z

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

@@ -138,6 +138,7 @@ class ProtobufCatalystDataConversionSuite
        data != null &&
        (data.get(0) == defaultValue ||
          (dt.fields(0).dataType == BinaryType &&
+            data.get(0) != null &&


We should allow null value for the field. I.e. change this to:

(data.get(0) == null || data.get(0).asInstanceOf[Array[Byte]].isEmpty)

I will test on my side to ensure this test stays stable with range of seed values.
cc: @HeartSaVioR

Ignore this suggestion. data.get(0) != null is correct. It allows testing null.

HeartSaVioR · 2023-10-24T20:29:25Z

Please run the test suite with all combination for available seeds and available testing types and make sure all of them are passing. Let's fix all the broken-but-not-caught cases.

rangadi · 2023-10-24T23:01:39Z

@HeartSaVioR I have run this fix with see values from 0 to 10000. All were successful. Could you merge this?

HeartSaVioR

+1

HeartSaVioR · 2023-10-24T23:14:52Z

@rangadi
I just indicated that the test suite with seed existed from 3.4. I guess you've also fixed some of the test issue in other PR. Should we port back that PR as well?

rangadi · 2023-10-24T23:31:13Z

@HeartSaVioR yes, backporting will be good.

HyukjinKwon · 2023-10-24T23:43:32Z

Merged to master

rangadi · 2023-10-24T23:50:32Z

Thanks @HyukjinKwon

HeartSaVioR · 2023-10-24T23:55:25Z

This has to go to 3.5/3.4 as well.

@panbingkun Would you mind filing PRs for backporting this to 3.5 and 3.4? Separate PRs would be preferred.
Please also include the change in this PR #43424 - only need to incorporate the change in connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala

Thanks in advance!

panbingkun · 2023-10-25T01:41:19Z

Would you mind filing PRs for backporting this to 3.5 and 3.4? Separate PRs would be preferred. Please also include the change in this PR #43424 - only need to incorporate the change in connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala

Thanks in advance!

Let me do it right away.
Thank for all reviewing and help testing @rangadi @HeartSaVioR @MaxGekk @HyukjinKwon @LuciferYang

panbingkun · 2023-10-25T02:32:11Z

This has to go to 3.5/3.4 as well.

@panbingkun Would you mind filing PRs for backporting this to 3.5 and 3.4? Separate PRs would be preferred. Please also include the change in this PR #43424 - only need to incorporate the change in connector/protobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala

Thanks in advance!

Done.
Branch-3.4: #43520
Branch-3.5: #43521

…tDataConversionSuite ### What changes were proposed in this pull request? The pr aims to fix flaky ProtobufCatalystDataConversionSuite, include: - Fix the type check (when the random value was empty array, we didn't skip it. Original intention is to skip default values for types.) [SPARK-45588] - When data.get(0) is null, data.get(0).asInstanceOf[Array[Byte]].isEmpty will be thrown java.lang.NullPointerException. [SPARK-45640] Backport above to branch 3.4. Master branch pr: #43424 & #43493 ### Why are the changes needed? Fix flaky ProtobufCatalystDataConversionSuite. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Pass GA - Manually test ### Was this patch authored or co-authored using generative AI tooling? No. Closes #43520 from panbingkun/branch-3.4_SPARK-45640. Authored-by: panbingkun <pbk1982@gmail.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>

…tDataConversionSuite ### What changes were proposed in this pull request? The pr aims to fix flaky ProtobufCatalystDataConversionSuite, include: - Fix the type check (when the random value was empty array, we didn't skip it. Original intention is to skip default values for types.) [SPARK-45588] - When data.get(0) is null, data.get(0).asInstanceOf[Array[Byte]].isEmpty will be thrown java.lang.NullPointerException. [SPARK-45640] Backport above to branch 3.5. Master branch pr: #43424 & #43493 ### Why are the changes needed? Fix flaky ProtobufCatalystDataConversionSuite. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Pass GA - Manually test ### Was this patch authored or co-authored using generative AI tooling? No. Closes #43521 from panbingkun/branch-3.5_SPARK-45640. Authored-by: panbingkun <pbk1982@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…tDataConversionSuite ### What changes were proposed in this pull request? The pr aims to fix flaky ProtobufCatalystDataConversionSuite, include: - Fix the type check (when the random value was empty array, we didn't skip it. Original intention is to skip default values for types.) [SPARK-45588] - When data.get(0) is null, data.get(0).asInstanceOf[Array[Byte]].isEmpty will be thrown java.lang.NullPointerException. [SPARK-45640] Backport above to branch 3.4. Master branch pr: apache#43424 & apache#43493 ### Why are the changes needed? Fix flaky ProtobufCatalystDataConversionSuite. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Pass GA - Manually test ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#43520 from panbingkun/branch-3.4_SPARK-45640. Authored-by: panbingkun <pbk1982@gmail.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>

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

bfe1796

github-actions bot added SQL PROTOBUF labels Oct 24, 2023

Trigger build

dd63420

LuciferYang reviewed Oct 24, 2023

View reviewed changes

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

tigrulya-exe mentioned this pull request Oct 24, 2023

[SPARK-39910][SQL] Delegate path qualification to filesystem during DataSource file path globbing #43463

Closed

SandishKumarHN approved these changes Oct 24, 2023

View reviewed changes

rangadi reviewed Oct 24, 2023

View reviewed changes

rangadi approved these changes Oct 24, 2023

View reviewed changes

HeartSaVioR approved these changes Oct 24, 2023

View reviewed changes

HyukjinKwon approved these changes Oct 24, 2023

View reviewed changes

HyukjinKwon closed this in a6503b2 Oct 24, 2023

This was referenced Oct 25, 2023

[SPARK-45588][SPARK-45640][SQL][TESTS][3.4] Fix flaky ProtobufCatalystDataConversionSuite #43520

Closed

[SPARK-45588][SPARK-45640][SQL][TESTS][3.5] Fix flaky ProtobufCatalystDataConversionSuite #43521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

panbingkun commented Oct 24, 2023

panbingkun commented Oct 24, 2023

MaxGekk commented Oct 24, 2023

SandishKumarHN left a comment

rangadi Oct 24, 2023 •

edited

Loading

rangadi Oct 24, 2023

HeartSaVioR commented Oct 24, 2023

rangadi commented Oct 24, 2023

HeartSaVioR left a comment

HeartSaVioR commented Oct 24, 2023

rangadi commented Oct 24, 2023

HyukjinKwon commented Oct 24, 2023

rangadi commented Oct 24, 2023

HeartSaVioR commented Oct 24, 2023 •

edited

Loading

panbingkun commented Oct 25, 2023

panbingkun commented Oct 25, 2023

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

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

Conversation

panbingkun commented Oct 24, 2023

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?

Was this patch authored or co-authored using generative AI tooling?

panbingkun commented Oct 24, 2023

MaxGekk commented Oct 24, 2023

SandishKumarHN left a comment

Choose a reason for hiding this comment

rangadi Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

rangadi Oct 24, 2023

Choose a reason for hiding this comment

HeartSaVioR commented Oct 24, 2023

rangadi commented Oct 24, 2023

HeartSaVioR left a comment

Choose a reason for hiding this comment

HeartSaVioR commented Oct 24, 2023

rangadi commented Oct 24, 2023

HyukjinKwon commented Oct 24, 2023

rangadi commented Oct 24, 2023

HeartSaVioR commented Oct 24, 2023 • edited Loading

panbingkun commented Oct 25, 2023

panbingkun commented Oct 25, 2023

rangadi Oct 24, 2023 •

edited

Loading

HeartSaVioR commented Oct 24, 2023 •

edited

Loading