Skip to content

GH-26727: [C++][Flight] Use ipc::RecordBatchWriter with custom IpcPayloadWriter for TransportMessageWriter (DoExchange)#47410

Merged
raulcd merged 10 commits intoapache:mainfrom
raulcd:GH-26727
Aug 26, 2025
Merged

GH-26727: [C++][Flight] Use ipc::RecordBatchWriter with custom IpcPayloadWriter for TransportMessageWriter (DoExchange)#47410
raulcd merged 10 commits intoapache:mainfrom
raulcd:GH-26727

Conversation

@raulcd
Copy link
Member

@raulcd raulcd commented Aug 22, 2025

Rationale for this change

The Flight Server DoExchange method currently does not support Dictionary replacement or Dictionary Deltas, similar to how the client currently behaves or how we do for DoGet we should use an ipc::RecordBatchWriter with a custom IpcPayloadWriter instead of reimplementing Dictionary Replacement / Deltas logic.

What changes are included in this PR?

Removes manually generation of individual ipc Payloads and uses an ipc::RecordBatchWriter and a custom TransportMessagePayloadWriter to modify the IpcPayloads into FlightPayloads.

Are these changes tested?

Yes, existing tests cover the DoExchange functionality and new test for Python has been added where Dictionary deltas are being send via DoExchange. The test was failing before this change because the dictionary wasn't updated:

            received_table = reader.read_all()
            expected_table = simple_dicts_table()
>           assert received_table.equals(expected_table)
E           assert False
E            +  where False = equals(pyarrow.Table\nsome_dicts: dictionary<values=string, indices=int64, ordered=0>\n----\nsome_dicts: [  -- dictionary:\n["foo... -- dictionary:\n["foo","baz","quux"]  -- indices:\n[2,1],  -- dictionary:\n["foo","baz","quux","new"]  -- indices:\n[0,3]])
E            +    where equals = pyarrow.Table\nsome_dicts: dictionary<values=string, indices=int64, ordered=0>\n----\nsome_dicts: [  -- dictionary:\n["foo...ull],  -- dictionary:\n["foo","baz","quux"]  -- indices:\n[2,1],  -- dictionary:\n["foo","baz","quux"]  -- indices:\n[0,3]].equals

pyarrow/tests/test_flight.py:2596: AssertionError
================================================================================== short test summary info ===================================================================================
FAILED pyarrow/tests/test_flight.py::test_flight_dictionary_deltas_do_exchange - assert False

Are there any user-facing changes?

No, only that the expected dictionary replacement/deltas will work for DoExchange.

@github-actions
Copy link

⚠️ GitHub issue #26727 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 22, 2025
@raulcd
Copy link
Member Author

raulcd commented Aug 22, 2025

@raulcd raulcd marked this pull request as ready for review August 22, 2025 09:25
@github-actions github-actions bot added awaiting merge Awaiting merge awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting merge Awaiting merge labels Aug 24, 2025
@lidavidm lidavidm changed the title GH-26727: [C++][Flight] Use ipc::RecordBatchWriter with custom IpcPayoadWriter for TransportMessageWriter (DoExchange) GH-26727: [C++][Flight] Use ipc::RecordBatchWriter with custom IpcPayloadWriter for TransportMessageWriter (DoExchange) Aug 24, 2025
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 25, 2025
@github-actions github-actions bot removed the awaiting change review Awaiting change review label Aug 25, 2025
return arrow::Status::OK();
}
Status Close() override {
// Closing is handled one layer up in TransportMessageWriter::Close
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, but I'm curious, shouldn't ServerDataStream::WritesDone be called here instead of at the end of ServerTransport::DoExchange? @lidavidm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well ideally, we would just expose gRPC and then the server could decide when it wants to end its output (it can continue reading even after doing so)

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Aug 25, 2025
@github-actions github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 25, 2025
@github-actions github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 25, 2025
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 25, 2025
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 25, 2025
@raulcd raulcd requested a review from pitrou August 26, 2025 06:35
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @raulcd !

@raulcd raulcd merged commit de52048 into apache:main Aug 26, 2025
40 of 41 checks passed
@raulcd raulcd removed the awaiting changes Awaiting changes label Aug 26, 2025
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit de52048.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants