You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/09/07 12:58:57 UTC
[GitHub] [spark] LeeeeLiu opened a new pull request, #37819: SPARK-40377 Allow customize maxBroadcastTableBytes and maxBroadcastRows
LeeeeLiu opened a new pull request, #37819:
URL: https://github.com/apache/spark/pull/37819
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Add two parameters to allow customizing maxBroadcastTableBytes and maxBroadcastRows
### Why are the changes needed?
Recently, we encountered some driver OOM problems. Some tables with large data volume were compressed using Snappy and then broadcast join was performed, but the actual data volume was too large, which resulted in driver OOM.
The values of maxBroadcastTableBytes and maxBroadcastRows are hardcoded, 8GB and 512000000 respectively. Maybe we can allow customization of these values, configure smaller values according to different scenarios, and prohibit broadcast joins for tables with large data volumes to avoid driver OOM.
### Does this PR introduce _any_ user-facing change?
NO
### How was this patch tested?
Unit test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LeeeeLiu commented on pull request #37819: [SPARK-40377][SQL] Allow customize maxBroadcastTableBytes and maxBroadcastRows
Posted by GitBox <gi...@apache.org>.
LeeeeLiu commented on PR #37819:
URL: https://github.com/apache/spark/pull/37819#issuecomment-1245333086
cc. @dongjoon-hyun @cloud-fan @HyukjinKwon . Could you please verify this patch? Thanks so much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #37819: [SPARK-40377][SQL] Allow customize maxBroadcastTableBytes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #37819:
URL: https://github.com/apache/spark/pull/37819#issuecomment-1386275532
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LeeeeLiu commented on a diff in pull request #37819: [SPARK-40377][SQL] Allow customize maxBroadcastTableBytes and maxBroadcastRows
Posted by GitBox <gi...@apache.org>.
LeeeeLiu commented on code in PR #37819:
URL: https://github.com/apache/spark/pull/37819#discussion_r990778356
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/BroadcastExchangeExec.scala:
##########
@@ -121,7 +120,7 @@ case class BroadcastExchangeExec(
// and only 70% of the slots can be used before growing in UnsafeHashedRelation,
// here the limitation should not be over 341 million.
(BytesToBytesMap.MAX_CAPACITY / 1.5).toLong
- case _ => 512000000
Review Comment:
It's a good idea. Maybe I can try to add the config. I'll revert the changes. Thanks so much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] closed pull request #37819: [SPARK-40377][SQL] Allow customize maxBroadcastTableBytes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #37819: [SPARK-40377][SQL] Allow customize maxBroadcastTableBytes
URL: https://github.com/apache/spark/pull/37819
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #37819: [SPARK-40377][SQL] Allow customize maxBroadcastTableBytes and maxBroadcastRows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #37819:
URL: https://github.com/apache/spark/pull/37819#issuecomment-1241842477
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] holdenk commented on a diff in pull request #37819: [SPARK-40377][SQL] Allow customize maxBroadcastTableBytes and maxBroadcastRows
Posted by GitBox <gi...@apache.org>.
holdenk commented on code in PR #37819:
URL: https://github.com/apache/spark/pull/37819#discussion_r989487173
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -518,6 +518,24 @@ object SQLConf {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("10MB")
+ val MAX_BROADCAST_TABLE_BYTES =
+ buildConf("spark.sql.maxBroadcastTableBytes")
+ .doc("Configures the maximum size in bytes for a table when performing a broadcast join. " +
+ "The maximum allowed value is 8G.")
Review Comment:
Any particular reason why this value was chosen?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #37819: [SPARK-40377][SQL] Allow customize maxBroadcastTableBytes and maxBroadcastRows
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37819:
URL: https://github.com/apache/spark/pull/37819#discussion_r990166936
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/BroadcastExchangeExec.scala:
##########
@@ -121,7 +120,7 @@ case class BroadcastExchangeExec(
// and only 70% of the slots can be used before growing in UnsafeHashedRelation,
// here the limitation should not be over 341 million.
(BytesToBytesMap.MAX_CAPACITY / 1.5).toLong
- case _ => 512000000
Review Comment:
This hardcoded value is probably the physical limitation. If we want fine-grained control over broadcast join decisions, we can probably add a config that is similar to `spark.sql.autoBroadcastJoinThreshold` and is based on number of rows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LeeeeLiu commented on a diff in pull request #37819: [SPARK-40377][SQL] Allow customize maxBroadcastTableBytes and maxBroadcastRows
Posted by GitBox <gi...@apache.org>.
LeeeeLiu commented on code in PR #37819:
URL: https://github.com/apache/spark/pull/37819#discussion_r990753194
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -518,6 +518,24 @@ object SQLConf {
.bytesConf(ByteUnit.BYTE)
.createWithDefaultString("10MB")
+ val MAX_BROADCAST_TABLE_BYTES =
+ buildConf("spark.sql.maxBroadcastTableBytes")
+ .doc("Configures the maximum size in bytes for a table when performing a broadcast join. " +
+ "The maximum allowed value is 8G.")
Review Comment:
In some cases, we need to set a smaller value to avoid using too much memory for broadcast joins. If the actual value of the broadcast table loaded into memory is greater than this value, an exception will be thrown to avoid driver OOM. 8GB is the historical experience value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org