Copilot commented on code in PR #60930:
URL: https://github.com/apache/doris/pull/60930#discussion_r2868938072
##########
be/src/olap/rowset/segment_v2/variant/nested_group_provider.cpp:
##########
@@ -102,6 +102,14 @@ class DefaultNestedGroupReadProvider final : public
NestedGroupReadProvider {
return Status::NotSupported(
"NestedGroup element-to-parent mapping is not available in
this build");
}
+
+ Status create_root_merge_iterator(ColumnIteratorUPtr base_iterator,
+ const NestedGroupReaders& /*readers*/,
+ const StorageReadOptions* /*opt*/,
+ ColumnIteratorUPtr* out) override {
+ *out = std::move(base_iterator);
+ return Status::OK();
+ }
Review Comment:
The new `create_root_merge_iterator` virtual method added to
`NestedGroupReadProvider` has no test in `nested_group_provider_test.cpp`, even
though all other virtual methods of `DefaultNestedGroupReadProvider` (e.g.,
`InitReadersCEBehavior`, `CreateNestedGroupIteratorCEBehavior`,
`MapElementsToParentOrdsCEBehavior`, `GetTotalElementsCEBehavior`) are covered
by individual test cases. A corresponding test for the CE no-op behavior of
`create_root_merge_iterator` (which should pass through `base_iterator`
unchanged and return `OK`) is missing.
##########
be/src/olap/rowset/segment_v2/variant/nested_group_provider.h:
##########
@@ -214,6 +211,14 @@ class NestedGroupReadProvider {
uint64_t* total_elements) const = 0;
// Map element-level bitmap to row-level bitmap through the nested group
chain.
+ // Create an iterator that wraps |base_iterator| with root-level NG merge
logic.
+ // For root variant reads, top-level NestedGroup arrays must be merged
back into
+ // the reconstructed variant. CE no-op returns base_iterator unchanged.
Review Comment:
The doc comment at line 213 (`// Map element-level bitmap to row-level
bitmap through the nested group chain.`) belongs to
`map_elements_to_parent_ords` but is now misplaced above
`create_root_merge_iterator`. The comment block at lines 213–216 contains two
semantically unrelated sentences: the first sentence describes
`map_elements_to_parent_ords`, while the remaining sentences describe
`create_root_merge_iterator`. The original `// Map element-level bitmap...`
comment should be moved to immediately precede `map_elements_to_parent_ords` at
line 222.
##########
be/src/olap/rowset/segment_v2/variant/nested_group_path.h:
##########
@@ -35,6 +39,10 @@ inline bool contains_nested_group_marker(std::string_view
path) {
return path.find(kNestedGroupMarker) != std::string_view::npos;
}
+inline bool is_materialized_root_union_fallback_leaf(std::string_view path) {
+ return is_root_nested_group_path(path) &&
!contains_nested_group_marker(path);
+}
Review Comment:
The new `is_root_nested_group_path` and
`is_materialized_root_union_fallback_leaf` helpers added to
`nested_group_path.h` have no test cases in `nested_group_path_test.cpp`, while
all other functions in that header (`contains_nested_group_marker`,
`strip_nested_group_marker`, `build_nested_group_offsets_column_name`,
`ends_with`, constants) are individually tested. Tests for these two new
helpers should be added to maintain coverage parity.
##########
be/src/olap/rowset/segment_v2/variant/nested_group_path.h:
##########
@@ -35,6 +39,10 @@ inline bool contains_nested_group_marker(std::string_view
path) {
return path.find(kNestedGroupMarker) != std::string_view::npos;
}
+inline bool is_materialized_root_union_fallback_leaf(std::string_view path) {
+ return is_root_nested_group_path(path) &&
!contains_nested_group_marker(path);
Review Comment:
The `is_materialized_root_union_fallback_leaf` function always returns the
same result as `is_root_nested_group_path` because `kRootNestedGroupPath`
(`"__D0_root__"`) can never contain the `kNestedGroupMarker` substring
(`"__D0_ng__"`), making the `!contains_nested_group_marker(path)` guard always
true whenever `is_root_nested_group_path(path)` is true. The extra condition
adds no discriminating power and may be misleading about the intended
semantics. If the intent is to also handle a future case where `__D0_root__`
co-occurs with the NG marker, that case should be documented explicitly, or the
function should just delegate to `is_root_nested_group_path`.
```suggestion
// Currently equivalent to is_root_nested_group_path(path). If future
paths can contain
// both the root marker and the nested group marker, update this
function accordingly.
return is_root_nested_group_path(path);
```
--
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]
