[PR] fix(arrow/flight): limit descriptors to next payload [arrow-go]

["fallintoplace (via GitHub)" <gi...@apache.org>]

fallintoplace opened a new pull request, #1042:
URL: https://github.com/apache/arrow-go/pull/1042

   ## What changed
   
   Clear a configured Flight descriptor immediately after sending one IPC payload rather than after the entire record-batch write.
   
   ## Why
   
   One record write can emit multiple payloads, such as a schema and record batch. The previous cleanup happened after all of them, so every payload carried the descriptor despite `SetFlightDescriptor` documenting that it applies only to the next payload.
   
   The regression test captures every payload from one write and verifies that only the first contains the descriptor.
   
   ## Validation
   
   `go test ./arrow/flight`
   


-- 
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] fix(arrow/flight): limit descriptors to next payload [arrow-go]

["zeroshade (via GitHub)" <gi...@apache.org>]

zeroshade merged PR #1042:
URL: https://github.com/apache/arrow-go/pull/1042


-- 
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] fix(arrow/flight): limit descriptors to next payload [arrow-go]

["zeroshade (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org