github-actions[bot] commented on code in PR #65087:
URL: https://github.com/apache/doris/pull/65087#discussion_r3510066241
##########
be/src/core/column/column_variant.cpp:
##########
@@ -826,19 +825,28 @@ size_t ColumnVariant::allocated_bytes() const {
return res;
}
-void ColumnVariant::for_each_subcolumn(ColumnCallback callback) {
Review Comment:
`ColumnVariant` is still missing from the shared-child COW tests. This PR
changes `ColumnVariant::mutate_subcolumns()` to detach every materialized
subcolumn part plus `serialized_sparse_column` and
`serialized_doc_value_column`, but `column_mutate_subcolumns_test.cpp` only
exercises Nullable/Array/Map/Const/Struct. Existing Variant tests cover general
filter behavior and `clone()` deep-copying, but they do not keep aliases to
these child columns and then call `IColumn::mutate(std::move(variant))` on an
exclusive parent. A missed child here would leave the mutated Variant sharing
storage with another owner, which is exactly the failure mode these new tests
are meant to catch. Please add a Variant case that aliases at least a
materialized subcolumn and the serialized sparse/doc-value map columns, mutates
the Variant, then writes or filters the result and asserts the aliases remain
unchanged.
##########
be/src/core/column/column_variant.cpp:
##########
@@ -826,19 +825,28 @@ size_t ColumnVariant::allocated_bytes() const {
return res;
}
-void ColumnVariant::for_each_subcolumn(ColumnCallback callback) {
+void ColumnVariant::mutate_subcolumns() {
Review Comment:
`ColumnVariant` is still missing from the shared-child COW tests. This PR
changes `ColumnVariant::mutate_subcolumns()` to detach every materialized
subcolumn part plus `serialized_sparse_column` and
`serialized_doc_value_column`, but `column_mutate_subcolumns_test.cpp` only
exercises Nullable/Array/Map/Const/Struct. Existing Variant tests cover general
filter behavior and `clone()` deep-copying, but they do not keep aliases to
these child columns and then call `IColumn::mutate(std::move(variant))` on an
exclusive parent. A missed child here would leave the mutated Variant sharing
storage with another owner, which is exactly the failure mode these new tests
are meant to catch. Please add a Variant case that aliases at least a
materialized subcolumn and the serialized sparse/doc-value map columns, mutates
the Variant, then writes or filters the result and asserts the aliases remain
unchanged.
--
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]
