You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "owencorrigan (via GitHub)" <gi...@apache.org> on 2026/09/10 14:04:58 UTC
[PR] feat: support reading dictionary-encoded columns over Arrow Flight [arrow-dotnet]
owencorrigan opened a new pull request, #433:
URL: https://github.com/apache/arrow-dotnet/pull/433
## What's Changed
The C# Arrow Flight reader (`RecordBatchReaderImplementation`) threw
`NotImplementedException` on any dictionary batch, so a Flight stream containing a
dictionary-encoded column could not be read.
- `ReadNextRecordBatchAsync` now loops over the incoming Flight messages, feeding
each through the base `CreateArrowObjectFromMessage` (which reads dictionary
batches into the `DictionaryMemo` and returns null for them) until it yields a
record batch, instead of throwing on any non-record-batch message. Handles both
single and replacement dictionaries.
- The schema is decoded using the reader's `DictionaryMemo` via a new base helper
`ReadSchemaFromMessage`, so dictionary-encoded fields are registered
(`FlightMessageSerializer` previously discarded them).
The change routes dictionary batches to the existing `ArrowReaderImplementation`
decoder, so it is not specific to any particular index or value type. Validated
against a pyarrow `RecordBatchStream` serving a `dictionary<int16, string>` column
(single + replacement dictionaries); the current reader throws on the same stream.
Refs #180 (receive side).
Generated-By: Claude Opus 4.8
--
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
Re: [PR] feat: support reading dictionary-encoded columns over Arrow Flight [arrow-dotnet]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #433:
URL: https://github.com/apache/arrow-dotnet/pull/433#discussion_r4029206512
##########
src/Apache.Arrow.Flight/Internal/FlightDataStream.cs:
##########
@@ -74,25 +68,28 @@ public async Task Write(RecordBatch recordBatch, ByteString applicationMetadata)
}
ResetStream();
- _currentFlightData = new Protocol.FlightData();
-
- if (applicationMetadata != null)
- {
- _currentFlightData.AppMetadata = applicationMetadata;
- }
+ // Attached to the record-batch message, not to any preceding dictionary messages.
+ _recordBatchAppMetadata = applicationMetadata;
+ // Writes any dictionary-batch messages followed by the record-batch message. Each is
+ // flushed as its own FlightData frame (see WriteMessageAsync) so that dictionary batches
+ // are delivered before the record batch that references them.
await WriteRecordBatchInternalAsync(recordBatch, customMetadata: null).ConfigureAwait(false);
Review Comment:
This still inherits `ArrowStreamWriter`'s one-time `HasWrittenDictionaryBatch` gate, so after the first `Write` no dictionary message is emitted for later record batches. If a later batch carries a replacement/new dictionary (the Flight issue calls for dictionaries to be resent per batch), its indices are decoded against the first batch's dictionary and yield incorrect values; please resend/recollect the dictionaries for each Flight batch or implement explicit reuse/delta handling.
--
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
Re: [PR] feat: support reading dictionary-encoded columns over Arrow Flight [arrow-dotnet]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on PR #433:
URL: https://github.com/apache/arrow-dotnet/pull/433#issuecomment-5683332400
A round-trip test sounds like a good idea. Can you also please rebase and then resolve the conflicts?
--
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
Re: [PR] feat: support reading dictionary-encoded columns over Arrow Flight [arrow-dotnet]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #433:
URL: https://github.com/apache/arrow-dotnet/pull/433#discussion_r4001191667
##########
src/Apache.Arrow.Flight/Internal/RecordBatchReaderImplementation.cs:
##########
@@ -120,8 +122,9 @@ public override async ValueTask<RecordBatch> ReadNextRecordBatchAsync(Cancellati
{
await ReadSchemaAsync(cancellationToken).ConfigureAwait(false);
}
- var moveNextResult = await _flightDataStream.MoveNext().ConfigureAwait(false);
- if (moveNextResult)
+ // Dictionary batches precede the record batch that references them; keep
+ // reading until CreateArrowObjectFromMessage yields a record batch.
+ while (await _flightDataStream.MoveNext().ConfigureAwait(false))
Review Comment:
This loop is the new path that can await multiple Flight messages (for example, a replacement dictionary followed by a record batch), but it drops the caller's cancellation token. A cancellation requested while the stream is being drained will not be observed until the server ends the stream; pass `cancellationToken` to `MoveNext` here.
##########
src/Apache.Arrow.Flight/Internal/RecordBatchReaderImplementation.cs:
##########
@@ -120,8 +122,9 @@ public override async ValueTask<RecordBatch> ReadNextRecordBatchAsync(Cancellati
{
await ReadSchemaAsync(cancellationToken).ConfigureAwait(false);
}
- var moveNextResult = await _flightDataStream.MoveNext().ConfigureAwait(false);
- if (moveNextResult)
+ // Dictionary batches precede the record batch that references them; keep
+ // reading until CreateArrowObjectFromMessage yields a record batch.
+ while (await _flightDataStream.MoveNext().ConfigureAwait(false))
Review Comment:
The new Flight-specific dictionary path has no regression test: the existing Flight tests exercise plain batches, while dictionary coverage is only in the IPC reader tests. Please add a FlightData-stream test with a dictionary batch before a record batch and a replacement/delta case, asserting the decoded schema, dictionary values, and indices; otherwise this receive-side behavior can regress without a Flight test catching it.
--
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
Re: [PR] feat: support reading dictionary-encoded columns over Arrow Flight [arrow-dotnet]
Posted by "owencorrigan (via GitHub)" <gi...@apache.org>.
owencorrigan commented on PR #433:
URL: https://github.com/apache/arrow-dotnet/pull/433#issuecomment-5682665280
I've added support for cancellation. I have also added support for sending dictionary encoded columns over flight so that we could do a round trip test
Let me know if this is OK
--
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