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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
