-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-45640][SQL][TESTS] Fix flaky ProtobufCatalystDataConversionSuite #43493
Conversation
...tobuf/src/test/scala/org/apache/spark/sql/protobuf/ProtobufCatalystDataConversionSuite.scala
Show resolved
Hide resolved
@SandishKumarHN Could you take a look at the PR, please. Seems similar changes as in #38515 . Also cc @rangadi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -138,6 +138,7 @@ class ProtobufCatalystDataConversionSuite | |||
data != null && | |||
(data.get(0) == defaultValue || | |||
(dt.fields(0).dataType == BinaryType && | |||
data.get(0) != null && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore this suggestion. data.get(0) != null
is correct. It allows testing null.
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. |
@HeartSaVioR I have run this fix with see values from 0 to 10000. All were successful. Could you merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@rangadi |
@HeartSaVioR yes, backporting will be good. |
Merged to master |
Thanks @HyukjinKwon |
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. Thanks in advance! |
Let me do it right away. |
|
…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>
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 thrownjava.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:
Was this patch authored or co-authored using generative AI tooling?
No.