[GitHub] [arrow] eerhardt commented on a change in pull request #8146: ARROW-5034: [C#] ArrowStreamWriter implement sync WriteRecordBatch

[GitBox <gi...@apache.org>]

eerhardt commented on a change in pull request #8146:
URL: https://github.com/apache/arrow/pull/8146#discussion_r485898496



##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -212,6 +212,96 @@ private void CountSelfAndChildrenNodes(IArrowType type, ref int count)
             count++;
         }
 
+        private protected void WriteRecordBatchInternal(RecordBatch recordBatch)

Review comment:
       What about supporting `sync` write methods on `ArrowFileWriter`? Is that something left undone? Since that class inherits from this class, it is going to be a bit odd that some of its methods support sync, and some don't.

##########
File path: csharp/src/Apache.Arrow/Extensions/StreamExtensions.netcoreapp2.1.cs
##########
@@ -25,5 +27,26 @@ public static int Read(this Stream stream, Memory<byte> buffer)
         {
             return stream.Read(buffer.Span);
         }
+
+        public static void Write(this Stream stream, ReadOnlyMemory<byte> buffer)

Review comment:
       This whole method should just be:
   
   ```C#
   public static void Write(this Stream stream, ReadOnlyMemory<byte> buffer)
   {
       stream.Write(buffer.Span);
   }
   ```

##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -212,6 +212,96 @@ private void CountSelfAndChildrenNodes(IArrowType type, ref int count)
             count++;
         }
 
+        private protected void WriteRecordBatchInternal(RecordBatch recordBatch)
+        {
+            // TODO: Truncate buffers with extraneous padding / unused capacity
+
+            if (!HasWrittenSchema)
+            {
+                WriteSchema(Schema);
+                HasWrittenSchema = true;
+            }
+
+            Builder.Clear();

Review comment:
       Do you think the next ~40 lines of code could be refactored out so they weren't duplicate?




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