[
https://issues.apache.org/jira/browse/SPARK-58358?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jubin Soni updated SPARK-58358:
-------------------------------
Component/s: SQL
(was: Tests)
Language: Scala (was: python)
Affects Version/s: 4.0.0
(was: 4.1.0)
Description:
{{SparkSession.removeTag(tag)}} in Spark Classic does not validate its
argument, even though:
* Its API contract documents that tags {*}cannot be empty or contain a comma
({{{}','{}}}){*}.
* The sibling {{addTag}} API validates input via
{{{}SparkContext.throwIfInvalidTag{}}}.
* The Spark Connect implementation validates both {{addTag}} and
{{{}removeTag{}}}.
* {{SparkContext.removeJobTags}} also validates its input.
The current Spark Classic implementation removes the tag directly from the
managed tag set:
override def removeTag(tag: String): Unit = managedJobTags.get().remove(tag)
As a result, calls such as {{removeTag("")}} and {{removeTag("a,b")}} silently
succeed instead of throwing {{{}IllegalArgumentException{}}}.
This is a correctness and consistency issue because:
* Spark Classic does not enforce its documented API contract.
* Spark Classic and Spark Connect exhibit different behavior for the same API.
*Proposed Fix:*
Update the Spark Classic implementation of `{{{}SparkSession.removeTag{}}}` to
call:
{code:java}
SparkContext.throwIfInvalidTag(tag){code}
before removing the tag, mirroring the existing behavior of {{{}addTag{}}}.
*Test Coverage:*
Add missing validation tests for both implementations:
* *Scala ({{{}SparkSessionJobTaggingAndCancellationSuite{}}})*
** Verify {{addTag}} and {{removeTag}} reject empty and comma-containing tags.
** Verify invalid tags are not added to the current tag set.
** Verify removing a valid but absent tag remains a no-op.
* *PySpark ({{{}test_job_cancellation{}}})*
** Verify the same API contract in both Spark Classic and Spark Connect (via
the shared test mixin).
** Additionally, tighten the {{check_job_cancellation}} helper to re-raise
non-cancellation exceptions instead of treating any exception as a successful
cancellation.
*Impact:*
* Test and correctness improvement only.
* No user-facing API changes beyond enforcing the existing documented contract
consistently across Spark Classic and Spark Connect.
was:
The test helper {{check_job_cancellation}} in:
python/pyspark/sql/tests/test_job_cancellation.py
Because this catches {*}all exceptions{*}, unrelated failures (for example, a
UDF import error, runs a UDF-based job in a thread and currently classifies the
outcome as "cancelled" using a broad {{except Exception}} handler:
try:
setter(job_id)
...
self.spark.createDataFrame(...).collect()
is_job_cancelled[index] = False
except Exception:
# Assume that exception means job cancellation.
is_job_cancelled[index] = True
Because this catches {*}all exceptions{*}, unrelated failures (for example, a
UDF import error, serialization failure, or environment/configuration issue)
are incorrectly interpreted as successful job cancellation.
This can mask real test failures and allow cancellation tests such as
{{test_interrupt_all}} (and, once re-enabled, {{{}test_interrupt_tag{}}}) to
pass for the wrong reason.
*Proposal:*
Narrow the exception handling so that the test only treats
cancellation-specific failures as a successful cancellation. The helper should
verify that the raised exception corresponds to an actual Spark job
cancellation (for example, by checking the relevant cancellation
exception/error class) rather than assuming any exception indicates
cancellation.
Any non-cancellation failure should fail the test instead of being counted as a
pass.
*Impact:*
* Test-only change.
* No user-facing behavior change.
* Improves test correctness by preventing unrelated failures from being
silently ignored.
Issue Type: Bug (was: Test)
Priority: Minor (was: Major)
> Tighten job cancellation test to assert on the cancellation error instead of
> swallowing all exceptions
> ------------------------------------------------------------------------------------------------------
>
> Key: SPARK-58358
> URL: https://issues.apache.org/jira/browse/SPARK-58358
> Project: Spark
> Issue Type: Bug
> Components: PySpark, SQL
> Affects Versions: 4.0.0
> Reporter: Jubin Soni
> Priority: Minor
>
> {{SparkSession.removeTag(tag)}} in Spark Classic does not validate its
> argument, even though:
> * Its API contract documents that tags {*}cannot be empty or contain a comma
> ({{{}','{}}}){*}.
> * The sibling {{addTag}} API validates input via
> {{{}SparkContext.throwIfInvalidTag{}}}.
> * The Spark Connect implementation validates both {{addTag}} and
> {{{}removeTag{}}}.
> * {{SparkContext.removeJobTags}} also validates its input.
> The current Spark Classic implementation removes the tag directly from the
> managed tag set:
> override def removeTag(tag: String): Unit = managedJobTags.get().remove(tag)
> As a result, calls such as {{removeTag("")}} and {{removeTag("a,b")}}
> silently succeed instead of throwing {{{}IllegalArgumentException{}}}.
> This is a correctness and consistency issue because:
> * Spark Classic does not enforce its documented API contract.
> * Spark Classic and Spark Connect exhibit different behavior for the same
> API.
> *Proposed Fix:*
> Update the Spark Classic implementation of `{{{}SparkSession.removeTag{}}}`
> to call:
> {code:java}
> SparkContext.throwIfInvalidTag(tag){code}
> before removing the tag, mirroring the existing behavior of {{{}addTag{}}}.
>
> *Test Coverage:*
> Add missing validation tests for both implementations:
> * *Scala ({{{}SparkSessionJobTaggingAndCancellationSuite{}}})*
> ** Verify {{addTag}} and {{removeTag}} reject empty and comma-containing
> tags.
> ** Verify invalid tags are not added to the current tag set.
> ** Verify removing a valid but absent tag remains a no-op.
> * *PySpark ({{{}test_job_cancellation{}}})*
> ** Verify the same API contract in both Spark Classic and Spark Connect (via
> the shared test mixin).
> ** Additionally, tighten the {{check_job_cancellation}} helper to re-raise
> non-cancellation exceptions instead of treating any exception as a successful
> cancellation.
> *Impact:*
> * Test and correctness improvement only.
> * No user-facing API changes beyond enforcing the existing documented
> contract consistently across Spark Classic and Spark Connect.
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
