jubins opened a new pull request, #57550:
URL: https://github.com/apache/spark/pull/57550
### What is the purpose of the change
Fixes SPARK-58358 - makes `SparkSession.removeTag` reject invalid tags
(empty string, or a tag containing the `,` separator), matching `addTag`, the
documented API contract, and the Spark Connect implementation.
In Spark Classic, `removeTag` went straight to the tag map
(`managedJobTags.get().remove(tag)`) and skipped validation, so `removeTag("")`
and `removeTag("a,b")` silently succeeded. This contradicts the method's own
docstring ("Cannot contain ',' (comma) character or be an empty string"),
diverges from Spark Connect (which validates both `addTag` and `removeTag`),
and is inconsistent with the sibling `addTag` and with
`SparkContext.removeJobTags`, all of which call
`SparkContext.throwIfInvalidTag`.
### Brief change log
- `sql/core/src/main/scala/org/apache/spark/sql/classic/SparkSession.scala`:
`removeTag` now calls `SparkContext.throwIfInvalidTag(tag)` before removing,
mirroring `addTag`.
-
`sql/core/src/test/scala/org/apache/spark/sql/SparkSessionJobTaggingAndCancellationSuite.scala`:
added a test asserting `addTag` and `removeTag` reject empty /
comma-containing tags, that rejected tags do not leak into the tag set, and
that removing a valid absent tag stays a no-op.
- `python/pyspark/sql/tests/test_job_cancellation.py`: added
`test_invalid_tags` covering the same contract in both Spark Classic and Spark
Connect modes (via the shared mixin); also tightened the
`check_job_cancellation` helper to re-raise non-cancellation exceptions instead
of misreading them as successful cancellations.
### Verifying this change
This change adds a correctness fix plus test coverage.
- New Scala test `SPARK-58358: addTag and removeTag reject invalid tags`
passes with the fix and fails without it (reverting the one-line fix produces
"Expected exception java.lang.IllegalArgumentException to be thrown, but no
exception was thrown" at the `removeTag` assertion), confirming the test
exercises the fixed behavior.
- New PySpark test `test_invalid_tags` runs in both Classic and Connect
modes.
- Existing tag/cancellation tests in the affected suites continue to pass.
Run:
- build/sbt 'sql/testOnly
org.apache.spark.sql.SparkSessionJobTaggingAndCancellationSuite'
- python/run-tests --testnames pyspark.sql.tests.test_job_cancellation
- python/run-tests --testnames
pyspark.sql.tests.connect.test_parity_job_cancellation
### Does this pull request potentially affect one of the following parts
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with
@Public/@Evolving: yes - `SparkSession.removeTag` now throws
`IllegalArgumentException` on invalid tags where it previously silently ignored
them; this aligns Classic with the documented contract and with Spark Connect
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: no
- The S3 file system connector: no
### Documentation
Does this pull request introduce a new feature? No - this is a bug fix that
enforces existing documented behaviour.
### Was generative AI tooling used to co-author this PR?
- [x] Yes - Claude Code was used as a pair-programming assistant. All
changes were reviewed and verified by the author. Generated-by: Claude Opus 4.8
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
