-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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-51096][SQL][TESTS] Splitting TransformWithStateSuite into UnsafeRow and Avro encoding suites #49815
Conversation
b9f87ce
to
6286632
Compare
9f4b473
to
14dc48b
Compare
} | ||
} | ||
|
||
test("test that introducing TTL after restart fails query") { |
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.
This applies to both modes right ?
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.
Yeah it does, but the expected error is different based on the encoding type. Couldn't find a good way to expect a different error in the test
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.
Thanks for making this change. Only a nit.
@@ -1815,7 +1352,6 @@ class TransformWithStateSuite extends StateStoreMetricsTest | |||
TransformWithStateSuiteUtils.NUM_SHUFFLE_PARTITIONS.toString) { | |||
withTempDir { checkpointDir => | |||
// When Avro is used, we want to set the StructFields to nullable | |||
val shouldBeNullable = usingAvroEncoding() |
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.
@ericm-db Looks like failure happens from refactored suite? |
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 pending CI
The CI failure is not relevant - it only failed from pyspark-pandas and this PR only touched "Scala tests". |
Thanks! Merging to master/4.0. |
…feRow and Avro encoding suites ### What changes were proposed in this pull request? Splitting the TransformWithStateSuite into TransformWithStateUnsafeRowEncodingSuite and TransformWithStateAvroEncodingSuite, in order to remove it from the slow test category ### Why are the changes needed? To remove TransformWithState-related suites from the Slow test category ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Just moving tests around, no need to test ### Was this patch authored or co-authored using generative AI tooling? No Closes #49815 from ericm-db/split-tws-suite. Authored-by: Eric Marnadi <[email protected]> Signed-off-by: Jungtaek Lim <[email protected]> (cherry picked from commit 9dbbb5b) Signed-off-by: Jungtaek Lim <[email protected]>
…feRow and Avro encoding suites ### What changes were proposed in this pull request? Splitting the TransformWithStateSuite into TransformWithStateUnsafeRowEncodingSuite and TransformWithStateAvroEncodingSuite, in order to remove it from the slow test category ### Why are the changes needed? To remove TransformWithState-related suites from the Slow test category ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Just moving tests around, no need to test ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#49815 from ericm-db/split-tws-suite. Authored-by: Eric Marnadi <[email protected]> Signed-off-by: Jungtaek Lim <[email protected]>
What changes were proposed in this pull request?
Splitting the TransformWithStateSuite into TransformWithStateUnsafeRowEncodingSuite and TransformWithStateAvroEncodingSuite, in order to remove it from the slow test category
Why are the changes needed?
To remove TransformWithState-related suites from the Slow test category
Does this PR introduce any user-facing change?
No
How was this patch tested?
Just moving tests around, no need to test
Was this patch authored or co-authored using generative AI tooling?
No