github-actions[bot] commented on code in PR #63001:
URL: https://github.com/apache/doris/pull/63001#discussion_r3218653325
##########
be/src/exec/common/variant_util.cpp:
##########
@@ -2217,10 +2216,10 @@ Status _parse_and_materialize_variant_columns(Block&
block,
for (size_t i = 0; i < variant_pos.size(); ++i) {
auto column_ref = block.get_by_position(variant_pos[i]).column;
bool is_nullable = column_ref->is_nullable();
- MutableColumnPtr var_column = column_ref->assume_mutable();
+ MutableColumnPtr var_column = std::move(*column_ref).mutate();
if (is_nullable) {
const auto& nullable = assert_cast<const
ColumnNullable&>(*column_ref);
- var_column = nullable.get_nested_column_ptr()->assume_mutable();
+ var_column = std::move(*nullable.get_nested_column_ptr()).mutate();
}
Review Comment:
This still bypasses COW for nullable variants. `var_column` at line 2219 is
the deep-mutated nullable owner, but this branch then looks back at the
original `column_ref` and mutates its nested column. If the nullable parent is
shared, the nested column can still have `use_count()==1` because it is only
referenced from that shared parent object, so `mutate()` may return the
original nested `ColumnVariant` and `finalize()`/`ensure_root_node_type()`
below mutates all aliases of the nullable column. Please take the nested column
from the already-mutated nullable owner instead, for example by casting
`var_column.get()` to `ColumnNullable*` and using its mutable nested column
before dropping the wrapper.
##########
be/src/core/column/column_map.cpp:
##########
@@ -518,59 +518,82 @@ void ColumnMap::insert_range_from_ignore_overflow(const
IColumn& src, size_t sta
}
ColumnPtr ColumnMap::filter(const Filter& filt, ssize_t result_size_hint)
const {
- auto k_arr =
- ColumnArray::create(keys_column->assume_mutable(),
offsets_column->assume_mutable())
- ->filter(filt, result_size_hint);
- auto v_arr =
- ColumnArray::create(values_column->assume_mutable(),
offsets_column->assume_mutable())
- ->filter(filt, result_size_hint);
+ // For const filter we must clone subcolumns so the original ColumnMap
remains intact.
+ // IColumn::mutate(copy) clones if use_count>1, returns self if exclusive.
+ auto offsets_mut =
IColumn::mutate(static_cast<IColumn::Ptr>(offsets_column));
+ MutableColumnPtr offsets_copy = offsets_mut->clone_empty();
+ offsets_copy->insert_range_from(*offsets_mut, 0, offsets_mut->size());
+ auto k_arr =
ColumnArray::create(IColumn::mutate(static_cast<IColumn::Ptr>(keys_column)),
+ std::move(offsets_mut))
Review Comment:
This pre-clones the complete key/value/offset columns before calling
`ColumnArray::filter(const Filter&, ...)`, but that overload already builds and
returns a new filtered array without mutating its input. For large map columns
this turns a const filter into an extra full unfiltered copy, and the final
`ColumnMap::create(...)` can clone the filtered outputs again while the
temporary arrays still hold aliases. The same pattern appears in `permute()`
below. Please keep the inputs shared/const for these return-new operations and
only detach where an in-place mutation is actually performed.
--
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]
