You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "cloud-fan (via GitHub)" <gi...@apache.org> on 2026/06/10 19:52:04 UTC
[PR] [SPARK-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
cloud-fan opened a new pull request, #56437:
URL: https://github.com/apache/spark/pull/56437
### What changes were proposed in this pull request?
This PR adds a new lightweight CI job `binding-policy` ("Config binding policy exceptions check") to `build_and_test.yml`. The job diffs `sql/hive/src/test/resources/conf/binding-policy-exceptions/configs-without-binding-policy-exceptions` against the latest apache/spark base (using the same fork-sync mechanism as the existing `buf` job) and fails if the PR adds any entries to the file, with an error message pointing the author to `.withBindingPolicy(ConfigBindingPolicy.SESSION/PERSISTED/NOT_APPLICABLE)`. Removing entries from the file is still allowed (and encouraged).
The job runs on a bare runner in well under a minute, is gated on the existing `lint` flag from the `precondition` job, only runs on forks (where PR builds run, so the diff against apache/spark master is exactly the PR's change), and skips cleanly on branches where the exceptions file does not exist.
### Why are the changes needed?
The exceptions file is a frozen list of pre-existing configs that were created before the binding policy was introduced, and it should only ever shrink. `SparkConfigBindingPolicySuite` forces every config to either declare a binding policy or be listed in this file, so PR authors who add a new config without a binding policy see a test failure and "fix" it by adding the config to the exceptions file, which defeats the purpose of the enforcement (see https://github.com/apache/spark/pull/56323#discussion_r3389669681 for a recent example). A new config always has a valid policy choice, including `NOT_APPLICABLE` for configs that do not affect the behavior of SQL views/UDFs/procedures, so additions to the exceptions file are never justified. Only a diff-based CI check can catch this, since to a regular test the grown file looks consistent.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Simulated the check locally with the same script against three diffs: a commit adding an entry to the exceptions file (check fails and lists the added entries), a commit removing an entry (check passes), and no change to the file (check passes). Also verified the workflow YAML parses.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code
This pull request and its description were written by Isaac.
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #56437:
URL: https://github.com/apache/spark/pull/56437#issuecomment-4674416307
We have a concurrent task limit for github actions and it's at maximum already. I don't think we can add a task here. However, this does not need to be its own individual task either. In `dev/run-tests.py` there's code to identify changed files. You only need to check if the policy file is in the list and report an error.
The downside is you'll stop all the tests - which is not that bad. The check should be super fast and code change would be minimal.
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56437:
URL: https://github.com/apache/spark/pull/56437#issuecomment-4675551864
@gaogaotiantian The new job never runs on apache/spark, so it doesn't add anything to the ASF concurrent task limit. It is gated by `github.repository != 'apache/spark'` at the job level, and Spark PR CI runs on the contributor's fork (via the `push` trigger of `build_main.yml`), consuming the contributor's own GitHub Actions quota. On apache/spark itself (post-merge and daily scheduled builds) the job is skipped at the scheduler level and never occupies a runner.
I also considered `dev/run-tests.py`, but it is executed by every test job in the build matrix, so the check would run (and on violation, fail) in a dozen jobs per build, while a dedicated job runs it exactly once on a bare runner in ~10 seconds. It also gives PR authors a clearly-named failing check ("Config binding policy exceptions check") right in the PR checks UI, instead of a failure buried in test logs.
It's verified end to end in this PR: a temp commit adding an entry made the job fail with the guidance message in ~10s (https://github.com/cloud-fan/spark/actions/runs/27302316185), and the subsequent commits that shrink the file pass.
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #56437:
URL: https://github.com/apache/spark/pull/56437#discussion_r3392881130
##########
.github/workflows/build_and_test.yml:
##########
@@ -1038,6 +1038,56 @@ jobs:
if: inputs.branch != 'branch-3.5'
run: ./dev/check-protos.py
+ # The config binding policy exceptions file lists pre-existing configs that were created before
+ # the binding policy was introduced. It must never grow: new configs must declare a binding
+ # policy via .withBindingPolicy() instead of being added to the exceptions file. This job fails
Review Comment:
can you add similar words to `SparkConfigBindingPolicySuite` or `configs-without-binding-policy-exceptions`?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -3250,6 +3250,7 @@ object SQLConf {
"This enables sink evolution capability where sinks can be changed while maintaining " +
"a historical record of all sinks used in the checkpoint.")
.version("4.1.0")
Review Comment:
wrong version?
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56437:
URL: https://github.com/apache/spark/pull/56437#issuecomment-4675563906
@yadavay-amzn I've included your conf in this PR
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #56437: [SPARK-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file
URL: https://github.com/apache/spark/pull/56437
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56437:
URL: https://github.com/apache/spark/pull/56437#issuecomment-4677207275
@pan3793 Good question. First, SESSION and NOT_APPLICABLE behave identically at runtime -- the Analyzer retains both kinds when resolving views/UDFs, so the active session's value applies either way. The difference is declared intent: NOT_APPLICABLE says the config is never consulted during view/UDF/procedure resolution at all (physical planning, runtime, transport, etc.); SESSION says it is consulted and the caller's session value should win.
PERSISTED is the only policy that actually changes behavior: the value captured at creation (or the Spark default, if none was saved) is used instead of the caller's. The mental model: use PERSISTED when the view/UDF/procedure should use the create-time value of the config -- think ANSI mode or session timezone, where a persisted object should keep computing the same result as it did at creation regardless of who calls it. Changing query behavior alone is not enough: a bug-fix flag also changes query behavior, but we don't want views to freeze the buggy behavior, so it gets SESSION.
Agreed that by this rule `spark.sql.window.segmentTree.enabled` would more precisely be NOT_APPLICABLE -- since it's runtime-identical to SESSION there's no behavior difference, just labeling. I've also added this decision rule to the `ConfigBindingPolicy` scaladoc in this PR so it's documented where authors pick the policy.
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56437:
URL: https://github.com/apache/spark/pull/56437#issuecomment-4681395852
thanks for the review, merging to master/4.x!
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on PR #56437:
URL: https://github.com/apache/spark/pull/56437#issuecomment-4675180473
Thanks for pinging me @dongjoon-hyun.
Yes, `spark.sql.hive.thriftServer.http.sniHostCheckEnabled` should get `NOT_APPLICABLE` since it's a transport-layer toggle for Jetty's SNI host check on incoming HTTPS connections to ThriftHttpCLIService and has no effect on SQL view / UDF / procedure semantics. Same shape as `useHashAggregateExec` here.
I can either fold the fix into this PR or send it as a small separate PR, whichever is preferred. The sibling `spark.ui.jetty.sniHostCheckEnabled` (from SPARK-56528) has the same reasoning if it's also worth cleaning up while we're at it.
cc @pan3793 to keep me honest on my assessment above
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #56437:
URL: https://github.com/apache/spark/pull/56437#issuecomment-4675678852
> The new job never runs on apache/spark, so it doesn't add anything to the ASF concurrent task limit.
Ah okay that makes sense.
--
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-57377][INFRA] Add CI check to prevent new entries in the config binding policy exceptions file [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #56437:
URL: https://github.com/apache/spark/pull/56437#discussion_r3394989551
##########
core/src/main/scala/org/apache/spark/internal/config/package.scala:
##########
Review Comment:
Oh, I just found this config should live in the `yarn` module instead of the `core`.
Since it is irrelevant to this PR's topic, I can fix it separately later
--
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