[SPARK-48610][SQL] refactor: use auxiliary idMap instead of OP_ID_TAG #46965
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
cloud-fan
reviewed
Jun 13, 2024
| val OP_ID_TAG = TreeNodeTag[Int]("operatorId") | ||
| val CODEGEN_ID_TAG = new TreeNodeTag[Int]("wholeStageCodegenId") | ||
|
|
||
| val localIdMap: ThreadLocal[java.util.Map[QueryPlan[_], Int]] = ThreadLocal.withInitial(() => |
Contributor
There was a problem hiding this comment.
can we define the scope of this thread local? When it's set and when it's cleared.
Contributor
Author
There was a problem hiding this comment.
the scope is ExplainUtils.processPlan, but I defined it here because QueryPlan also need this, and don't have access to execution package from catalyst. Added comments to clarify.
cloud-fan
approved these changes
Jun 17, 2024
Contributor
|
thanks, merging to master/3.5! |
cloud-fan
pushed a commit
that referenced
this pull request
Jun 17, 2024
### What changes were proposed in this pull request? refactor: In `ExplainUtils.processPlan`, use auxiliary idMap instead of OP_ID_TAG ### Why are the changes needed? #45282 introduced synchronize to `ExplainUtils.processPlan` to avoid race condition when multiple queries refers to same cached plan. The granularity of lock is too large. We can try to fix the root cause of this concurrency issue by refactoring the usage of mutable `OP_ID_TAG`, which is not a good practice in terms of immutable nature of SparkPlan. Instead, we can use an auxiliary id map, with object identity as the key. The entire scope of `OP_ID_TAG` usage is within `ExplainUtils.processPlan`, therefore it's safe to do so, with thread local to make it available in other involved classes. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? existing UTs. ### Was this patch authored or co-authored using generative AI tooling? NO Closes #46965 from liuzqt/SPARK-48610. Authored-by: Ziqi Liu <ziqi.liu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit d3da240) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
senthh
pushed a commit
to acceldata-io/spark3
that referenced
this pull request
Aug 23, 2025
…P_ID_TAG ### What changes were proposed in this pull request? refactor: In `ExplainUtils.processPlan`, use auxiliary idMap instead of OP_ID_TAG ### Why are the changes needed? apache#45282 introduced synchronize to `ExplainUtils.processPlan` to avoid race condition when multiple queries refers to same cached plan. The granularity of lock is too large. We can try to fix the root cause of this concurrency issue by refactoring the usage of mutable `OP_ID_TAG`, which is not a good practice in terms of immutable nature of SparkPlan. Instead, we can use an auxiliary id map, with object identity as the key. The entire scope of `OP_ID_TAG` usage is within `ExplainUtils.processPlan`, therefore it's safe to do so, with thread local to make it available in other involved classes. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? existing UTs. ### Was this patch authored or co-authored using generative AI tooling? NO Closes apache#46965 from liuzqt/SPARK-48610. Authored-by: Ziqi Liu <ziqi.liu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit d3da240) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
senthh
pushed a commit
to acceldata-io/spark3
that referenced
this pull request
Oct 31, 2025
…P_ID_TAG ### What changes were proposed in this pull request? refactor: In `ExplainUtils.processPlan`, use auxiliary idMap instead of OP_ID_TAG ### Why are the changes needed? apache#45282 introduced synchronize to `ExplainUtils.processPlan` to avoid race condition when multiple queries refers to same cached plan. The granularity of lock is too large. We can try to fix the root cause of this concurrency issue by refactoring the usage of mutable `OP_ID_TAG`, which is not a good practice in terms of immutable nature of SparkPlan. Instead, we can use an auxiliary id map, with object identity as the key. The entire scope of `OP_ID_TAG` usage is within `ExplainUtils.processPlan`, therefore it's safe to do so, with thread local to make it available in other involved classes. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? existing UTs. ### Was this patch authored or co-authored using generative AI tooling? NO Closes apache#46965 from liuzqt/SPARK-48610. Authored-by: Ziqi Liu <ziqi.liu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit d3da240) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
senthh
pushed a commit
to acceldata-io/spark3
that referenced
this pull request
Nov 3, 2025
…P_ID_TAG ### What changes were proposed in this pull request? refactor: In `ExplainUtils.processPlan`, use auxiliary idMap instead of OP_ID_TAG ### Why are the changes needed? apache#45282 introduced synchronize to `ExplainUtils.processPlan` to avoid race condition when multiple queries refers to same cached plan. The granularity of lock is too large. We can try to fix the root cause of this concurrency issue by refactoring the usage of mutable `OP_ID_TAG`, which is not a good practice in terms of immutable nature of SparkPlan. Instead, we can use an auxiliary id map, with object identity as the key. The entire scope of `OP_ID_TAG` usage is within `ExplainUtils.processPlan`, therefore it's safe to do so, with thread local to make it available in other involved classes. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? existing UTs. ### Was this patch authored or co-authored using generative AI tooling? NO Closes apache#46965 from liuzqt/SPARK-48610. Authored-by: Ziqi Liu <ziqi.liu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit d3da240) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
turboFei
pushed a commit
to turboFei/spark
that referenced
this pull request
Nov 6, 2025
…apache#626) ### What changes were proposed in this pull request? refactor: In `ExplainUtils.processPlan`, use auxiliary idMap instead of OP_ID_TAG ### Why are the changes needed? apache#45282 introduced synchronize to `ExplainUtils.processPlan` to avoid race condition when multiple queries refers to same cached plan. The granularity of lock is too large. We can try to fix the root cause of this concurrency issue by refactoring the usage of mutable `OP_ID_TAG`, which is not a good practice in terms of immutable nature of SparkPlan. Instead, we can use an auxiliary id map, with object identity as the key. The entire scope of `OP_ID_TAG` usage is within `ExplainUtils.processPlan`, therefore it's safe to do so, with thread local to make it available in other involved classes. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? existing UTs. ### Was this patch authored or co-authored using generative AI tooling? NO Closes apache#46965 from liuzqt/SPARK-48610. Authored-by: Ziqi Liu <ziqi.liu@databricks.com> (cherry picked from commit d3da240) Signed-off-by: Wenchen Fan <wenchen@databricks.com> Co-authored-by: Ziqi Liu <ziqi.liu@databricks.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
refactor: In
ExplainUtils.processPlan, use auxiliary idMap instead of OP_ID_TAGWhy are the changes needed?
#45282 introduced synchronize to
ExplainUtils.processPlanto avoid race condition when multiple queries refers to same cached plan.The granularity of lock is too large. We can try to fix the root cause of this concurrency issue by refactoring the usage of mutable
OP_ID_TAG, which is not a good practice in terms of immutable nature of SparkPlan.Instead, we can use an auxiliary id map, with object identity as the key. The entire scope of
OP_ID_TAGusage is withinExplainUtils.processPlan, therefore it's safe to do so, with thread local to make it available in other involved classes.Does this PR introduce any user-facing change?
NO
How was this patch tested?
existing UTs.
Was this patch authored or co-authored using generative AI tooling?
NO