You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/12/31 03:51:02 UTC

[GitHub] [arrow] liyafan82 opened a new pull request #9053: ARROW-11081: [Java] Make IPC option immutable

liyafan82 opened a new pull request #9053:
URL: https://github.com/apache/arrow/pull/9053


   By making it immutable, the following benefits can be obtained:
   
   1. It makes the code easier to reason about.
   2. It allows JIT to make more optimizations.
   3. Immutable objects can be shared, so many object allocations can be avoided.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] emkornfield commented on a change in pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#discussion_r551600824



##########
File path: java/flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java
##########
@@ -194,10 +194,9 @@ public ArrowMessage(FlightDescriptor descriptor) {
   private ArrowMessage(FlightDescriptor descriptor, MessageMetadataResult message, ArrowBuf appMetadata,
                        ArrowBuf buf) {
     // No need to take IpcOption as this is used for deserialized ArrowMessage coming from the wire.
-    this.writeOption = new IpcOption();
-    if (message != null) {
-      this.writeOption.metadataVersion = MetadataVersion.fromFlatbufID(message.getMessage().version());
-    }
+    this.writeOption = message != null ?
+        new IpcOption(false, MetadataVersion.fromFlatbufID(message.getMessage().version())) :

Review comment:
       please comment the literal.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] liyafan82 commented on pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#issuecomment-754651491


   > @liyafan82 does this actually make a difference in benchmarks? I agree it is easier to reason about, but is there any way to avoid backward incompability?
   
   @emkornfield Sorry I do not have a benchmark result. I think it should be a positive change, as it avoids object allocations. 
   To avoid backward incompability, I have restored the default constructor. 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] BryanCutler commented on pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#issuecomment-775505415


   Apologies for the delay, I agree sooner than later for this so +1 for me.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] emkornfield closed pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
emkornfield closed pull request #9053:
URL: https://github.com/apache/arrow/pull/9053


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#issuecomment-770856425


   I'd be in favor of this, if we're going to break compatibility, I'd rather we do it sooner. And if this is a major regression, it should be simple to revert as well (we can just make the fields not final again and encourage people to migrate to the constructors).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] liyafan82 commented on a change in pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#discussion_r551933603



##########
File path: java/flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java
##########
@@ -194,10 +194,9 @@ public ArrowMessage(FlightDescriptor descriptor) {
   private ArrowMessage(FlightDescriptor descriptor, MessageMetadataResult message, ArrowBuf appMetadata,
                        ArrowBuf buf) {
     // No need to take IpcOption as this is used for deserialized ArrowMessage coming from the wire.
-    this.writeOption = new IpcOption();
-    if (message != null) {
-      this.writeOption.metadataVersion = MetadataVersion.fromFlatbufID(message.getMessage().version());
-    }
+    this.writeOption = message != null ?
+        new IpcOption(false, MetadataVersion.fromFlatbufID(message.getMessage().version())) :

Review comment:
       done. thanks for the good suggestion. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] liyafan82 commented on a change in pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#discussion_r551933913



##########
File path: java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestMetadataVersion.java
##########
@@ -56,9 +56,8 @@ public static void setUpClass() {
     schema = new Schema(Collections.singletonList(Field.nullable("foo", new ArrowType.Int(32, true))));
     unionSchema = new Schema(
         Collections.singletonList(Field.nullable("union", new ArrowType.Union(UnionMode.Dense, new int[]{0}))));
-    optionV4 = new IpcOption();
-    optionV4.metadataVersion = MetadataVersion.V4;
-    optionV5 = new IpcOption();
+    optionV4 = new IpcOption(false, MetadataVersion.V4);

Review comment:
       commented. thank you.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] emkornfield commented on pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#issuecomment-754256316


   @liyafan82 does this actually make a difference in benchmarks?  I agree it is easier to reason about, but is there any way to avoid backward incompability?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] emkornfield commented on pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#issuecomment-774787988


   +1 going to merge.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] emkornfield commented on pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#issuecomment-770457666


   In general I think this is good in the longer but since this is a breaking change, I'd like to get others thoughts before merging.  @BryanCutler @lidavidm any objections?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#issuecomment-752838345


   https://issues.apache.org/jira/browse/ARROW-11081


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kiszk commented on pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#issuecomment-753915971


   Looks good to me


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] liyafan82 commented on pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#issuecomment-752834119


   This PR also fixes a bug in ArrowMessage#ArrowMessage(ArrowDictionaryBatch, IpcOption)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] emkornfield commented on a change in pull request #9053: ARROW-11081: [Java] Make IPC option immutable

Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #9053:
URL: https://github.com/apache/arrow/pull/9053#discussion_r551601013



##########
File path: java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestMetadataVersion.java
##########
@@ -56,9 +56,8 @@ public static void setUpClass() {
     schema = new Schema(Collections.singletonList(Field.nullable("foo", new ArrowType.Int(32, true))));
     unionSchema = new Schema(
         Collections.singletonList(Field.nullable("union", new ArrowType.Union(UnionMode.Dense, new int[]{0}))));
-    optionV4 = new IpcOption();
-    optionV4.metadataVersion = MetadataVersion.V4;
-    optionV5 = new IpcOption();
+    optionV4 = new IpcOption(false, MetadataVersion.V4);

Review comment:
       please comment the literal.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org