Skip to content

[SPARK-52962][SQL] BroadcastExchangeExec should not reset metrics - #51673

Closed
viirya wants to merge 3 commits into
apache:masterfrom
viirya:broadcast_resetmetrics
Closed

[SPARK-52962][SQL] BroadcastExchangeExec should not reset metrics#51673
viirya wants to merge 3 commits into
apache:masterfrom
viirya:broadcast_resetmetrics

Conversation

@viirya

@viiryaviirya commented Jul 26, 2025

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This patch implements a no-op resetMetrics method for BroadcastExchangeExec.

Why are the changes needed?

This was found during debugging test failures at #51623.

A materialized broadcast exchange (i.e., BroadcastExchangeExec) operator will be trimmed by AQE's empty relation propagation rule to an empty local relation. That's is because its metrics was reset. BroadcastExchangeExec after materialized won't be materialized again, so its metrics won't be updated again.

It is weird and inconsistent that a materialized broadcast exchange still hold broadcast value but its metrics are reset.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit test

Was this patch authored or co-authored using generative AI tooling?

No

@viiryaviirya changed the title [SPARK-XXXXX][SQL] BroadcastExchangeExec should not reset metrics[SPARK-52962][SQL] BroadcastExchangeExec should not reset metricsJul 26, 2025

@peter-tothpeter-toth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, LGTM.

@viirya

Copy link
Copy Markdown
MemberAuthor

Merged to master. Thanks @peter-toth

@viirya
viirya deleted the broadcast_resetmetrics branch July 26, 2025 16:12

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Thank you, @viirya and Peter.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@viirya@peter-toth@dongjoon-hyun