feat: add ICompressionCodecFactory support to Flight record batch readers

[balicat]

Summary

Resolves #184.

The Problem

When a Flight server sends IPC-compressed record batches (LZ4/Zstd), the C# Flight client throws:

"Body is compressed with codec LZ4_FRAME but no ICompressionCodecFactory has been configured to decompress buffers"

The decompression infrastructure already exists — ArrowStreamReader accepts ICompressionCodecFactory and the Apache.Arrow.Compression package provides LZ4/Zstd codecs. But the Flight reader chain never passes the factory through:

FlightClient.GetStream()
  → FlightClientRecordBatchStreamReader(stream)
    → FlightRecordBatchStreamReader(stream)
      → RecordBatchReaderImplementation(stream)
        → ArrowReaderImplementation()          ← parameterless, _compressionCodecFactory = null

RecordBatchReaderImplementation calls the base constructor without a codec factory, so GetBufferCreator() always hits the "no factory configured" error path.

The Fix

Thread ICompressionCodecFactory through the reader hierarchy (5 files, all with = null defaults for backward compatibility):

1. RecordBatchReaderImplementation — accept ICompressionCodecFactory, pass to base(allocator: null, compressionCodecFactory)
2. FlightRecordBatchStreamReader — accept and forward to RecordBatchReaderImplementation
3. FlightClientRecordBatchStreamReader — forward to base
4. FlightServerRecordBatchStreamReader — forward to base (both constructors)
5. FlightClient — store as field, add constructor overloads, pass to readers in GetStream() and DoExchange()

After the fix:

FlightClient(channel, new CompressionCodecFactory())
  → stores _compressionCodecFactory

FlightClient.GetStream(ticket)
  → FlightClientRecordBatchStreamReader(stream, _compressionCodecFactory)
    → FlightRecordBatchStreamReader(stream, compressionCodecFactory)
      → RecordBatchReaderImplementation(stream, compressionCodecFactory)
        → ArrowReaderImplementation(null, compressionCodecFactory)  ← decompression works

Usage

using Apache.Arrow.Compression;

// Before (compressed batches cause error):
var client = new FlightClient(channel);

// After (LZ4/Zstd decompression handled transparently):
var client = new FlightClient(channel, new CompressionCodecFactory());

Impact

• 25 lines added, 8 removed across 5 files
• No breaking changes — all new parameters default to null
• No new dependencies — ICompressionCodecFactory is in Apache.Arrow.Ipc, already referenced by the Flight package

Test plan

• Existing Flight tests pass unchanged (backward compatibility via null defaults)
• Test with a Flight server sending LZ4-compressed record batches
• Test with a Flight server sending Zstd-compressed record batches
• Confirm GetStream() and DoExchange() correctly decompress when ICompressionCodecFactory is provided
• Confirm no regression when ICompressionCodecFactory is null (existing behavior)

[CurtHagenlocher]

In addition to ensuring that the existing tests are all passing and hopefully creating some new ones, could you please change the code so FlightClient takes an ArrowContext instead of an ICompressionCodecFactory as the additional argument? This would allow FlightClient to support extension types and a custom allocator in addition to the compression codecs.

[CurtHagenlocher]

If I understand the error message in the failing tests correctly -- and I'm not sure I do -- then there can only be one constructor available to DI whose first argument is CallInvoker. I think we can work around this by marking the added constructor with [ActivatorUtilitiesConstructor], but this then also requires that we have a dependency on the Microsoft.Extensions.DependencyInjection.Abstractions package which would be unfortunate. I am not a user of Flight (or more broadly of Grpc) so I don't know what the tradeoffs here are.

I'm also not super keen on dependency injection :(.

[balicat]

Updated per review feedback:

• Replaced ICompressionCodecFactory with ArrowContext across all Flight record batch readers
• ArrowContext carries compression codec factory, memory allocator, and extension type registry
• Used optional parameters (ArrowContext context = null) instead of constructor overloads — this avoids the DI ambiguity with multiple CallInvoker constructors, no need for [ActivatorUtilitiesConstructor] or extra dependencies
• All existing code works unchanged (context defaults to null)

Usage:
var context = new ArrowContext(compressionCodecFactory: new CompressionCodecFactory());
var client = new FlightClient(grpcChannel, context);

[balicat]

Updated with tests and fixes:

• 4 new tests added (55 total, all passing):
  • TestGetWithArrowContext — verify Get works with ArrowContext
  • TestGetWithNullArrowContext — backward compat with null context
  • TestPutAndGetWithArrowContext — put+get round-trip with context
  • TestFlightClientDefaultConstructorStillWorks — original constructor unchanged
• Fixed pre-existing netstandard2.1 build error in CookieExtensions.cs (collection expression syntax ambiguous with Split overloads)

I've been testing this end-to-end with our production Arrow Flight server (EnergyScope — energy market data). With lz4 compression enabled, we see a 3.5x improvement on large queries (1.45M rows: 45s → 13s)
through our C# Excel add-in.

[CurtHagenlocher]

Thanks, this looks great! Where possible, we would like to retain binary backwards-compatibility in addition to source backwards-compatibility so that means adding an overload for public methods instead of adding an optional parameter. This would be straightforward if not for the DI-based client factory. I've sketched out an approach which seems to work for that.

[CurtHagenlocher]

For binary backwards compatibility, can you instead add a second public constructor with the additional parameter? i.e.

public FlightServerRecordBatchStreamReader(IAsyncStreamReader<FlightData> flightDataStream)
    : this(flightDataStream, null)
{
}

public FlightServerRecordBatchStreamReader(IAsyncStreamReader<FlightData> flightDataStream, ArrowContext context)
    : base(new StreamReader<FlightData, Protocol.FlightData>(flightDataStream, data => data.ToProtocol()), context)
{
}

[CurtHagenlocher]

For binary backwards compatibility, can you instead add a second public constructor with the additional parameter? i.e.

public FlightClient(ChannelBase grpcChannel) : this(grpcChannel. null)
{
}

public FlightClient(ChannelBase grpcChannel, ArrowContext context)
{
    _client = new FlightService.FlightServiceClient(grpcChannel);
    _context = context;
}

[CurtHagenlocher]

For binary backwards compatibility, can you instead add a second constructor with the additional parameter? This one is trickier because the DI mechanism doesn't like having two constructors that both match what it's looking for, so we need to use a factory method instead:

public FlightClient(CallInvoker callInvoker) : this(callInvoker. null)
{
}

private FlightClient(CallInvoker callInvoker, ArrowContext context)
{
    _client = new FlightService.FlightServiceClient(callInvoker);
    _context = context;
}

public static FlightClient Create(CallInvoker callInvoker, ArrowContext context)
{
    return new FlightClient(callInvoker, context);
}

[CurtHagenlocher]

It would then also be good to add a test which exercises this code path. You could clone the existing TestIntegrationWithGrpcNetClientFactory test and then replace the client setup with

services.AddGrpcClient<FlightClient>(grpc => grpc.Address = new Uri(_testWebFactory.GetAddress()))
    .ConfigureGrpcClientCreator(invoker =>
{
    return FlightClient.Create(invoker, new ArrowContext());
});

This will also demonstrate to users how to use an ArrowContext with the factory.

[CurtHagenlocher]

@balicat, I'd be happy to take over this PR if you don't have the time to work on it. Let me know!

[balicat]

Thanks Curt — I'd like to finish it myself. I willl push this week.

[balicat]

Updated per review:

• Restored the original single-parameter constructors and added explicit ArrowContext overloads — binary compatibility preserved
• CallInvoker path: private two-arg constructor + public static FlightClient.Create(CallInvoker, ArrowContext) factory, keeping DI resolution unambiguous
• Added TestIntegrationWithGrpcNetClientFactoryAndArrowContext exercising the factory via ConfigureGrpcClientCreator, as suggested
• While in there: DoExchange wasn't passing the client's ArrowContext to its response stream reader (compressed exchange responses couldn't decompress) — fixed

All 56 Flight + 88 Flight.Sql tests passing.

[CurtHagenlocher]

Thanks! Can you either roll back the formatting change in CookieExtensions.cs or explain why you think it's a good idea? Everything else looks great.

[CurtHagenlocher]

I don't specifically have a preference for the newer collection syntax, but I don't see any reason to change it to the older one.

[balicat]

Rolled back in c5b734c — good catch, and it deserves an explanation rather than a shrug: it wasn't a style preference. On my local SDK (8.0.128) those three calls fail to compile for netstandard2.1 with CS0121 (char[]/string overload ambiguity on Split), and the workaround snuck into the test commit unnoticed. CI's compiler is clearly happy with the collection expressions, so they're restored — I'll build locally with a newer SDK instead.

[CurtHagenlocher]

Rolled back in c5b734c — good catch, and it deserves an explanation rather than a shrug: it wasn't a style preference. On my local SDK (8.0.128) those three calls fail to compile for netstandard2.1 with CS0121 (char[]/string overload ambiguity on Split).

Interesting. I wonder why it doesn't cause a similar problem in CI (or on my own machine).

[balicat]

Chased it down: dotnet/roslyn#70701. Under the original C# 12 rules a char collection expression was deliberately treated as convertible to string for overload-resolution purposes (LDM 2023-10-02) — even though actually constructing the string errors — so Split(['='], 2, …) was ambiguous by design against the (string, int, StringSplitOptions) overload that exists on netstandard2.1. The collection-expression betterness revisions in C# 13-era compilers resolve it in favor of char[]. Since the repo sets LangVersion latest and has no global.json, the behavior tracks whichever SDK does the build: my 8.0.128 still carries the old rules, while CI and presumably your machine run something newer. Updating my local SDK.