Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
dongjoon-hyun commented on PR #43796: URL: https://github.com/apache/spark/pull/43796#issuecomment-1813954802 Merged to master. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
dongjoon-hyun closed pull request #43796: [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition URL: https://github.com/apache/spark/pull/43796 -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on PR #43796: URL: https://github.com/apache/spark/pull/43796#issuecomment-1813710853 rebased -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on PR #43796: URL: https://github.com/apache/spark/pull/43796#issuecomment-1812528892 wait https://github.com/apache/spark/pull/43817 -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on code in PR #43796:
URL: https://github.com/apache/spark/pull/43796#discussion_r1394074332
##
common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java:
##
@@ -1612,19 +1612,8 @@ private void verifyMetrics(
assertEquals(expectedIgnoredBlocksBytes, ignoredBlockBytes.getCount(),
"ignored block bytes");
}
- private static class PushBlock {
-private final int shuffleId;
-private final int shuffleMergeId;
-private final int mapIndex;
-private final int reduceId;
-private final ByteBuffer buffer;
-PushBlock(int shuffleId, int shuffleMergeId, int mapIndex, int reduceId,
ByteBuffer buffer) {
- this.shuffleId = shuffleId;
- this.shuffleMergeId = shuffleMergeId;
- this.mapIndex = mapIndex;
- this.reduceId = reduceId;
- this.buffer = buffer;
-}
+ private record PushBlock(
+ int shuffleId, int shuffleMergeId, int mapIndex, int reduceId,
ByteBuffer buffer) {
Review Comment:
done
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on code in PR #43796:
URL: https://github.com/apache/spark/pull/43796#discussion_r1394064308
##
common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java:
##
@@ -153,24 +153,6 @@ private static ShuffleServiceMetricsInfo
getShuffleServiceMetricsInfoForGenericV
valueName + " value of " + baseName);
}
- private static class ShuffleServiceMetricsInfo implements MetricsInfo {
Review Comment:
Yes, Modifier `static` is redundant for inner records
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on code in PR #43796:
URL: https://github.com/apache/spark/pull/43796#discussion_r1394067701
##
common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java:
##
@@ -1612,19 +1612,8 @@ private void verifyMetrics(
assertEquals(expectedIgnoredBlocksBytes, ignoredBlockBytes.getCount(),
"ignored block bytes");
}
- private static class PushBlock {
-private final int shuffleId;
-private final int shuffleMergeId;
-private final int mapIndex;
-private final int reduceId;
-private final ByteBuffer buffer;
-PushBlock(int shuffleId, int shuffleMergeId, int mapIndex, int reduceId,
ByteBuffer buffer) {
- this.shuffleId = shuffleId;
- this.shuffleMergeId = shuffleMergeId;
- this.mapIndex = mapIndex;
- this.reduceId = reduceId;
- this.buffer = buffer;
-}
+ private record PushBlock(
+ int shuffleId, int shuffleMergeId, int mapIndex, int reduceId,
ByteBuffer buffer) {
Review Comment:
Ok, let me revert this one. The previous commit did not revert it due to
this in test case.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on code in PR #43796:
URL: https://github.com/apache/spark/pull/43796#discussion_r1394064308
##
common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java:
##
@@ -153,24 +153,6 @@ private static ShuffleServiceMetricsInfo
getShuffleServiceMetricsInfoForGenericV
valueName + " value of " + baseName);
}
- private static class ShuffleServiceMetricsInfo implements MetricsInfo {
Review Comment:
Yes, Modifier 'static' is redundant for inner records
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
dongjoon-hyun commented on code in PR #43796:
URL: https://github.com/apache/spark/pull/43796#discussion_r1394022682
##
common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java:
##
@@ -153,24 +153,6 @@ private static ShuffleServiceMetricsInfo
getShuffleServiceMetricsInfoForGenericV
valueName + " value of " + baseName);
}
- private static class ShuffleServiceMetricsInfo implements MetricsInfo {
Review Comment:
`static class` is equivalent to `record`? Specifically, `static` part.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
dongjoon-hyun commented on code in PR #43796:
URL: https://github.com/apache/spark/pull/43796#discussion_r1394018312
##
common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java:
##
@@ -1612,19 +1612,8 @@ private void verifyMetrics(
assertEquals(expectedIgnoredBlocksBytes, ignoredBlockBytes.getCount(),
"ignored block bytes");
}
- private static class PushBlock {
-private final int shuffleId;
-private final int shuffleMergeId;
-private final int mapIndex;
-private final int reduceId;
-private final ByteBuffer buffer;
-PushBlock(int shuffleId, int shuffleMergeId, int mapIndex, int reduceId,
ByteBuffer buffer) {
- this.shuffleId = shuffleId;
- this.shuffleMergeId = shuffleMergeId;
- this.mapIndex = mapIndex;
- this.reduceId = reduceId;
- this.buffer = buffer;
-}
+ private record PushBlock(
+ int shuffleId, int shuffleMergeId, int mapIndex, int reduceId,
ByteBuffer buffer) {
Review Comment:
These fields should be `private`, shouldn't these?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on PR #43796: URL: https://github.com/apache/spark/pull/43796#issuecomment-1812139559 > > @dongjoon-hyun I want to clarify the issue. We don't want to use `record` here because `field` in the original class doesn't provide an Accessor, but since `record` automatically generates an Accessor method, it exposes the information of `field`. So, in similar scenarios, we shouldn't use `record`, right? > > Yes, exactly. That's the point. Is it ok now? -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
dongjoon-hyun commented on PR #43796: URL: https://github.com/apache/spark/pull/43796#issuecomment-1812013863 > @dongjoon-hyun I want to clarify the issue. We don't want to use `record` here because `field` in the original class doesn't provide an Accessor, but since `record` automatically generates an Accessor method, it exposes the information of `field`. So, in similar scenarios, we shouldn't use `record`, right? Yes, exactly. That's the point. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on PR #43796: URL: https://github.com/apache/spark/pull/43796#issuecomment-1811959949 @dongjoon-hyun I want to clarify the issue. We don't want to use `record` here because `field` in the original class doesn't provide an Accessor, but since `record` automatically generates an Accessor method, it exposes the information of `field`. So, in similar scenarios, we shouldn't use `record`, right? -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on PR #43796:
URL: https://github.com/apache/spark/pull/43796#issuecomment-1811951525
> This is a nice syntax in general, @LuciferYang .
>
> However, we cannot use this when the class provides information hiding.
>
> ```java
> jshell> private static class FieldAccessor { private final String field;
FieldAccessor(String field) { this.field = field; } }
> | created class FieldAccessor
>
> jshell> var a = new FieldAccessor("secret")
> a ==> FieldAccessor@12edcd21
>
> jshell> a.field
> | Error:
> | field has private access in FieldAccessor
> | a.field
> | ^-^
>
> jshell> private record FieldAccessor2(String field) {}
> | created record FieldAccessor2
>
> jshell> var b = new FieldAccessor2("secret")
> b ==> FieldAccessor2[field=secret]
>
> jshell> b.field()
> $5 ==> "secret"
> ```
>
> Could you remove all changes like the above?
OK， let me double check this
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
LuciferYang commented on code in PR #43796:
URL: https://github.com/apache/spark/pull/43796#discussion_r1393752725
##
common/network-common/src/main/java/org/apache/spark/network/protocol/StreamChunkId.java:
##
@@ -26,14 +26,7 @@
/**
* Encapsulates a request for a particular chunk of a stream.
*/
-public final class StreamChunkId implements Encodable {
Review Comment:
Yes
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-45919][CORE][SQL] Use Java 16 `record` to simplify Java class definition [spark]
dongjoon-hyun commented on code in PR #43796:
URL: https://github.com/apache/spark/pull/43796#discussion_r1393751280
##
common/network-common/src/main/java/org/apache/spark/network/protocol/StreamChunkId.java:
##
@@ -26,14 +26,7 @@
/**
* Encapsulates a request for a particular chunk of a stream.
*/
-public final class StreamChunkId implements Encodable {
Review Comment:
Although this is `public`, this is not a part of our public Java doc, right?
- https://spark.apache.org/docs/latest/api/java/index.html
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
