You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "bhollis-dbx (via GitHub)" <gi...@apache.org> on 2026/09/12 02:31:06 UTC
[PR] [SPARK-59456][SQL] Fix: RuleExecutor.PlanChangeLogger does work even when it doesn't log [spark]
bhollis-dbx opened a new pull request, #58755:
URL: https://github.com/apache/spark/pull/58755
<!--
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
'common/utils/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
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Added a functional `withLogLevel` helper to Logging to make it easy to skip work when the log level is below a target.
Then used that in `RuleExecutor.PlanChangeLogger` to avoid calling `fastEquals` unnecessarily.
### Why are the changes needed?
The existing code always called `fastEquals`, even when it couldn't log anything because of the log leve..
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
The change itself is trivial.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex CLI 0.147.0 (GPT-5)
--
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
Re: [PR] [SPARK-59456][SQL] Fix: RuleExecutor.PlanChangeLogger does work even when it doesn't log [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #58755:
URL: https://github.com/apache/spark/pull/58755#discussion_r4299741301
##########
common/utils/src/main/scala/org/apache/spark/internal/Logging.scala:
##########
@@ -307,6 +307,10 @@ trait Logging {
log.isTraceEnabled
}
+ protected def withLogLevel(level: Slf4jLevel)(f: => Unit): Unit = {
Review Comment:
**[P2] Keep the log-level guard out of the shared Logging trait**
Adding this method generates a public forwarder in serializable `Logging` implementations, changing `UDTRegistration$`'s computed `serialVersionUID` from `-3417430582631602433` to `-3728152430189732578`. This exactly matches the existing `UdfSerializationSuite` CI failure. Spark Connect payloads from released 4.1/4.2 clients that capture `UDTRegistration` now fail with `InvalidClassException`; the audited 4.0 compatibility transition also stops matching because `UdfSerialization` requires the exact producer/consumer UID pair. Keep the guard directly in `PlanChangeLogger`, or use a private helper there, to preserve serialization compatibility. Updating only the existing transition's local UID would still miss the newly necessary transitions from 4.1/4.2.
--
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
Re: [PR] [SPARK-59456][SQL] Fix: RuleExecutor.PlanChangeLogger does work even when it doesn't log [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang closed pull request #58755: [SPARK-59456][SQL] Fix: RuleExecutor.PlanChangeLogger does work even when it doesn't log
URL: https://github.com/apache/spark/pull/58755
--
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
Re: [PR] [SPARK-59456][SQL] Fix: RuleExecutor.PlanChangeLogger does work even when it doesn't log [spark]
Posted by "bhollis-dbx (via GitHub)" <gi...@apache.org>.
bhollis-dbx commented on PR #58755:
URL: https://github.com/apache/spark/pull/58755#issuecomment-6331095379
@gengliangwang only an unrelated build issue remains
--
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
Re: [PR] [SPARK-59456][SQL] Fix: RuleExecutor.PlanChangeLogger does work even when it doesn't log [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #58755:
URL: https://github.com/apache/spark/pull/58755#issuecomment-6271739120
@bhollis-dbx could you re-trggier the CI jobs? Thanks!
--
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
Re: [PR] [SPARK-59456][SQL] Fix: RuleExecutor.PlanChangeLogger does work even when it doesn't log [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #58755:
URL: https://github.com/apache/spark/pull/58755#issuecomment-6346181903
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/f96ffb6921640b983f4744b47625ddafb2f43f06
- merged into branch-4.x https://github.com/apache/spark/commit/e02ab1c28355a20a84305231bf38dc7c4b5cab1b
*Posted by `merge_spark_pr.py`*
--
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