github-actions[bot] commented on code in PR #61383:
URL: https://github.com/apache/doris/pull/61383#discussion_r2939187574
##########
be/src/exec/common/variant_util.cpp:
##########
@@ -969,6 +983,16 @@ Status VariantCompactionUtil::check_path_stats(const
std::vector<RowsetSharedPtr
return Status::OK();
}
}
+ for (const auto& column : output->tablet_schema()->columns()) {
+ if (column->is_variant_type() &&
!should_check_variant_path_stats(*column)) {
+ return Status::OK();
+ }
+ }
+ for (const auto& column : output->tablet_schema()->columns()) {
+ if (!column->is_variant_type()) {
+ continue;
+ }
+ }
Review Comment:
**[Bug: Dead code / empty loop body]**
This loop iterates over columns, skips non-variant types via `continue`, but
then does **nothing** for variant-type columns — the loop body is entirely
empty. This appears to be leftover code from an incomplete refactoring.
If the intent was to further filter variant columns (e.g., skip columns
matching some condition before proceeding to the stats aggregation below), the
filtering logic is missing. If the intent was just the early-return in the loop
above (lines 986-990), then this loop should be removed entirely.
```cpp
// This loop does nothing — remove it or add the intended logic
for (const auto& column : output->tablet_schema()->columns()) {
if (!column->is_variant_type()) {
continue;
}
// <-- empty body for variant columns
}
```
##########
be/src/exec/common/variant_util.cpp:
##########
@@ -969,6 +983,16 @@ Status VariantCompactionUtil::check_path_stats(const
std::vector<RowsetSharedPtr
return Status::OK();
}
}
+ for (const auto& column : output->tablet_schema()->columns()) {
+ if (column->is_variant_type() &&
!should_check_variant_path_stats(*column)) {
+ return Status::OK();
+ }
Review Comment:
**[Correctness concern: early-return skips all variant columns]**
This loop returns `OK` as soon as it finds **any** variant column where
`should_check_variant_path_stats()` returns false (i.e., any column with nested
groups enabled). This means if the schema has **two** variant columns — one
with nested groups and one without — the path stats check for the
non-nested-group variant is skipped entirely.
Was the intent to skip only the nested-group variant columns from path stats
checking, while still checking the others? If so, this should collect which
variant UIDs to skip rather than returning early for all columns.
Similarly, the equivalent check at lines 943-951 in the same function has
the same early-return-for-all semantics from input rowset columns.
--
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]
