[C#] Add decompression support for Record Batches

[asfimport]

C# Implementation does not support reading batches written in other implementations of Arrow when the compression is specified in IPC Write options.

e.g. Reading this batch from pyarrow in C# will fail:

pyarrow.ipc.RecordStreamBatchWriter(sink, schema, options=pyarrow,ipcWriteOptions(compression="lz4"))

 

This is to support decompression (lz4 & zstd) in the C# implementation.

Reporter: Rishabh Rana / @rishabh-rana

Related issues:

• [C#] Support compression in IPC format (is duplicated by)

_{Note: This issue was originally created as ARROW-16921. Please see the migration documentation for further details.}

[asfimport]

Rishabh Rana / @rishabh-rana:
@eerhardt @HashidaTKS I have a small query for this change. I would be grateful if you can answer/ point me to the correct person to help me with this:

Are we allowed to add a dependency to Arrow for an lz4 compression/ decompression library as part of this change? If not can you suggest another way forward?

[asfimport]

Eric Erhardt / @eerhardt:
One option would be to add an extension point into the reader/writer where a compression library could be "plugged in". That way the core Arrow library doesn't need to depend on all the different compression libraries, especially when they might not be used.

[asfimport]

Rishabh Rana / @rishabh-rana:
@eerhardt I looked at the possible options and the least intrusive one I could find is exposing an interface to the user for the library. They must write a "Decode" class based on that interface and pass that to the reader. This way users can use any library they want to decode the body. 

example:

interface Decoder {
  public void Lz4Decode(Stream input, Stream output);
  public void ZstdDecode(Stream input, Stream output);
}

// usage
public class LZ4Decoder: Decoder {
  public void Lz4Decode(Stream input, Stream output) {
    LZ4Stream.Decode(input).CopyTo(output);
  }
 public void Lz4Decode(Stream input, Stream output) {
    throw new NotImplementedException();
  }
}

ArrowStreamReader(stream, new LZ4Decoder());

Does this sound good to you?

Apologies for confirming again but this would impact quite a bit of code, So I wanted to make sure that I follow the best practices. Thanks!

[asfimport]

Rishabh Rana / @rishabh-rana:
@eerhardt pinging you for the query above. Do you think this approach looks good?

[asfimport]

Weston Pace / @westonpace:
@rishabh-rana I think this approach could be slightly improved. What you have described would require the reader to know ahead of time what compression method was used on the IPC file. Could you perhaps do something like...

interface Decoder {
  public void Decode(Stream input, Stream output);
}
interface CompressionProvider {
  public Decoder GetDecoder(Apache.Arrow.Flatbuf.CompressionType compressionType);
}
public class LZ4Decoder : Decoder {
  public void Lz4Decode(Stream input, Stream output) {
    LZ4Stream.Decode(input).CopyTo(output);
  }
}
public class LZ4OnlyCompressionProvider : CompressionProvider {

  private readonly LZ4Decoder _lz4Decoder = new LZ4Decoder();

  public Decoder GetDecoder(Apache.Arrow.Flatbuf.CompressionType compressionType) {
    if (compressionType == CompressionType.LZ4_FRAME) {
      return _lz4Decoder;
    } else {
      throw new NotImplementedException();
    }
  }
}

ArrowStreamReader(stream, new LZ4OnlyCompressionProvider());

This opens the door for a future where someone could write a compression provider that supported both LZ4 and gzip (I think that gzip is part of System.IO.Compression and so should be very easy to support).

[asfimport]

Weston Pace / @westonpace:
Also, it might be nice if there were a standard compression provider which auto-detected the presence of common compression packages and added support automatically if the package was installed. That way the user wouldn't have to do anything except make sure they had the appropriate compression package installed.

For example, you could use Type.GetType to check if System.IO.Compression.GZipStream exists and, if it does, add a GZipDecoder automatically. However, this would require reflection to create the GZipStream. Maybe someone can think of a more clever solution.

[asfimport]

Adam Reeve / @adamreeve:
Hi, we're interested in this feature at G-Research and believe compression support is important for use at scale. We're keen to help out where we can. I agree that it would be nice if there was a way for compression support to be added more automatically, without users needing to implement decoders themselves. An alternative approach that builds on the Type.GetType idea would be to provide wrapper packages for each compression format used in the IPC format (currently only Zstd and LZ4 I believe), and these could provide implementations of an IDecoder interface defined in the main dotnet Arrow library. So instead of getting the LZ4Stream type with Type.GetType for example, we could do something like this to work with the IDecoder interface without needing to use reflection:

var lz4DecoderType = Type.GetType("Apache.Arrow.Compression.Lz4.Lz4Decoder, Apache.Arrow.Compression.Lz4", false);
if (lz4DecoderType != null)
{
    if (Activator.CreateInstance(lz4DecoderType) is IDecoder decoder)
    {
        // use decoder
    }
    else
    {
        throw new Exception("Failed to cast Lz4Decoder to IDecoder");
    }
}

Having to maintain these extra wrapper packages would be a bit more work from an operational point of view though. It does seem like just adding new dependencies directly to the Arrow package would be a lot more straightforward, and given there are only two compression formats currently used is this really a problem?

On a more minor point, would Decompressor be a more precise term to use rather than Decoder? At least in the Parquet world, which I'm a bit more familiar with, encodings are a separate concept to compression formats.

[asfimport]

Rishabh Rana / @rishabh-rana:
Thanks for your responses @adamreeve and @westonpace.

Personally, I am more inclined towards the approach that Eric and Weston have proposed. To avoid any unnecessary dependencies on Arrow. I will try to put the code together for the issue soon for a more fruitful discussion on the PR itself.

Thanks!

[asfimport]

Adam Reeve / @adamreeve:
Hi @rishabh-rana, I wanted to get some proof-of-concept code working using decompression so I've implemented basic Zstd decompression support in my Arrow fork: master...adamreeve:arrow:dotnet_decompression

It isn't in a state suitable for a pull request and just adds ZstdNet as a dependency of Apache.Arrow, which required bumping the target framework to netstandard2.0, but it might still be useful to you as a starting point, depending on how far you've already got with this.

[asfimport]

Apache Arrow JIRA Bot:
This issue was last updated over 90 days ago, which may be an indication it is no longer being actively worked. To better reflect the current state, the issue is being unassigned per project policy. Please feel free to re-take assignment of the issue if it is being actively worked, or if you plan to start that work soon.

[asfimport]

Adam Reeve / @adamreeve:
 Hi @rishabh-rana, are you still interested in working on this? If not I could look at adapting my code to work with the API discussed here.

[adamreeve]

I went ahead and tidied up my code and used the reflection approach suggested by @westonpace to implement a default compression provider. I've created a PR for this at #33603

I'm still not sure the reflection approach is the best way to go, it's pretty verbose and not very discoverable by users. I've included some comments in that PR about alternative approaches but maybe it's better to discuss them here rather than on the PR, so I'll copy them below:

An alternative approach that could be considered instead of reflection is to use these extra dependencies as build time dependencies but not make them dependencies of the NuGet package. I tested this out in adamreeve@4544afd and it seems to work reasonably well too but required bumping the version of System.Runtime.CompilerServices.Unsafe under the netstandard2.0 and netcoreapp3.1 targets. This reduces all the reflection boilerplate but seems pretty hacky as now Apache.Arrow.dll depends on these extra dlls and we rely on the dotnet runtime behaviour of not trying to load them until they're used. So I think the reflection approach is better.

Another alternative would be to move decompression support into a separate NuGet package (eg. Apache.Arrow.Compression) that depends on Apache.Arrow and has an implementation of ICompressionProvider that users can pass in to the ArrowFileReader constructor, or maybe has a way to register itself with the Apache.Arrow package so it only needs to be configured once. That would seem cleaner to me but I'm not sure how much work it would be to set up a whole new package.

PS. It wasn't very obvious that this issue had been migrated to GitHub, maybe comments should be added to open Jira issues to explain that comments are disabled and point to the migrated issue on GitHub? Looks like I just had bad timing, there's a comment there now

[westonpace]

Another alternative would be to move decompression support into a separate NuGet package (eg. Apache.Arrow.Compression) that depends on Apache.Arrow and has an implementation of ICompressionProvider that users can pass in to the ArrowFileReader constructor, or maybe has a way to register itself with the Apache.Arrow package so it only needs to be configured once. That would seem cleaner to me but I'm not sure how much work it would be to set up a whole new package.

This sounds like a good approach to me. I think having a standard package that just picks some common compression libs is going to be sufficient for 80% of use cases.

[adamreeve]

Cool okay, @lidavidm mentioned on the PR that the Java library works that way too so I'll take a look at the API used there to see if I can make the dotnet API consistent with the way the Java API works, and pare the PR back to remove the reflection based implementation for now. It might make sense to add the new package in a separate PR.

[westonpace]

It might make sense to add the new package in a separate PR.

That seems reasonable to me.