github-actions[bot] commented on code in PR #61633:
URL: https://github.com/apache/doris/pull/61633#discussion_r2975906599
##########
be/src/exec/operator/materialization_opertor.cpp:
##########
@@ -80,6 +80,14 @@ Status MaterializationSharedState::merge_multi_response() {
}
}
+ // return error if any column in response block is not compatible with
source block column
+ for (int k = 0; k < response_blocks[i].columns(); ++k) {
+ const auto& resp_col_type =
response_blocks[i].get_datatype_by_position(k);
+ for (const auto& [_, source_block_rows] : block_maps) {
+ RETURN_IF_ERROR(resp_col_type->check_column(
Review Comment:
**[AGENTS.md Violation] Defensive programming: should use `DORIS_CHECK`
instead of `RETURN_IF_ERROR`**
Per AGENTS.md:
> *Assert correctness only—never use defensive programming with `if` or
similar constructs. Any `if` check for errors must have a clearly known
inevitable failure path (not speculation).*
Both `response_blocks[i]` and the deserialized `partial_block` in
`block_maps` are derived from the same `SlotDescriptor` set — `response_blocks`
is built in `init_multi_requests()` (line 258) from `slots_res`, and the remote
BE constructs its response block from the same slots serialized into the
protobuf request (line 255). Under normal operation, their column types are
guaranteed to match.
The PR description uses speculative language ("in case of any column type
mismatch") and no issue is linked demonstrating a real failure path. A column
type mismatch here would indicate a fundamental invariant violation (corrupted
schema propagation), and per AGENTS.md the correct response is to crash, not
silently return an error.
Suggested change:
```cpp
for (int k = 0; k < response_blocks[i].columns(); ++k) {
const auto& resp_col_type =
response_blocks[i].get_datatype_by_position(k);
for (const auto& [_, source_block_rows] : block_maps) {
DORIS_CHECK(resp_col_type->check_column(
*source_block_rows.first.get_by_position(k).column).ok());
}
}
```
This is not inside a performance-sensitive inner loop (it iterates over
columns × backends, both small), so `DORIS_CHECK` is appropriate over `DCHECK`.
--
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]
