Skip to content

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Jan 13, 2026

Rationale for this change

The IPC file exposes redundant information about Message sizes so as to allow for random access from the file footer.

We tried adding consistency checks in the past but this hit a bug in the JavaScript IPC writer at the time, so the checks were left disabled.

The JavaScript implementation was fixed soon after (7 years ago), so this PR re-enables those checks so as to more easily detect potentially invalid IPC files.

Are these changes tested?

By existing tests.

Are there any user-facing changes?

No, unless they try reading invalid IPC files.

@pitrou pitrou changed the title GH-48844: [C++][IPC] Turn disabled bodyLength assertions into error checks GH-48844: [C++]Check IPC Message body length consistency in IPC file Jan 13, 2026
@pitrou pitrou force-pushed the ipc-check-body-length branch 2 times, most recently from 2a29297 to e893e27 Compare January 13, 2026 16:53
@pitrou pitrou marked this pull request as ready for review January 13, 2026 16:53
@pitrou pitrou requested review from WillAyd and emkornfield January 13, 2026 17:03
@pitrou pitrou changed the title GH-48844: [C++]Check IPC Message body length consistency in IPC file GH-48844: [C++] Check IPC Message body length consistency in IPC file Jan 13, 2026
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jan 13, 2026
@pitrou pitrou force-pushed the ipc-check-body-length branch from e893e27 to 9ee2f5e Compare January 13, 2026 17:59
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 13, 2026
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jan 13, 2026
@pitrou
Copy link
Member Author

pitrou commented Jan 13, 2026

The occasional integration test failure is unrelated, see apache/arrow-js#15

@pitrou pitrou merged commit fed23f3 into apache:main Jan 13, 2026
51 of 52 checks passed
@pitrou pitrou removed the awaiting merge Awaiting merge label Jan 13, 2026
@pitrou pitrou deleted the ipc-check-body-length branch January 13, 2026 19:36
@conbench-apache-arrow
Copy link

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

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.

2 participants