CurtHagenlocher commented on code in PR #424:
URL: https://github.com/apache/arrow-dotnet/pull/424#discussion_r3849194740
##########
src/Apache.Arrow/Ipc/ArrowStreamWriter.cs:
##########
@@ -1132,11 +1163,21 @@ public virtual void WriteRecordBatch(RecordBatch
recordBatch)
WriteRecordBatchInternal(recordBatch);
}
+ public virtual void WriteRecordBatch(RecordBatch recordBatch,
IReadOnlyDictionary<string, string> customMetadata)
+ {
+ WriteRecordBatchInternal(recordBatch, customMetadata);
Review Comment:
Based on my previous analysis of the original PR, this overload breaks
`ArrowFileWriter` because it would skip the call to `WriteStart` which is
performed in that class's `WriteRecordBatch`/`WriteRecordBatchAsync` overrides.
##########
src/Apache.Arrow/Ipc/ArrowStreamWriter.cs:
##########
@@ -840,14 +854,21 @@ private protected void
WriteRecordBatchInternal(RecordBatch recordBatch)
variadicCountsOffset);
long metadataLength =
WriteMessage(Flatbuf.MessageHeader.RecordBatch,
- recordBatchOffset, recordBatchBuilder.TotalLength);
+ recordBatchOffset, recordBatchBuilder.TotalLength,
customMetadataVectorOffset);
long bufferLength = WriteBufferData(recordBatchBuilder.Buffers);
FinishedWritingRecordBatch(bufferLength, metadataLength);
}
+ private protected Task WriteRecordBatchInternalAsync(RecordBatch
recordBatch,
Review Comment:
My suspicion is that it's better not to have this overload. We don't need it
for backwards-compatibility and removing it might avoid an error in a derived
class.
##########
src/Apache.Arrow/Ipc/ArrowStreamWriter.cs:
##########
@@ -1376,14 +1437,23 @@ private protected long WriteMessage<T>(
/// <returns>
/// The number of bytes written to the stream.
/// </returns>
+ private protected virtual ValueTask<long> WriteMessageAsync<T>(
Review Comment:
Consider making this non-virtual or removing it entirely and forcing use of
the signature with the `customMetadataOffset` (which is what `WriteMessage`
does).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
