-
Notifications
You must be signed in to change notification settings - Fork 28.7k
[SPARK-52880][CORE] Improve toString
by JEP-280
instead of ToStringBuilder
#51572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
toString
by JEP-280 instead of ToStringBuilder
toString
by JEP-280
instead of ToStringBuilder
cc @cloud-fan , @HyukjinKwon , @zhengruifeng , @ueshin , @viirya , @MaxGekk , @gengliangwang , @yaooqinn , @LuciferYang , @panbingkun , @huaxingao , @peter-toth |
@@ -190,6 +190,10 @@ | |||
<property name="format" value="new URL\("/> | |||
<property name="message" value="Use URI.toURL or URL.of instead of URL constructors." /> | |||
</module> | |||
<module name="RegexpSinglelineJava"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for review, @LuciferYang . We already banned lang2
package completely.
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> |
@@ -299,6 +299,11 @@ This file is divided into 3 sections: | |||
<customMessage>Use org.apache.spark.util.Pair instead</customMessage> | |||
</check> | |||
|
|||
<check customId="commonslang3tuple" level="error" class="org.scalastyle.file.RegexChecker" enabled="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
return "TransportClient[remoteAddress=" + channel.remoteAddress() + "clientId=" + clientId + | ||
"isActive=" + isActive() + "]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return "TransportClient[remoteAddress=" + channel.remoteAddress() + "clientId=" + clientId + | |
"isActive=" + isActive() + "]"; | |
return "TransportClient[remoteAddress=" + channel.remoteAddress() + ",clientId=" + clientId + | |
",isActive=" + isActive() + "]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
.toString(); | ||
return "FetchShuffleBlockChunks[appId=" + appId + ",execId=" + execId + | ||
",shuffleId=" + shuffleId + ",shuffleMergeId=" + shuffleMergeId + | ||
",reduceIds=" + Arrays.toString(reduceIds) + ",chunkIds=" + Arrays.toString(chunkIds) + "]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why changing chunkIds
from Arrays.deepToString
to Arrays.toString
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thank you. I'll fix it.
return "FetchShuffleBlocks[appId=" + appId + ",execId=" + execId + ",shuffleId=" + shuffleId + | ||
",mapIds=" + Arrays.toString(mapIds) + ",reduceIds=" + Arrays.deepToString(reduceIds) + | ||
",batchFetchEnabled=" + batchFetchEnabled + "]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds more fields than before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It came from toStringHelper()
method at line 65. Technically, this PR removed toStringHelper
usage.
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); | |
} |
@@ -43,12 +43,14 @@ protected AbstractFetchShuffleBlocks( | |||
this.shuffleId = shuffleId; | |||
} | |||
|
|||
// checkstyle.off: RegexpSinglelineJava | |||
public ToStringBuilder toStringHelper() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably not be a public api, since deleting it doesn't cause a mima check failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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() { |
Thank you, @LuciferYang and @viirya . I addressed your comments and replied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a nit that maybe we could use .getClass().getSimpleName()
s instead of duplicating the class names.
Thank you, @peter-toth and @cloud-fan . |
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
|
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?
This PR aims to improve
toString
byJEP-280
instead ofToStringBuilder
. In addition,Scalastyle
andCheckstyle
rules are added to prevent a future regression.Why are the changes needed?
Since Java 9,
String Concatenation
has been handled better by default.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
BEFORE
AFTER
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.