[GitHub] [arrow] eerhardt commented on a diff in pull request #33893: GH-32240: [C#] Add new Apache.Arrow.Compression package to implement IPC decompression

["eerhardt (via GitHub)" <gi...@apache.org>]

eerhardt commented on code in PR #33893:
URL: https://github.com/apache/arrow/pull/33893#discussion_r1117359404


##########
csharp/src/Apache.Arrow.Compression/Apache.Arrow.Compression.csproj:
##########
@@ -0,0 +1,18 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks>

Review Comment:
   ```suggestion
       <TargetFramework>netstandard2.0</TargetFramework>
   ```
   
   Why target both? I don't see any `#if`s in the code. There isn't benefit in targeting both if you aren't compiling different code for each TFM.



##########
csharp/src/Apache.Arrow.Compression/Apache.Arrow.Compression.csproj:
##########
@@ -0,0 +1,17 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="CommunityToolkit.HighPerformance" Version="8.0.0" />

Review Comment:
   I found that the LZ4 library offers APIs that takes Spans. We should just use those APIs instead, and then we can get rid of this dependency.
   
   https://github.com/MiloszKrajewski/K4os.Compression.LZ4/blob/fa8b8e038b500d565efe12769db097852a28ddf7/src/K4os.Compression.LZ4/LZ4Codec.cs#L139-L144
   
   Note that there are 2 LZ4 libraries - one for streams (which this pr is using now) and one that doesn't use streams - https://www.nuget.org/packages/K4os.Compression.LZ4



##########
csharp/src/Apache.Arrow.Compression/Lz4CompressionCodec.cs:
##########
@@ -18,7 +18,7 @@
 using CommunityToolkit.HighPerformance;
 using K4os.Compression.LZ4.Streams;
 
-namespace Apache.Arrow.Tests.Compression
+namespace Apache.Arrow.Compression
 {
     internal sealed class Lz4CompressionCodec : ICompressionCodec

Review Comment:
   Since the `Lz4CompressionCodec` doesn't need to be disposed, would it make sense to just have a singleton object and return that from `CreateCodec`? It would save on a tiny allocation every time this is called.
   
   (I have the same comment for the `NoOpBufferCreator` in the previous PR, but I missed my window to add the comment there. Maybe we could update that as well?)



-- 
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: github-unsubscribe@arrow.apache.org

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