[SPARK-52880][CORE] Improve `toString` by `JEP-280` instead of `ToStringBuilder` #51572

dongjoon-hyun · 2025-07-19T13:53:16Z

What changes were proposed in this pull request?

This PR aims to improve toString by JEP-280 instead of ToStringBuilder. In addition, Scalastyle and Checkstyle rules are added to prevent a future regression.

Why are the changes needed?

Since Java 9, String Concatenation has been handled better by default.

ID	DESCRIPTION
JEP-280	Indify String Concatenation

For example, this PR improves OpenBlocks like the following. Both Java source code and byte code are simplified a lot by utilizing JEP-280 properly.

CODE CHANGE

- return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE)
-   .append("appId", appId)
-   .append("execId", execId)
-   .append("blockIds", Arrays.toString(blockIds))
-   .toString();
+ return "OpenBlocks[appId=" + appId + ",execId=" + execId + ",blockIds=" +
+     Arrays.toString(blockIds) + "]";

BEFORE

  public java.lang.String toString();
    Code:
       0: new           #39                 // class org/apache/commons/lang3/builder/ToStringBuilder
       3: dup
       4: aload_0
       5: getstatic     #41                 // Field org/apache/commons/lang3/builder/ToStringStyle.SHORT_PREFIX_STYLE:Lorg/apache/commons/lang3/builder/ToStringStyle;
       8: invokespecial #47                 // Method org/apache/commons/lang3/builder/ToStringBuilder."<init>":(Ljava/lang/Object;Lorg/apache/commons/lang3/builder/ToStringStyle;)V
      11: ldc           #50                 // String appId
      13: aload_0
      14: getfield      #7                  // Field appId:Ljava/lang/String;
      17: invokevirtual #51                 // Method org/apache/commons/lang3/builder/ToStringBuilder.append:(Ljava/lang/String;Ljava/lang/Object;)Lorg/apache/commons/lang3/builder/ToStringBuilder;
      20: ldc           #55                 // String execId
      22: aload_0
      23: getfield      #13                 // Field execId:Ljava/lang/String;
      26: invokevirtual #51                 // Method org/apache/commons/lang3/builder/ToStringBuilder.append:(Ljava/lang/String;Ljava/lang/Object;)Lorg/apache/commons/lang3/builder/ToStringBuilder;
      29: ldc           #56                 // String blockIds
      31: aload_0
      32: getfield      #16                 // Field blockIds:[Ljava/lang/String;
      35: invokestatic  #57                 // Method java/util/Arrays.toString:([Ljava/lang/Object;)Ljava/lang/String;
      38: invokevirtual #51                 // Method org/apache/commons/lang3/builder/ToStringBuilder.append:(Ljava/lang/String;Ljava/lang/Object;)Lorg/apache/commons/lang3/builder/ToStringBuilder;
      41: invokevirtual #61                 // Method org/apache/commons/lang3/builder/ToStringBuilder.toString:()Ljava/lang/String;
      44: areturn

AFTER

  public java.lang.String toString();
    Code:
       0: aload_0
       1: getfield      #7                  // Field appId:Ljava/lang/String;
       4: aload_0
       5: getfield      #13                 // Field execId:Ljava/lang/String;
       8: aload_0
       9: getfield      #16                 // Field blockIds:[Ljava/lang/String;
      12: invokestatic  #39                 // Method java/util/Arrays.toString:([Ljava/lang/Object;)Ljava/lang/String;
      15: invokedynamic #43,  0             // InvokeDynamic #0:makeConcatWithConstants:(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;
      20: areturn

Does this PR introduce any user-facing change?

No. This is an toString implementation improvement.

How was this patch tested?

Pass the CIs.

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

No.

…gBuilder`

dongjoon-hyun · 2025-07-19T15:19:03Z

cc @cloud-fan , @HyukjinKwon , @zhengruifeng , @ueshin , @viirya , @MaxGekk , @gengliangwang , @yaooqinn , @LuciferYang , @panbingkun , @huaxingao , @peter-toth

LuciferYang · 2025-07-20T06:45:46Z

dev/checkstyle.xml

            <property name="format" value="new URL\("/>
            <property name="message" value="Use URI.toURL or URL.of instead of URL constructors." />
        </module>
+        <module name="RegexpSinglelineJava">


We should ban the use of org.apache.commons.lang.builder.ToStringBuilder simultaneously, even though it is not currently being used, because the commons-lang/2.6//commons-lang-2.6.jar is still a dependency of Spark.

Thank you for review, @LuciferYang . We already banned lang2 package completely.

spark/scalastyle-config.xml

Lines 285 to 289 in c717624

<check customId="commonslang2" level="error" class="org.scalastyle.file.RegexChecker" enabled="true">

<parameters><parameter name="regex">org\.apache\.commons\.lang\.</parameter></parameters>

<customMessage>Use Commons Lang 3 classes (package org.apache.commons.lang3.*) instead

of Commons Lang 2 (package org.apache.commons.lang.*)</customMessage>

</check>

LuciferYang · 2025-07-20T06:45:51Z

scalastyle-config.xml

    <customMessage>Use org.apache.spark.util.Pair instead</customMessage>
  </check>

+  <check customId="commonslang3tuple" level="error" class="org.scalastyle.file.RegexChecker" enabled="true">


viirya · 2025-07-20T07:07:21Z

common/network-common/src/main/java/org/apache/spark/network/client/TransportClient.java

+    return "TransportClient[remoteAddress=" + channel.remoteAddress() + "clientId=" + clientId +
+        "isActive=" + isActive() + "]";


Suggested change

return "TransportClient[remoteAddress=" + channel.remoteAddress() + "clientId=" + clientId +

"isActive=" + isActive() + "]";

return "TransportClient[remoteAddress=" + channel.remoteAddress() + ",clientId=" + clientId +

",isActive=" + isActive() + "]";

viirya · 2025-07-20T07:09:46Z

...shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/FetchShuffleBlockChunks.java

-      .toString();
+    return "FetchShuffleBlockChunks[appId=" + appId + ",execId=" + execId +
+        ",shuffleId=" + shuffleId + ",shuffleMergeId=" + shuffleMergeId +
+        ",reduceIds=" + Arrays.toString(reduceIds) + ",chunkIds=" + Arrays.toString(chunkIds) + "]";


Why changing chunkIds from Arrays.deepToString to Arrays.toString?

Oh, thank you. I'll fix it.

viirya · 2025-07-20T07:10:41Z

...work-shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/FetchShuffleBlocks.java

+    return "FetchShuffleBlocks[appId=" + appId + ",execId=" + execId + ",shuffleId=" + shuffleId +
+        ",mapIds=" + Arrays.toString(mapIds) + ",reduceIds=" + Arrays.deepToString(reduceIds) +
+        ",batchFetchEnabled=" + batchFetchEnabled + "]";


This adds more fields than before?

It came from toStringHelper() method at line 65. Technically, this PR removed toStringHelper usage.

spark/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/AbstractFetchShuffleBlocks.java

Lines 46 to 51 in c717624

public ToStringBuilder toStringHelper() {

return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE)

.append("appId", appId)

.append("execId", execId)

.append("shuffleId", shuffleId);

}

LuciferYang · 2025-07-20T13:35:00Z

...ffle/src/main/java/org/apache/spark/network/shuffle/protocol/AbstractFetchShuffleBlocks.java

  }

+  // checkstyle.off: RegexpSinglelineJava
  public ToStringBuilder toStringHelper() {


This should probably not be a public api, since deleting it doesn't cause a mima check failure.

Ya, I hope to remove this eventually later because this method is not used from now.

However, this PR aims to focus on toString method improvement only in order to avoid any discussion about this method toStringHelper.

spark/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/protocol/AbstractFetchShuffleBlocks.java

Lines 27 to 46 in c717624

/**

* Base class for fetch shuffle blocks and chunks.

*

* @since 3.2.0

*/

public abstract class AbstractFetchShuffleBlocks extends BlockTransferMessage {

public final String appId;

public final String execId;

public final int shuffleId;

protected AbstractFetchShuffleBlocks(

String appId,

String execId,

int shuffleId) {

this.appId = appId;

this.execId = execId;

this.shuffleId = shuffleId;

}

public ToStringBuilder toStringHelper() {

dongjoon-hyun · 2025-07-20T22:55:52Z

Thank you, @LuciferYang and @viirya . I addressed your comments and replied.

peter-toth

LGTM, just a nit that maybe we could use .getClass().getSimpleName()s instead of duplicating the class names.

dongjoon-hyun · 2025-07-21T15:37:50Z

Thank you, @peter-toth and @cloud-fan .

dongjoon-hyun · 2025-07-21T15:44:18Z

To @peter-toth , you are right that it's a generally good approach. Actually, I intentionally avoid that generalized pattern here due to the nature of three additional operations; two additional invokevirtuals and one additional string argument at makeConcatWithConstants at the end. For this PR, let's keep toString as simple/faster as possible for now because this is Spark internals. We may want to generalize it later.

       0: aload_0
       1: invokevirtual #39                 // Method java/lang/Object.getClass:()Ljava/lang/Class;
       4: invokevirtual #43                 // Method java/lang/Class.getSimpleName:()Ljava/lang/String;

dongjoon-hyun · 2025-07-21T15:46:49Z

Merged to master for Apache Spark 4.1.0.

Thank you, @LuciferYang , @viirya , @peter-toth , @cloud-fan , @MaxGekk !

### What changes were proposed in this pull request? Improve `toString` by JEP-280 instead of `ToStringBuilder`. ### Why are the changes needed? Since Java 9, String Concatenation has been handled better by default. ID | DESCRIPTION -- | -- JEP-280 | [Indify String Concatenation](https://openjdk.org/jeps/280) Backport apache/spark#51572. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI. Closes #3380 from SteNicholas/CELEBORN-2077. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: Wang, Fei <fwang12@ebay.com>

Improve `toString` by JEP-280 instead of `ToStringBuilder`. Since Java 9, String Concatenation has been handled better by default. ID | DESCRIPTION -- | -- JEP-280 | [Indify String Concatenation](https://openjdk.org/jeps/280) Backport apache/spark#51572. No. CI. Closes #3380 from SteNicholas/CELEBORN-2077. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: Wang, Fei <fwang12@ebay.com> (cherry picked from commit 66856f2) Signed-off-by: Wang, Fei <fwang12@ebay.com>

…tead of `ToStringBuilder` ### What changes were proposed in this pull request? This PR aims to improve `SentinelResourceState.toString` by JEP-280 instead of `ToStringBuilder`. ### Why are the changes needed? This is aligned with Apache Spark main repository improvement. - apache/spark#51572 Since Java 9, `String Concatenation` has been handled better by default. | ID | DESCRIPTION | | - | - | | JEP-280 | [Indify String Concatenation](https://openjdk.org/jeps/280) | For example, `SentinelResourceState.toString` is changed like the following by this PR. **BEFORE** ``` public java.lang.String toString(); Code: 0: new #43 // class org/apache/commons/lang3/builder/ToStringBuilder 3: dup 4: aload_0 5: invokespecial #45 // Method org/apache/commons/lang3/builder/ToStringBuilder."<init>":(Ljava/lang/Object;)V 8: ldc #48 // String resource 10: aload_0 11: getfield #17 // Field resource:Lorg/apache/spark/k8s/operator/BaseResource; 14: invokevirtual #49 // Method org/apache/commons/lang3/builder/ToStringBuilder.append:(Ljava/lang/String;Ljava/lang/Object;)Lorg/apache/commons/lang3/builder/ToStringBuilder; 17: ldc #53 // String previousGeneration 19: aload_0 20: getfield #39 // Field previousGeneration:J 23: invokevirtual #54 // Method org/apache/commons/lang3/builder/ToStringBuilder.append:(Ljava/lang/String;J)Lorg/apache/commons/lang3/builder/ToStringBuilder; 26: ldc #57 // String isHealthy 28: aload_0 29: getfield #13 // Field isHealthy:Z 32: invokevirtual #58 // Method org/apache/commons/lang3/builder/ToStringBuilder.append:(Ljava/lang/String;Z)Lorg/apache/commons/lang3/builder/ToStringBuilder; 35: invokevirtual #61 // Method org/apache/commons/lang3/builder/ToStringBuilder.toString:()Ljava/lang/String; 38: areturn ``` **AFTER** ``` public java.lang.String toString(); Code: 0: aload_0 1: getfield #17 // Field resource:Lorg/apache/spark/k8s/operator/BaseResource; 4: invokestatic #43 // Method java/lang/String.valueOf:(Ljava/lang/Object;)Ljava/lang/String; 7: aload_0 8: getfield #39 // Field previousGeneration:J 11: aload_0 12: getfield #13 // Field isHealthy:Z 15: invokedynamic #49, 0 // InvokeDynamic #0:makeConcatWithConstants:(Ljava/lang/String;JZ)Ljava/lang/String; 20: areturn ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #369 from dongjoon-hyun/SPARK-53818. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

[SPARK-52880][CORE] Improve toString by JEP-280 instead of `ToStrin…

429e7b4

…gBuilder`

github-actions bot added SQL BUILD CORE labels Jul 19, 2025

dongjoon-hyun changed the title ~~[SPARK-52880][CORE] Improve toString by JEP-280 instead of ToStringBuilder~~ [SPARK-52880][CORE] Improve toString by JEP-280 instead of ToStringBuilder Jul 19, 2025

LuciferYang approved these changes Jul 20, 2025

View reviewed changes

viirya reviewed Jul 20, 2025

View reviewed changes

LuciferYang reviewed Jul 20, 2025

View reviewed changes

Address comments

ea75ba0

peter-toth approved these changes Jul 21, 2025

View reviewed changes

cloud-fan approved these changes Jul 21, 2025

View reviewed changes

MaxGekk approved these changes Jul 21, 2025

View reviewed changes

dongjoon-hyun closed this in 27dcbcd Jul 21, 2025

dongjoon-hyun deleted the SPARK-52880 branch July 21, 2025 15:46

SteNicholas mentioned this pull request Jul 22, 2025

[CELEBORN-2077] Improve toString by JEP-280 instead of ToStringBuilder apache/celeborn#3380

Closed

dongjoon-hyun mentioned this pull request Aug 28, 2025

[SPARK-53325] Support Prometheus 2.0 text-based-format and best practices for metrics naming apache/spark-kubernetes-operator#298

Closed

dongjoon-hyun mentioned this pull request Oct 7, 2025

[SPARK-53818] Improve SentinelResourceState.toString by JEP-280 instead of ToStringBuilder apache/spark-kubernetes-operator#369

Closed

	<check customId="commonslang2" level="error" class="org.scalastyle.file.RegexChecker" enabled="true">
	<parameters><parameter name="regex">org\.apache\.commons\.lang\.</parameter></parameters>
	<customMessage>Use Commons Lang 3 classes (package org.apache.commons.lang3.*) instead
	of Commons Lang 2 (package org.apache.commons.lang.*)</customMessage>
	</check>

		return "TransportClient[remoteAddress=" + channel.remoteAddress() + "clientId=" + clientId +
		"isActive=" + isActive() + "]";

	public ToStringBuilder toStringHelper() {
	return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE)
	.append("appId", appId)
	.append("execId", execId)
	.append("shuffleId", shuffleId);
	}

	/**
	* Base class for fetch shuffle blocks and chunks.
	*
	* @since 3.2.0
	*/
	public abstract class AbstractFetchShuffleBlocks extends BlockTransferMessage {
	public final String appId;
	public final String execId;
	public final int shuffleId;

	protected AbstractFetchShuffleBlocks(
	String appId,
	String execId,
	int shuffleId) {
	this.appId = appId;
	this.execId = execId;
	this.shuffleId = shuffleId;
	}

	public ToStringBuilder toStringHelper() {

[SPARK-52880][CORE] Improve toString by JEP-280 instead of ToStringBuilder #51572

[SPARK-52880][CORE] Improve toString by JEP-280 instead of ToStringBuilder #51572

Uh oh!

Conversation

dongjoon-hyun commented Jul 19, 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!

dongjoon-hyun commented Jul 19, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 20, 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!

dongjoon-hyun commented Jul 20, 2025

Uh oh!

peter-toth left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 21, 2025

Uh oh!

dongjoon-hyun commented Jul 21, 2025

Uh oh!

dongjoon-hyun commented Jul 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[SPARK-52880][CORE] Improve `toString` by `JEP-280` instead of `ToStringBuilder` #51572

[SPARK-52880][CORE] Improve `toString` by `JEP-280` instead of `ToStringBuilder` #51572

dongjoon-hyun commented Jul 19, 2025 •

edited

Loading

dongjoon-hyun Jul 20, 2025 •

edited

Loading