zeroshade commented on code in PR #1042:
URL: https://github.com/apache/arrow-go/pull/1042#discussion_r3666503404
##########
arrow/flight/record_batch_writer.go:
##########
@@ -47,7 +47,9 @@ func (f *flightPayloadWriter) WritePayload(payload
ipc.Payload) error {
payload.SerializeBody(&f.buf)
f.fd.DataBody = f.buf.Bytes()
- return f.w.Send(&f.fd)
+ err := f.w.Send(&f.fd)
+ f.fd.FlightDescriptor = nil
+ return err
Review Comment:
Clearing per-payload rather than per-`Write` is the correct granularity, and
it matches C++ (`client.cc:339`, `first_payload_` flag) and Java
(`FlightClient.java:409`).
Two consequences to handle:
1. `arrow/flight/flightsql/client_test.go:475-477` asserts the old behavior
via `.Twice()` and now fails. Needs updating in this PR.
2. On the read side, `record_batch_reader.go:76` overwrites `descr` for
every message, so `LatestFlightDescriptor()` / `Chunk().Desc` now return nil
for the first record batch — which contradicts the contract documented at
`record_batch_reader.go:130-133`. Consider latching the first non-nil
descriptor there, or adjusting that doc.
Also worth noting `err` is now returned after mutating `f.fd`, so the
descriptor is cleared even when `Send` fails. That's almost certainly what you
want (the stream is broken anyway), but it's a deliberate choice worth a
comment.
--
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]
