[SPARK-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated #51772

yaooqinn · 2025-08-01T06:45:16Z

What changes were proposed in this pull request?

This PR implements and calls new APIs in FileCommitProtocol instead of the deprecated

Why are the changes needed?

FileCommitProtocol and related classes are complicated as they play a lot of tricks for tasks like file naming, config setting/propagation, e.t.c. Removing these references can improve the call stack a bit. And also, we can make these deprecated ones ignorable。

Does this PR introduce any user-facing change?

No, nothing changes for existing implementations or end-users

How was this patch tested?

Pass existing CIs

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

no

…eCommitProtocol

...main/scala/org/apache/spark/sql/execution/streaming/runtime/ManifestFileCommitProtocol.scala

dongjoon-hyun · 2025-08-01T21:09:02Z

sql/core/src/test/scala/org/apache/spark/sql/sources/PartitionedWriteSuite.scala


  override def newTaskTempFileAbsPath(
-      taskContext: TaskAttemptContext, absoluteDir: String, ext: String): String = {
+       taskContext: TaskAttemptContext, absoluteDir: String, spec: FileNameSpec): String = {


nit. A wrong indentation.

dongjoon-hyun · 2025-08-01T21:10:23Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala

      taskAttemptContext,
      None,
-      f"-c$fileCounter%03d" + ext)
+      FileNameSpec("", f"-c$fileCounter%03d" + ext))


This looks like a new independent change, too.

Calling the newer newTaskTempFile with spec is the goal of this PR

PR title is not~

Remove inner references of deprecated APIs in FileCommitProtocol

If this is this PR goal, please remove the throwing Exceptions in the deprecated APIs.

Calling the newer newTaskTempFile with spec is the goal of this PR

@dongjoon-hyun, I got your point. I've changed the title to Implement and call new APIs in FileCommitProtocol instead of deprecated. Does the change here look reasonable to you with this positive tone? If still not, I will separate the overrides and callers into 2 PRs

Thank you. I trust your decision. Initially, I want to remove the default implementation, throw SparkException, from this PR. But, this could be a migration step too. So, I agree with your decision.

Thank you @dongjoon-hyun

dongjoon-hyun · 2025-08-02T01:19:35Z

core/src/main/scala/org/apache/spark/internal/io/FileCommitProtocol.scala

  @deprecated("use newTaskTempFile(..., spec: FileNameSpec) instead", "3.3.0")
-  def newTaskTempFile(taskContext: TaskAttemptContext, dir: Option[String], ext: String): String
+  def newTaskTempFile(taskContext: TaskAttemptContext, dir: Option[String], ext: String): String = {
+    throw SparkException.mustOverrideOneMethodError("newTaskTempFile")


In other words, I don't think this is required to achieve your goal.

I agree, it is not clear why these changes are being made.
I got confused with the intent as well - though now I realize what is being attempted.

dongjoon-hyun · 2025-08-02T01:19:49Z

core/src/main/scala/org/apache/spark/internal/io/FileCommitProtocol.scala

  def newTaskTempFileAbsPath(
-      taskContext: TaskAttemptContext, absoluteDir: String, ext: String): String
+      taskContext: TaskAttemptContext, absoluteDir: String, ext: String): String = {
+    throw SparkException.mustOverrideOneMethodError("newTaskTempFileAbsPath")


sql/core/src/test/scala/org/apache/spark/sql/sources/PartitionedWriteSuite.scala

yaooqinn · 2025-08-04T05:57:05Z

Merged to master, thank you @dongjoon-hyun again.

common/utils/src/main/scala/org/apache/spark/SparkException.scala

mridulm · 2025-08-07T06:43:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

-    new SparkRuntimeException(
-      "INTERNAL_ERROR",
-      Map("message" -> msg))
+    SparkException.mustOverrideOneMethodError(msg)


This should be SparkException.mustOverrideOneMethodError(methodName) ? Or better still remove mustOverrideOneMethodError and delegate to SparkException.mustOverrideOneMethodError insead ?

mridulm · 2025-08-07T06:48:20Z

It is unclear what the benefits of this PR are.
Note that these classes get extended by users for customizing behavior, and I am not completely certain if there is parity in functionality after this PR.

yaooqinn · 2025-08-07T07:02:13Z

Hi @mridulm,

If we implement a new Custom-FileCommitProtocol, we don't need to override the deprecated newTaskTempFile anymore. This makes the future removal of these APIs safer.

mridulm · 2025-08-07T07:20:11Z

There are unfortunately nontrivial number of user/library implementations which override from these public classes (a github search shows a lot - even if we exclude forlks, etc there appears to be significant count), and depend on the way they were currently written (for better or for worse).
I have not looked in detail at whether the changes in this PR will not cause regression - but please do evaluate with that lens : if there is nontrivial value, we can definitely enforce deprecation - but otherwise, we should be careful not to break compatibility

yaooqinn · 2025-08-07T07:29:33Z

As long as the base interface can satisfy the current built-in implementations, there's no difference for third-party implementations

[SPARK-53063][CORE] Remove inner references of deprecated APIs in Fil…

f7fa560

…eCommitProtocol

github-actions bot added SQL STRUCTURED STREAMING CORE labels Aug 1, 2025

yaooqinn requested a review from cloud-fan August 1, 2025 16:17

dongjoon-hyun reviewed Aug 1, 2025

View reviewed changes

...main/scala/org/apache/spark/sql/execution/streaming/runtime/ManifestFileCommitProtocol.scala Show resolved Hide resolved

dongjoon-hyun reviewed Aug 1, 2025

View reviewed changes

dongjoon-hyun reviewed Aug 2, 2025

View reviewed changes

yaooqinn changed the title ~~[SPARK-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol~~ [SPARK-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of deprecated Aug 4, 2025

yaooqinn changed the title ~~[SPARK-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of deprecated~~ [SPARK-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated Aug 4, 2025

dongjoon-hyun approved these changes Aug 4, 2025

View reviewed changes

yaooqinn commented Aug 4, 2025

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/sources/PartitionedWriteSuite.scala Outdated Show resolved Hide resolved

yaooqinn commented Aug 4, 2025

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/sources/PartitionedWriteSuite.scala Outdated Show resolved Hide resolved

Apply suggestion from @yaooqinn

fa54cdc

yaooqinn closed this in c9b85a2 Aug 4, 2025

yaooqinn deleted the SPARK-53063 branch August 4, 2025 05:56

mridulm reviewed Aug 7, 2025

View reviewed changes

common/utils/src/main/scala/org/apache/spark/SparkException.scala Show resolved Hide resolved

mridulm reviewed Aug 7, 2025

View reviewed changes

[SPARK-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated #51772

[SPARK-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated #51772

Uh oh!

Conversation

yaooqinn commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yaooqinn commented Aug 4, 2025

Uh oh!

Uh oh!

mridulm Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mridulm commented Aug 7, 2025

Uh oh!

yaooqinn commented Aug 7, 2025

Uh oh!

mridulm commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaooqinn commented Aug 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yaooqinn commented Aug 1, 2025 •

edited

Loading

dongjoon-hyun Aug 4, 2025 •

edited

Loading

dongjoon-hyun Aug 2, 2025 •

edited

Loading

mridulm Aug 7, 2025 •

edited

Loading

mridulm commented Aug 7, 2025 •

edited

Loading