eldenmoon commented on code in PR #33493:
URL: https://github.com/apache/doris/pull/33493#discussion_r1560392867


##########
be/src/vec/columns/column_object.h:
##########
@@ -456,13 +460,11 @@ class ColumnObject final : public COWHelper<IColumn, 
ColumnObject> {
         LOG(FATAL) << "should not call the method in column object";
     }
 
-    void replace_column_data(const IColumn&, size_t row, size_t self_row) 
override {
-        LOG(FATAL) << "should not call the method in column object";
-    }
+    bool is_variable_length() const override { return true; }
 
-    void replace_column_data_default(size_t self_row) override {
-        LOG(FATAL) << "should not call the method in column object";
-    }
+    void replace_column_data(const IColumn&, size_t row, size_t self_row) 
override;
+
+    void replace_column_data_default(size_t self_row) override;

Review Comment:
   replace only called in ColumnObject::replace_column_data



##########
be/src/vec/columns/column.h:
##########
@@ -673,6 +673,10 @@ class IColumn : public COW<IColumn> {
 
     virtual void replace_column_null_data(const uint8_t* __restrict null_map) 
{}
 
+    virtual void replace(const Field& f, size_t self_row) {
+        LOG(FATAL) << "Method replace is not supported for " << get_name();

Review Comment:
   i think let it crash or throw exception instead of implement the interface 
is better, since some column may not implement it, so crash is more better than 
implement in wrong way.



##########
be/src/vec/columns/column_object.cpp:
##########
@@ -1513,4 +1514,78 @@ Status ColumnObject::sanitize() const {
     return Status::OK();
 }
 
+void ColumnObject::Subcolumn::replace(Field field, int row_num) {
+    finalize();
+    Field new_field;
+    if (!field.is_null()) {
+        convert_field_to_type(field, *least_common_type.get(), &new_field);
+        data.back()->replace(new_field, row_num);
+    } else {
+        data.back()->replace(Null(), row_num);
+    }
+}
+
+void ColumnObject::replace_column_data(const IColumn& col, size_t row, size_t 
self_row) {
+    DCHECK(size() > self_row);
+    DCHECK(col.size() > row);
+#ifndef NDEBUG
+    check_consistency();
+#endif
+    Field var_filed = VariantMap();
+    col.get(row, var_filed);
+    const auto& object = var_filed.get<const VariantMap&>();
+    phmap::flat_hash_set<std::string> inserted;
+    size_t old_size = size();
+    for (const auto& [key_str, value] : object) {
+        PathInData key;
+        if (!key_str.empty()) {
+            key = PathInData(key_str);
+        }
+        if (value.is_null()) {
+            // ignore nulls
+            continue;
+        }
+        inserted.insert(key_str);
+        if (!has_subcolumn(key)) {
+            auto most_common_type = std::make_shared<MostCommonType>();
+            auto type = is_nullable ? make_nullable(most_common_type) : 
most_common_type;
+            auto column = type->create_column();
+            column->resize(old_size);
+            bool succ = add_sub_column(key, std::move(column), type);
+            if (!succ) {
+                throw doris::Exception(doris::ErrorCode::INVALID_ARGUMENT,
+                                       "Failed to add sub column {}", 
key.get_path());
+            }
+        }
+        auto* subcolumn = get_subcolumn(key);
+        if (!subcolumn) {
+            doris::Exception(doris::ErrorCode::INVALID_ARGUMENT,
+                             fmt::format("Failed to find sub column {}", 
key.get_path()));
+        }
+        subcolumn->replace(value, self_row);
+    }
+    for (auto& entry : subcolumns) {
+        if (!inserted.contains(entry->path.get_path())) {
+            entry->data.replace(Null(), self_row);
+        }
+    }
+
+#ifndef NDEBUG
+    check_consistency();
+#endif
+}
+
+void ColumnObject::replace_column_data_default(size_t self_row) {
+#ifndef NDEBUG
+    check_consistency();
+#endif
+    if (!is_finalized()) {
+        finalize();
+    }
+    Field null;
+    for (auto& entry : subcolumns) {
+        entry->data.data[0]->replace(null, self_row);

Review Comment:
   we could not clear, this replace semantic means to replace null at row pos 
`self_row`, not clear them



-- 
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: commits-unsubscr...@doris.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org

Reply via email to