kou commented on code in PR #48674:
URL: https://github.com/apache/arrow/pull/48674#discussion_r2659953513
##########
cpp/src/arrow/status_test.cc:
##########
@@ -342,4 +342,23 @@ TEST(StatusTest, ReturnIfNotOk) {
ASSERT_EQ(StripContext(st.message()), "StatusLike: 43");
}
+#ifdef ARROW_EXTRA_ERROR_CONTEXT
+TEST(StatusTest, ToStringWithoutContextLines) {
+ Status status = Status::IOError("base error");
+ status.AddContextLine("file1.cc", 42, "expr");
+ status.AddContextLine("file2.cc", 100, "expr");
+
+ ASSERT_EQ(status.ToStringWithoutContextLines(), "IOError: base error");
+
+ Status status2(StatusCode::Invalid,
+ "Error message\nThis line has: a colon but no digits");
+ status2.AddContextLine("file.cc", 20, "expr");
+
+ std::string result = status2.ToStringWithoutContextLines();
+ ASSERT_EQ(result.find("file.cc:20"), std::string::npos);
+ ASSERT_NE(result.find("This line has: a colon but no digits"),
std::string::npos);
+ ASSERT_NE(result.find("Error message"), std::string::npos);
Review Comment:
Could you check the exact expected message instead for easy to debug failure
message? If we use `find()` and `std::string::npos`, we can't show the actual
message in the failure log.
```suggestion
ASSERT_EQ(status2.ToStringWithoutContextLines(),
"Error message\nThis line has: a colon but no digits");
```
##########
cpp/src/arrow/status.cc:
##########
@@ -131,8 +132,15 @@ std::string Status::ToStringWithoutContextLines() const {
if (last_new_line_position == std::string::npos) {
break;
}
- // TODO: We may want to check /:\d+ /
- if (message.find(":", last_new_line_position) == std::string::npos) {
+ // Check for the pattern ":\d+" (colon followed by digits) to identify
+ // context lines in the format "filename:line expr"
+ auto colon_position = message.find(":", last_new_line_position);
+ if (colon_position == std::string::npos) {
+ break;
+ }
+ // Verify that the colon is followed by at least one digit
+ if (colon_position + 1 >= message.size() ||
+ !std::isdigit(static_cast<unsigned char>(message[colon_position +
1]))) {
Review Comment:
Can we also check the `+ ` part in `/:\d+ /`?
--
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]
