Copilot commented on code in PR #64755:
URL: https://github.com/apache/doris/pull/64755#discussion_r3461039626
##########
be/src/exec/scan/olap_scanner.cpp:
##########
@@ -301,9 +301,21 @@ Status OlapScanner::_open_impl(RuntimeState* state) {
auto res = _tablet_reader->init(_tablet_reader_params);
if (!res.ok()) {
- res.append("failed to initialize storage reader. tablet=" +
- std::to_string(_tablet_reader_params.tablet->tablet_id()) +
- ", backend=" + BackendOptions::get_localhost());
+ const auto tablet_id =
std::to_string(_tablet_reader_params.tablet->tablet_id());
+ const auto& backend = BackendOptions::get_localhost();
+ // The reader eagerly reads the first block of each rowset during
init() to seed
+ // the merge heap, so errors from evaluating pushed-down expressions
(e.g. a strict
+ // cast on an illegal string -> INVALID_ARGUMENT) can surface here.
Only label
+ // genuine storage-level failures as "failed to initialize storage
reader"; for
+ // data/expression errors keep the message neutral so users are not
misled into
+ // suspecting a corrupted tablet/segment.
+ if (res.is<ErrorCode::INVALID_ARGUMENT>()) {
+ res.append("error occurred while reading tablet=" + tablet_id + ",
backend=" + backend +
+ " (data/expression error, not a storage reader
failure)");
+ } else {
+ res.append("failed to initialize storage reader. tablet=" +
tablet_id +
+ ", backend=" + backend);
Review Comment:
Same as above: `Status::append()` does not insert whitespace, so this
context should begin with a separator to avoid producing "...<original
msg>failed to initialize..." as one word.
##########
be/src/exec/scan/olap_scanner.cpp:
##########
@@ -301,9 +301,21 @@ Status OlapScanner::_open_impl(RuntimeState* state) {
auto res = _tablet_reader->init(_tablet_reader_params);
if (!res.ok()) {
- res.append("failed to initialize storage reader. tablet=" +
- std::to_string(_tablet_reader_params.tablet->tablet_id()) +
- ", backend=" + BackendOptions::get_localhost());
+ const auto tablet_id =
std::to_string(_tablet_reader_params.tablet->tablet_id());
+ const auto& backend = BackendOptions::get_localhost();
+ // The reader eagerly reads the first block of each rowset during
init() to seed
+ // the merge heap, so errors from evaluating pushed-down expressions
(e.g. a strict
+ // cast on an illegal string -> INVALID_ARGUMENT) can surface here.
Only label
+ // genuine storage-level failures as "failed to initialize storage
reader"; for
+ // data/expression errors keep the message neutral so users are not
misled into
+ // suspecting a corrupted tablet/segment.
+ if (res.is<ErrorCode::INVALID_ARGUMENT>()) {
+ res.append("error occurred while reading tablet=" + tablet_id + ",
backend=" + backend +
+ " (data/expression error, not a storage reader
failure)");
Review Comment:
`Status::append()` concatenates directly onto the existing error message
without adding a separator (see be/src/common/status.cpp:74-81). This appended
context string should start with a space or delimiter; otherwise the
user-facing message will be glued together (e.g. "...string: ''error
occurred...") and harder to read.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
