-
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-50992][SQL] OOMs and performance issues with AQE in large plans #49724
base: master
Are you sure you want to change the base?
Conversation
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.
Thank you for making a PR, @SauronShepherd . I have a few comments.
- Please revise the PR title to describe what this PR code provides. The current title is proper for JIRA issue report, but looks improper for PR title.
- Although I understand the intention, please don't change the default in this PR because it's a behavior change.
- Please minimize the change by adding new value
off
at the end. For example,
- or 'formatted'.
+ , 'formatted', or 'off'.
@@ -218,7 +216,7 @@ case class CachedRDDBuilder( | |||
private val materializedPartitions = cachedPlan.session.sparkContext.longAccumulator | |||
|
|||
val cachedName = tableName.map(n => s"In-memory table $n") | |||
.getOrElse(StringUtils.abbreviate(cachedPlan.toString, 1024)) | |||
.getOrElse(cachedPlan.simpleStringWithNodeId()) |
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.
Is this equivalent logically?
I was expecting your first point since it's a behavioral change. However, given that keeping the default value as-is has a major impact on memory I thought it was worth discussing. I still believe that converting plans to strings shouldn't be Spark's default behavior unless strictly necessary. Besides, I'm not sure many Spark users would even notice it -well, actually maybe they would do, because of the performance improvement in their applications-. I've been working with Spark for over eight years, and whenever I needed to inspect a plan, I explicitly ran the explain method rather than relying on the Spark UI. However, I’ve often seen teams resort to checkpointing their DataFrames simply because Spark took what felt like an eternity to begin execution— and this month I've observed the same looking at GraphFrames code. I still don’t see the need for Spark to be so verbose internally; in my view, performance should take precedence over verbosity in logging. Regarding the new "off" value being placed at the end, the changes are minimal. However, to me, it makes more sense for "off" to come before the value that produces less content (i.e., "simple"), following a logical progression from lower to higher verbosity. As for the change in cachedName, it doesn’t yield the exact same result, but since it’s merely a literal identifier -a description- and not a key, this difference shouldn't matter. Like the previous changes, this modification is crucial to avoid traversing the entire tree of the plan each time a CachedRDDBuilder is created. Because even with the new explain mode off, despite avoiding the OOM, generating the string still has a significant impact in performance, even for not too large plans. That said, I have no problem in making these changes in my PR—I was just hoping to address an internal behavior that I’m not convinced Spark should have by default. Thanks for your feedback, @dongjoon-hyun Ángel |
What changes were proposed in this pull request?
This PR introduces a new explain mode
off
to disable the generation of physical plan strings. It also modifies the internal attributecachedName
ofCachedRDDBuilder
objects.Why are the changes needed?
Whenever a plan changes (which happens frequently when AQE kicks in), the physical plan's explain is generated as a plain string. This process is highly expensive for large plans. Moreover, these strings are stored in the
ListenerBus
ofSparkContext
, consuming heap memory and potentially leading to OutOfMemory errors.Due to its potential negative impact on Spark applications, this information should be available only on demand for debugging purposes. This PR introduces a new explain mode
off
, which is set as the default to prevent unnecessary string generation. However, explicit explanations of a DataFrame remain accessible even when this mode is active.Additionally, when a
CachedRDDBuilder
object is created without a definedtableName
, the full string representation of the plan is also computed, only to later extract the first 1024 characters. This is an expensive operation and has been replaced with a more efficient call tosimpleStringWithNodeId
to avoid unnecessary computation.IMPORTANT NOTE: This issue is causing an OutOfMemory (OOM) error in certain unit tests within GraphFrames, as reported in Connected Components gives wrong results. It may also be a contributing factor to the frequent overuse of checkpoints not only in GraphFrames, but also for many Spark users.
Does this PR introduce any user-facing change?
Yes. By default, plan descriptions will no longer be available in the Spark UI. If users require this information, they must explicitly enable it by setting the
spark.sql.ui.explainMode
Spark configuration.How was this patch tested?
Unit tests from sql/core and sql/catalyst, along with the test attached to the SPARK-50992 ticket.
Was this patch authored or co-authored using generative AI tooling?
No.