Copilot commented on code in PR #49771:
URL: https://github.com/apache/arrow/pull/49771#discussion_r4221772365
##########
cpp/src/arrow/ipc/read_write_test.cc:
##########
@@ -2265,6 +2265,75 @@ TEST(TestRecordBatchStreamReader, MalformedInput) {
ASSERT_RAISES(Invalid, RecordBatchStreamReader::Open(&garbage_reader));
}
+TEST(TestRecordBatchStreamReader, OpenFileFormatSuggestsFileReader) {
+ std::shared_ptr<RecordBatch> batch;
+ ASSERT_OK(MakeIntRecordBatch(&batch));
+
+ FileWriterHelper helper;
+ ASSERT_OK(helper.Init(batch->schema(), IpcWriteOptions::Defaults()));
+ ASSERT_OK(helper.WriteBatch(batch));
+ ASSERT_OK(helper.Finish());
+
+ io::BufferReader reader(helper.buffer_);
+ // Check we mention using the file_reader when we detect file format
+ EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid,
+ ::testing::HasSubstr("Try the IPC file
reader"),
+ RecordBatchStreamReader::Open(&reader));
+}
+
+TEST(TestRecordBatchStreamReader, CorruptDataDoesNotSuggestFileReader) {
+ // Continuation marker + metadata_length = 100, then 8 bytes of non-magic
data.
+ const std::string corrupt(
+ "\xff\xff\xff\xff"
+ "\x64\x00\x00\x00"
+ "ABABABAB",
+ 16);
+ auto buffer = std::make_shared<Buffer>(corrupt);
+ io::BufferReader reader(buffer);
+ // Validate that we don't suggest file reader when file is just corrupt
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid, ::testing::Not(::testing::HasSubstr("Try the IPC file reader")),
+ RecordBatchStreamReader::Open(&reader));
+}
+
+TEST(TestRecordBatchStreamReader,
FileMagicAfterFirstMessageDoesNotSuggestFileReader) {
+ std::shared_ptr<RecordBatch> batch;
+ ASSERT_OK(MakeIntRecordBatch(&batch));
+
+ StreamWriterHelper helper;
+ ASSERT_OK(helper.Init(batch->schema(), IpcWriteOptions::Defaults()));
+ ASSERT_OK(helper.WriteBatch(batch));
+ ASSERT_OK(helper.Finish());
+
+ // Replace the 8-byte marker with the IPC file magic.
Review Comment:
Remove the duplicated space in this comment.
##########
cpp/src/arrow/ipc/message.cc:
##########
@@ -1051,6 +1051,19 @@ class InputStreamMessageReader : public MessageReader,
public MessageDecoderList
Status OnMessageDecoded(std::unique_ptr<Message> message) override {
message_ = std::move(message);
+ first_message_ = false;
+ return Status::OK();
+ }
+
+ Status OnMetadata() override {
+ // An IPC file starts with "ARROW1". Read as a stream, its first four bytes
+ // ("ARRO") decode as a metadata length of 0x4F525241.
+ constexpr int64_t kArrowMagicAsMetadataLength = 0x4F525241;
+ if (first_message_ && decoder_.next_required_size() ==
kArrowMagicAsMetadataLength) {
Review Comment:
This identifies only the four-byte `ARRO` prefix, not the complete six-byte
`ARROW1` file magic. Consequently, corrupt or unrelated input beginning with
`ARRO` is incorrectly described as an IPC file and receives the wrong-reader
suggestion, despite the new corrupt-data behavior. Please verify the remaining
`W1` bytes before returning this diagnostic, while preserving those bytes for
normal decoding when they do not match.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
