GH-33823: [C++] Improve error messages when opening files that are the wrong format#49771
GH-33823: [C++] Improve error messages when opening files that are the wrong format#49771RobertLD wants to merge 5 commits intoapache:mainfrom
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
d7f5423 to
abd2b9c
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves user-facing error messages in the C++ IPC readers/decoders when callers attempt to open an IPC File as a Stream (or vice versa), providing more actionable guidance aligned with GH-33823.
Changes:
- Update the IPC file reader footer/magic check failure message to suggest the streaming reader when appropriate.
- Add a heuristic in the IPC stream message decoder to detect an IPC file magic prefix being misinterpreted as a stream message length, and emit a more instructive error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cpp/src/arrow/ipc/reader.cc |
Improves the “not an Arrow file” error when the footer magic doesn’t match, suggesting the streaming reader. |
cpp/src/arrow/ipc/message.cc |
Adds detection for IPC File magic bytes when decoding a stream message and returns a more targeted “wrong format” error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Decent feedback Mr. Bot, I'll make changes shortly. Honestly impressed the LLM review had such good context. Good bot |
Create more generic message instead of referencing c++ interface Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Similarly, keep messages more generic and not referencing the c++ interface Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@kou Comments addressed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Could you add tests for these cases? |
We want to ensure we're providing advice to the user that is correct/actionable.
|
Added a pos/neg case, kept the string matching minimal so the tests weren't as flakey is the messages get reworked in another way in the future. If it's standard practice to match the entire err message I can make that change over a subset match @kou |
Rationale for this change
The original error messages did not provide instruction to users on how to best correct their usage
What changes are included in this PR?
Improving the messages
Are these changes tested?
Current unit tests were executed, no new tests were introduced
Are there any user-facing changes?
Yes, error messages have been updated that are surfaced to users