You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "yaooqinn (via GitHub)" <gi...@apache.org> on 2025/08/01 06:45:18 UTC
[PR] [SPARK-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
yaooqinn opened a new pull request, #51772:
URL: https://github.com/apache/spark/pull/51772
### What changes were proposed in this pull request?
This PR removes inner references of deprecated APIs in FileCommitProtocol
### 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?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
no
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #51772:
URL: https://github.com/apache/spark/pull/51772#issuecomment-3162839764
There are unfortunately hundreds of user/library implementations which override from these public classes, 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.
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #51772:
URL: https://github.com/apache/spark/pull/51772#issuecomment-3162727862
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.
--
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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2248869018
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala:
##########
@@ -172,7 +172,7 @@ class SingleDirectoryDataWriter(
val currentPath = committer.newTaskTempFile(
taskAttemptContext,
None,
- f"-c$fileCounter%03d" + ext)
+ FileNameSpec("", f"-c$fileCounter%03d" + ext))
Review Comment:
Is this relevant to 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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2249048598
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala:
##########
@@ -172,7 +172,7 @@ class SingleDirectoryDataWriter(
val currentPath = committer.newTaskTempFile(
taskAttemptContext,
None,
- f"-c$fileCounter%03d" + ext)
+ FileNameSpec("", f"-c$fileCounter%03d" + ext))
Review Comment:
Calling the newer newTaskTempFile with spec is the goal of 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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2249048078
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/runtime/ManifestFileCommitProtocol.scala:
##########
@@ -114,13 +114,13 @@ class ManifestFileCommitProtocol(jobId: String, path: String)
}
override def newTaskTempFile(
- taskContext: TaskAttemptContext, dir: Option[String], ext: String): String = {
+ taskContext: TaskAttemptContext, dir: Option[String], spec: FileNameSpec): String = {
Review Comment:
This is a necessary step， we need to override the new method
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2259241880
##########
common/utils/src/main/scala/org/apache/spark/SparkException.scala:
##########
@@ -131,6 +131,14 @@ object SparkException {
messageParameters: java.util.Map[String, String]): Map[String, String] = {
messageParameters.asScala.toMap
}
+
+ def mustOverrideOneMethodError(methodName: String): RuntimeException = {
+ val msg = s"You must override one `$methodName`. It's preferred to not override the " +
+ "deprecated one."
Review Comment:
Resolving this - I dont think this is adding value, and is a bit confusing.
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2259235355
##########
common/utils/src/main/scala/org/apache/spark/SparkException.scala:
##########
@@ -131,6 +131,14 @@ object SparkException {
messageParameters: java.util.Map[String, String]): Map[String, String] = {
messageParameters.asScala.toMap
}
+
+ def mustOverrideOneMethodError(methodName: String): RuntimeException = {
+ val msg = s"You must override one `$methodName`. It's preferred to not override the " +
+ "deprecated one."
Review Comment:
The exception says `preferred` - and yet we are enforcing it by failing ?
--
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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2249051888
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/runtime/ManifestFileCommitProtocol.scala:
##########
@@ -114,13 +114,13 @@ class ManifestFileCommitProtocol(jobId: String, path: String)
}
override def newTaskTempFile(
- taskContext: TaskAttemptContext, dir: Option[String], ext: String): String = {
+ taskContext: TaskAttemptContext, dir: Option[String], spec: FileNameSpec): String = {
Review Comment:
Yes, @yaooqinn . However, we don't need to remove inner references of deprecated APIs when we switch API in this layer. That's the reason why I requested to spin off. We can throw exception in this PR but not in the spin-off 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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2248872051
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/runtime/ManifestFileCommitProtocol.scala:
##########
@@ -114,13 +114,13 @@ class ManifestFileCommitProtocol(jobId: String, path: String)
}
override def newTaskTempFile(
- taskContext: TaskAttemptContext, dir: Option[String], ext: String): String = {
+ taskContext: TaskAttemptContext, dir: Option[String], spec: FileNameSpec): String = {
Review Comment:
Hi, @yaooqinn . Can we spin off this change independently? We had better focus on `ManifestFileCommitProtocol` change.
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2250391430
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala:
##########
@@ -172,7 +172,7 @@ class SingleDirectoryDataWriter(
val currentPath = committer.newTaskTempFile(
taskAttemptContext,
None,
- f"-c$fileCounter%03d" + ext)
+ FileNameSpec("", f"-c$fileCounter%03d" + ext))
Review Comment:
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.
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #51772:
URL: https://github.com/apache/spark/pull/51772#issuecomment-3162766469
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.
--
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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2248869018
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala:
##########
@@ -172,7 +172,7 @@ class SingleDirectoryDataWriter(
val currentPath = committer.newTaskTempFile(
taskAttemptContext,
None,
- f"-c$fileCounter%03d" + ext)
+ FileNameSpec("", f"-c$fileCounter%03d" + ext))
Review Comment:
Is this relevant to 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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2250460988
##########
sql/core/src/test/scala/org/apache/spark/sql/sources/PartitionedWriteSuite.scala:
##########
@@ -42,7 +43,7 @@ private class OnlyDetectCustomPathFileCommitProtocol(jobId: String, path: String
with Serializable with Logging {
override def newTaskTempFileAbsPath(
- taskContext: TaskAttemptContext, absoluteDir: String, ext: String): String = {
+ taskContext: TaskAttemptContext, absoluteDir: String, spec: FileNameSpec): String = {
Review Comment:
```suggestion
taskContext: TaskAttemptContext, absoluteDir: String, spec: FileNameSpec): String = {
```
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #51772:
URL: https://github.com/apache/spark/pull/51772#issuecomment-3149222877
Merged to master, thank you @dongjoon-hyun again.
--
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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2248872790
##########
sql/core/src/test/scala/org/apache/spark/sql/sources/PartitionedWriteSuite.scala:
##########
@@ -42,7 +43,7 @@ private class OnlyDetectCustomPathFileCommitProtocol(jobId: String, path: String
with Serializable with Logging {
override def newTaskTempFileAbsPath(
- taskContext: TaskAttemptContext, absoluteDir: String, ext: String): String = {
+ taskContext: TaskAttemptContext, absoluteDir: String, spec: FileNameSpec): String = {
Review Comment:
nit. A wrong indentation.
--
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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2249052129
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala:
##########
@@ -172,7 +172,7 @@ class SingleDirectoryDataWriter(
val currentPath = committer.newTaskTempFile(
taskAttemptContext,
None,
- f"-c$fileCounter%03d" + ext)
+ FileNameSpec("", f"-c$fileCounter%03d" + ext))
Review Comment:
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
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2259240875
##########
core/src/main/scala/org/apache/spark/internal/io/FileCommitProtocol.scala:
##########
@@ -96,7 +97,9 @@ abstract class FileCommitProtocol extends Logging {
* guarantees that files written by different tasks will not conflict.
*/
@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")
Review Comment:
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.
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2259246614
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -4203,9 +4203,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
def mustOverrideOneMethodError(methodName: String): RuntimeException = {
val msg = s"You must override one `$methodName`. It's preferred to not override the " +
"deprecated one."
- new SparkRuntimeException(
- "INTERNAL_ERROR",
- Map("message" -> msg))
+ SparkException.mustOverrideOneMethodError(msg)
Review Comment:
This should be `SparkException.mustOverrideOneMethodError(methodName)` ?
--
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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2249051888
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/runtime/ManifestFileCommitProtocol.scala:
##########
@@ -114,13 +114,13 @@ class ManifestFileCommitProtocol(jobId: String, path: String)
}
override def newTaskTempFile(
- taskContext: TaskAttemptContext, dir: Option[String], ext: String): String = {
+ taskContext: TaskAttemptContext, dir: Option[String], spec: FileNameSpec): String = {
Review Comment:
Yes, @yaooqinn . However, we don't need to remove inner references of deprecated APIs when we switch APIs in this layer. That's the reason why I requested to spin off. We can throw exception in this PR but not in the spin-off 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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2249053383
##########
core/src/main/scala/org/apache/spark/internal/io/FileCommitProtocol.scala:
##########
@@ -96,7 +97,9 @@ abstract class FileCommitProtocol extends Logging {
* guarantees that files written by different tasks will not conflict.
*/
@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")
Review Comment:
In other way, I don't think this is required to achieve your goal.
##########
core/src/main/scala/org/apache/spark/internal/io/FileCommitProtocol.scala:
##########
@@ -96,7 +97,9 @@ abstract class FileCommitProtocol extends Logging {
* guarantees that files written by different tasks will not conflict.
*/
@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")
Review Comment:
In other words, I don't think this is required to achieve your goal.
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2250461385
##########
sql/core/src/test/scala/org/apache/spark/sql/sources/PartitionedWriteSuite.scala:
##########
@@ -42,7 +43,7 @@ private class OnlyDetectCustomPathFileCommitProtocol(jobId: String, path: String
with Serializable with Logging {
override def newTaskTempFileAbsPath(
- taskContext: TaskAttemptContext, absoluteDir: String, ext: String): String = {
+ taskContext: TaskAttemptContext, absoluteDir: String, spec: FileNameSpec): String = {
Review Comment:
```suggestion
taskContext: TaskAttemptContext, absoluteDir: String, spec: FileNameSpec): String = {
```
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2259241880
##########
common/utils/src/main/scala/org/apache/spark/SparkException.scala:
##########
@@ -131,6 +131,14 @@ object SparkException {
messageParameters: java.util.Map[String, String]): Map[String, String] = {
messageParameters.asScala.toMap
}
+
+ def mustOverrideOneMethodError(methodName: String): RuntimeException = {
+ val msg = s"You must override one `$methodName`. It's preferred to not override the " +
+ "deprecated one."
Review Comment:
Resolving this - I dont think this is adding value, and is a bit confusing.
Overrides will go through anyway
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #51772:
URL: https://github.com/apache/spark/pull/51772#issuecomment-3162874497
As long as the base interface can satisfy the current built-in implementations, there's no difference for third-party implementations
--
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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2249053451
##########
core/src/main/scala/org/apache/spark/internal/io/FileCommitProtocol.scala:
##########
@@ -135,7 +138,9 @@ abstract class FileCommitProtocol extends Logging {
*/
@deprecated("use newTaskTempFileAbsPath(..., spec: FileNameSpec) instead", "3.3.0")
def newTaskTempFileAbsPath(
- taskContext: TaskAttemptContext, absoluteDir: String, ext: String): String
+ taskContext: TaskAttemptContext, absoluteDir: String, ext: String): String = {
+ throw SparkException.mustOverrideOneMethodError("newTaskTempFileAbsPath")
Review Comment:
ditto.
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of deprecated [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2250273184
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala:
##########
@@ -172,7 +172,7 @@ class SingleDirectoryDataWriter(
val currentPath = committer.newTaskTempFile(
taskAttemptContext,
None,
- f"-c$fileCounter%03d" + ext)
+ FileNameSpec("", f"-c$fileCounter%03d" + ext))
Review Comment:
@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
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2250391430
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala:
##########
@@ -172,7 +172,7 @@ class SingleDirectoryDataWriter(
val currentPath = committer.newTaskTempFile(
taskAttemptContext,
None,
- f"-c$fileCounter%03d" + ext)
+ FileNameSpec("", f"-c$fileCounter%03d" + ext))
Review Comment:
Thank you. I trust your decision. Initially, I want to remove the default implementation, `throw SparkException`, from 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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2250462498
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala:
##########
@@ -172,7 +172,7 @@ class SingleDirectoryDataWriter(
val currentPath = committer.newTaskTempFile(
taskAttemptContext,
None,
- f"-c$fileCounter%03d" + ext)
+ FileNameSpec("", f"-c$fileCounter%03d" + ext))
Review Comment:
Thank you @dongjoon-hyun
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn closed pull request #51772: [SPARK-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated
URL: https://github.com/apache/spark/pull/51772
--
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-53063][CORE] Remove inner references of deprecated APIs in FileCommitProtocol [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2248874897
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatDataWriter.scala:
##########
@@ -172,7 +172,7 @@ class SingleDirectoryDataWriter(
val currentPath = committer.newTaskTempFile(
taskAttemptContext,
None,
- f"-c$fileCounter%03d" + ext)
+ FileNameSpec("", f"-c$fileCounter%03d" + ext))
Review Comment:
This looks like a new independent change, too.
--
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-53063][CORE] Implement and call new APIs in FileCommitProtocol instead of the deprecated [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #51772:
URL: https://github.com/apache/spark/pull/51772#discussion_r2259246614
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -4203,9 +4203,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
def mustOverrideOneMethodError(methodName: String): RuntimeException = {
val msg = s"You must override one `$methodName`. It's preferred to not override the " +
"deprecated one."
- new SparkRuntimeException(
- "INTERNAL_ERROR",
- Map("message" -> msg))
+ SparkException.mustOverrideOneMethodError(msg)
Review Comment:
This should be `SparkException.mustOverrideOneMethodError(methodName)` ? Or better still remove `mustOverrideOneMethodError` and delegate to `SparkException.mustOverrideOneMethodError` insead ?
--
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