[SPARK-56455][SQL][TESTS] Add test for broadcast failure when exceeding maxBroadcastTableSize#55313
[SPARK-56455][SQL][TESTS] Add test for broadcast failure when exceeding maxBroadcastTableSize#55313pratham76 wants to merge 1 commit intoapache:masterfrom
Conversation
|
@sunchao @dongjoon-hyun Could you have a look? Thanks! |
| val ex = intercept[SparkException] { | ||
| joinDF.collect() | ||
| } | ||
| assert(ex.getMessage.contains("Cannot broadcast")) |
There was a problem hiding this comment.
nit: this check is probably to board: since we are testing against maxBroadcastTableSize, we can check the error message directly here or the error code _LEGACY_ERROR_TEMP_2249.
There was a problem hiding this comment.
Thanks @sunchao, have used the approach of using error code to verify here.
HyukjinKwon
changed the title
pratham76
force-pushed
the
test-broadcast
branch
from
e4bf10f to
022c945
Compare
|
@sunchao If there are no more changes required, could we merge this PR? Also please do let know if i can add it for 4.1.x as well, as the change was introduced then on. |
| } | ||
|
|
||
| test("SPARK-56455: maxBroadcastTableSize config enforcement") { | ||
| withSQLConf(SQLConf.MAX_BROADCAST_TABLE_SIZE.key -> "104857600") { |
There was a problem hiding this comment.
Given that 104857600 and spark.range(100) are roughly related, this positive case seems redundant because the other test cases covers this case already.
There was a problem hiding this comment.
Noted. Have removed the redundant case.
| assert(joinDF.collect().length == 100) | ||
| } | ||
|
|
||
| withSQLConf(SQLConf.MAX_BROADCAST_TABLE_SIZE.key -> "100") { |
There was a problem hiding this comment.
For this negative case, yes. I agree that it seems that we didn't have explicitly before. Could you revise the PR title and test case name specifically for this because the current PR title, "Add test coverage for maxBroadcastTableSize config enforcement", is too broad and ignores the existing test coverage.
There was a problem hiding this comment.
Noted. Could i change the title as "Add test for broadcast failure when exceeding maxBroadcastTableSize"? This seems to be appropriate for the test coverage. Do let me know your thoughts. Thanks!
pratham76
force-pushed
the
test-broadcast
branch
from
022c945 to
0546825
Compare
pratham76
changed the title
|
@dongjoon-hyun Could you inform if the changes are okay to proceed with? Thanks! |
pratham76
force-pushed
the
test-broadcast
branch
from
0546825 to
ef927b1
Compare
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Thank you, @pratham76 .
|
Pending CIs. |
|
Thanks @dongjoon-hyun. CI is successful can we merge this PR? |
|
Of course, @pratham76 . |
|
Merged to master. I added you to the Apache Spark contributor group and assigned SPARK-56455 to you. Welcome to the Apache Spark community and congratulations for your first JIRA issue (and the second commit). |
|
Thank you @dongjoon-hyun! 😊 Could we also port this test to |
|
This will be a part of Apache Spark 4.2.0 whose https://spark.apache.org/versioning-policy.html 
So, although I understand your passion, no, the requested backporting is not required in the community, @pratham76 . |
3 participants
What changes were proposed in this pull request?
Currently, there is no validation for enforcement of
spark.sql.maxBroadcastTableSize.This change introduces unit tests that verifies that proper exception is thrown when exceeding the threshold determined by the above config.
Why are the changes needed?
This helps prevent regressions and ensures consistent enforcement of broadcast size limits.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing Test Suites -- additional test cases added
Was this patch authored or co-authored using generative AI tooling?
No