GH-44363: [C#] Handle Flight data with zero batches #45315
Conversation
adamreeve
requested review from
assignUser,
jonkeane,
kou,
raulcd and
CurtHagenlocher
as code owners
| @@ -98,11 +98,25 @@ public AsyncUnaryCall<FlightInfo> GetInfo(FlightDescriptor flightDescriptor, Met | |||
| flightInfoResult.Dispose); | |||
| } | |||
|
|
|||
| [System.Obsolete("Use an async overload that takes a Schema")] | |||
There was a problem hiding this comment.
I'm not sure about whether these overloads should be obsolete, there might be valid use cases where the schema isn't immediately known? But that isn't supported by the Python or C++ implementations.
There was a problem hiding this comment.
Is there a specific reason to discourage its use?
There was a problem hiding this comment.
Only to better handle when zero batches are sent. I'm on the fence about this so am happy to remove the Obsolete if you think it's too disruptive.
There was a problem hiding this comment.
I guess I don't have a strong feeling either way.
There was a problem hiding this comment.
After thinking about this some more I've removed the Obsolete attributes and just added a remark to the new methods about the difference in behaviour, as I'm not sure this is a good enough reason to deprecate the existing methods.
github-actions
bot
added
awaiting committer review
| @@ -98,11 +98,25 @@ public AsyncUnaryCall<FlightInfo> GetInfo(FlightDescriptor flightDescriptor, Met | |||
| flightInfoResult.Dispose); | |||
| } | |||
|
|
|||
| [System.Obsolete("Use an async overload that takes a Schema")] | |||
There was a problem hiding this comment.
Is there a specific reason to discourage its use?
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit ead8d6f. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
See #44363. This improves compatibility with other Flight implementations and means user code works with empty data without needing to treat it as a special case to work around this limitation.
What changes are included in this PR?
FlightClient.StartPutthat immediately send the schema, before any data batches are sent.DoGeteven when there are no data batches.primitive_no_batchestest case for C# Flight.Are these changes tested?
Yes, using a new unit test and with the integration tests.
Are there any user-facing changes?
Yes. New overloads of the
FlightClient.StartPutmethod have been added that are async and accept aSchemaparameter, and ensure the schema is sent when no data batches are sent.