adamreeve commented on code in PR #46190:
URL: https://github.com/apache/arrow/pull/46190#discussion_r2101468768
##########
csharp/src/Apache.Arrow/Ipc/ICompressionCodec.cs:
##########
@@ -44,5 +44,21 @@ void Compress(ReadOnlyMemory<byte> source, Stream
destination)
#else
;
#endif
+
+ /// <summary>
+ /// try to write compressed data to span
+ /// </summary>
+ /// <param name="source">The data to compress</param>
+ /// <param name="destination">Span to write compressed data to</param>
+ /// <param name="bytesWritten">The number of bytes written to the
destination</param>
+ /// <returns>true if compressed was successful, false if the
destination buffer is too small</returns>
+ bool TryCompress(ReadOnlyMemory<byte> source, Memory<byte>
destination, out int bytesWritten)
+#if NET6_0_OR_GREATER
+ {
+ throw new NotImplementedException("This codec does not support
compression");
Review Comment:
Could you add a default implementation that uses the `Compress` method? Then
this is only breaking for .NET < 6.0.
##########
csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs:
##########
@@ -590,10 +607,8 @@ public void Visit(IArrowArray array)
private readonly bool _leaveOpen;
private readonly IpcOptions _options;
private readonly MemoryAllocator _allocator;
- // Reuse a single memory stream for writing compressed data to, to
reduce memory allocations
- private readonly MemoryStream _compressionStream = new MemoryStream();
- private protected const Flatbuf.MetadataVersion CurrentMetadataVersion
= Flatbuf.MetadataVersion.V5;
+ internal const Flatbuf.MetadataVersion CurrentMetadataVersion =
Flatbuf.MetadataVersion.V5;
Review Comment:
It doesn't look like this needed to be changed to internal?
##########
csharp/src/Apache.Arrow.Compression/Lz4CompressionCodec.cs:
##########
@@ -15,9 +15,14 @@
using System;
using System.IO;
+using System.Runtime.CompilerServices;
Review Comment:
I don't think any of these new using statements are actually needed
##########
csharp/test/Apache.Arrow.Tests/TestMemoryAllocator.cs:
##########
@@ -13,17 +13,54 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+using System;
using Apache.Arrow.Memory;
using System.Buffers;
+using System.Diagnostics;
Review Comment:
`System.Diagnostics` and `Xunit` usings aren't needed.
##########
csharp/test/Apache.Arrow.Tests/TestMemoryAllocator.cs:
##########
@@ -13,17 +13,54 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+using System;
using Apache.Arrow.Memory;
using System.Buffers;
+using System.Diagnostics;
+using System.Threading;
+using Xunit;
namespace Apache.Arrow.Tests
{
public class TestMemoryAllocator : MemoryAllocator
{
+ private int _rented = 0;
Review Comment:
Could the formatting in this class please be tidied up by adding blank lines
around methods.
##########
csharp/src/Apache.Arrow.Compression/Lz4CompressionCodec.cs:
##########
@@ -56,6 +61,21 @@ public void Compress(ReadOnlyMemory<byte> source, Stream
destination)
encoder.WriteManyBytes(source.Span);
}
+ public bool TryCompress(ReadOnlyMemory<byte> source, Memory<byte>
destination, out int bytesWritten)
+ {
+ using var memoryStream = new MemoryStream(destination.Length);
Review Comment:
Is this not a regression in behaviour by creating a new `MemoryStream` here
for each `TryCompress` call rather than reusing the stream from
`ArrowRecordBatchFlatBufferBuilder._compressionStream`?
--
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]
