Skip to content

GH-47422: [Python][C++][Flight] Expose ipc::ReadStats in Flight MetadataRecordBatchReader#47432

Merged
raulcd merged 7 commits intoapache:mainfrom
raulcd:GH-47422
Sep 4, 2025
Merged

GH-47422: [Python][C++][Flight] Expose ipc::ReadStats in Flight MetadataRecordBatchReader#47432
raulcd merged 7 commits intoapache:mainfrom
raulcd:GH-47422

Conversation

@raulcd
Copy link
Member

@raulcd raulcd commented Aug 26, 2025

Rationale for this change

Now that we have implemented dictionary replacement / dictionary deltas on DoGet/DoExchange, exposing ReadStatistics can help us easily understand / debug / diagnose much better what are the different messages being sent.

What changes are included in this PR?

Add API to expose arrow::ipc::ReadStats stats() on MetadataRecordBatchReader. As we are mainly using RecordBatchStreamReader everywhere now, the stats are already populated so we just have to expose them.
Add also Python bindings.

Are these changes tested?

Yes via several Python tests we are validating the stats are retrieved and the results are the expected ones.

Are there any user-facing changes?

No breaking changes, new API to retrieve Statistics from the readers.

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.

There's one nit and one question about the accounting of metadata-only messages, otherwise LGTM.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 28, 2025
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 2, 2025
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.

LGTM except one small suggestion

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 3, 2025
@raulcd raulcd merged commit cdea48e into apache:main Sep 4, 2025
43 of 44 checks passed
@raulcd raulcd removed the awaiting changes Awaiting changes label Sep 4, 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 cdea48e.

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