GH-48846: [C++] Read message metadata and body in one go in IPC file reader#48975
Conversation
|
|
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thank you for the submission @abhishek593. This looks good generally but I've made a number of suggestions below, feel free to update your PR when you have some time!
cpp/src/arrow/ipc/message.h
Outdated
| const int64_t offset, const int32_t metadata_length, io::RandomAccessFile* file, | ||
| const FieldsLoaderFunction& fields_loader = {}); | ||
|
|
||
| /// \brief Read encapsulated RPC message from position in file |
There was a problem hiding this comment.
Ok, I think "RPC" is a typo, it should be "IPC", could you perhaps fix all occurrences in this file?
| // there are 3 read IOs before reading body: | ||
| // 1) read magic and footer length IO | ||
| // 2) read footer IO | ||
| // 3) read record batch metadata IO | ||
| EXPECT_EQ(read_ranges.size(), 3 + expected_body_read_lengths.size()); |
There was a problem hiding this comment.
Ok, can we keep a less strict assertion to check that we can at least access the two first elements without segfaulting?
Something like:
// there are at least 2 read IOs before reading body:
// 1) read magic and footer length IO
// 2) read footer IO
EXPECT_GE(read_ranges.size(), 2);
cpp/src/arrow/ipc/read_write_test.cc
Outdated
| // there are 3 read IOs before reading body: | ||
| // 1) read magic and footer length IO | ||
| // 2) read footer IO | ||
| // 3) read record batch metadata IO |
There was a problem hiding this comment.
This comment only matches one of the two code paths. Perhaps fix it or use a separate comment per code path?
| int64_t total_body = 0; | ||
| for (auto len : expected_body_read_lengths) total_body += len; | ||
|
|
||
| EXPECT_GT(read_ranges[2].length, total_body); |
There was a problem hiding this comment.
Add a small comment explaining why? Also can take inspiration from the other code path and add:
EXPECT_LE(read_ranges[2].length, total_body + footer_length);
github-actions
bot
added
awaiting committer review
abhishek593
force-pushed
the
feature/ipc-read-message-optimization
branch
from
10990b2 to
cc7dd86
Compare
… helper and fix documentation
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Looking excellent now, thank you @abhishek593 . I'm just going to push a minor cosmetic change and we'll be able to merge if CI passes.
pitrou
force-pushed
the
feature/ipc-read-message-optimization
branch
from
cc7dd86 to
ff6a9e5
Compare
pitrou
changed the title
|
This is now merged, thanks again @abhishek593 :) |
2 participants
Rationale for this change
ReadMessageAsync takes a body_length parameter and reads Message metadata + body in one go, but the blocking version ReadMessage reads the body length from the Message and issues a second read for the body. This PR adds a ReadMessage overload that takes the body length as parameter and does a single read like the async version does. This reduces the number of IOs issued by the IPC file reader.
What changes are included in this PR?
Are these changes tested?
Yes, added TestReadMessage.ReadBodyWithLength and updated other tests to use the new overload.
Are there any user-facing changes?
No.