github-actions[bot] commented on code in PR #26749:
URL: https://github.com/apache/doris/pull/26749#discussion_r1389041132
##########
be/src/olap/rowset/segment_v2/segment.h:
##########
@@ -127,6 +141,35 @@ class Segment : public
std::enable_shared_from_this<Segment> {
void remove_from_segment_cache() const;
+ // Get the inner file column's data type
+ // ignore_chidren set to false will treat field as variant
+ // when it contains children with field paths
+ std::shared_ptr<const vectorized::IDataType> get_data_type_of(const Field&
filed,
Review Comment:
warning: function 'std::doris::segment_v2::Segment::get_data_type_of' has a
definition with different parameter names
[readability-inconsistent-declaration-parameter-name]
```cpp
std::shared_ptr<const vectorized::IDataType> get_data_type_of(const
Field& filed,
^
```
<details>
<summary>Additional context</summary>
**be/src/olap/rowset/segment_v2/segment.cpp:337:** the definition seen here
```cpp
vectorized::DataTypePtr Segment::get_data_type_of(const Field& field, bool
ignore_children) const {
^
```
**be/src/olap/rowset/segment_v2/segment.h:146:** differing parameters are
named here: ('filed'), in definition: ('field')
```cpp
std::shared_ptr<const vectorized::IDataType> get_data_type_of(const
Field& filed,
^
```
</details>
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -1810,6 +1877,49 @@
return status;
}
+Status SegmentIterator::_convert_to_expected_type(const std::vector<ColumnId>&
col_ids) {
+ for (ColumnId i : col_ids) {
+ if (_current_return_columns[i] == nullptr || _converted_column_ids[i]
||
+ _is_pred_column[i]) {
+ continue;
+ }
+ if (!_segment->same_with_storage_type(
+ i, *_schema, _opts.io_ctx.reader_type !=
ReaderType::READER_QUERY)) {
+ const Field* field_type = _schema->column(i);
+ vectorized::DataTypePtr expected_type =
Schema::get_data_type_ptr(*field_type);
+ vectorized::DataTypePtr file_column_type =
_storage_name_and_type[i].second;
+ vectorized::ColumnPtr expected;
+ vectorized::ColumnPtr original =
+ _current_return_columns[i]->assume_mutable()->get_ptr();
+ RETURN_IF_ERROR(vectorized::schema_util::cast_column({original,
file_column_type, ""},
+
expected_type, &expected));
+ _current_return_columns[i] = expected->assume_mutable();
+ _converted_column_ids[i] = 1;
+ VLOG_DEBUG << fmt::format("Convert {} fom file column type {} to
{}, num_rows {}",
+ field_type->path().get_path(),
file_column_type->get_name(),
+ expected_type->get_name(),
+ _current_return_columns[i]->size());
+ }
+ }
+ return Status::OK();
+}
+
+Status SegmentIterator::copy_column_data_by_selector(vectorized::IColumn*
input_col_ptr,
Review Comment:
warning: method 'copy_column_data_by_selector' can be made static
[readability-convert-member-functions-to-static]
```suggestion
static Status
SegmentIterator::copy_column_data_by_selector(vectorized::IColumn*
input_col_ptr,
```
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -1810,6 +1877,49 @@ Status SegmentIterator::next_batch(vectorized::Block*
block) {
return status;
}
+Status SegmentIterator::_convert_to_expected_type(const std::vector<ColumnId>&
col_ids) {
Review Comment:
warning: method '_convert_to_expected_type' can be made static
[readability-convert-member-functions-to-static]
```suggestion
static Status SegmentIterator::_convert_to_expected_type(const
std::vector<ColumnId>& col_ids) {
```
##########
be/src/vec/exec/scan/new_olap_scan_node.cpp:
##########
@@ -416,6 +421,48 @@
return fmt::format("VNewOlapScanNode({0})", _olap_scan_node.table_name);
}
+void NewOlapScanNode::_filter_and_collect_suspended_eliminate_cast_column(
+ const VExpr* expr,
+ phmap::flat_hash_map<std::string, std::vector<PrimitiveType>>&
colname_to_cast_types) {
Review Comment:
warning: all parameters should be named in a function
[readability-named-parameter]
```suggestion
phmap::flat_hash_map<std::string, std::vector<PrimitiveType>
/*unused*/>& colname_to_cast_types) {
```
##########
be/src/vec/exec/scan/new_olap_scanner.cpp:
##########
@@ -408,6 +412,55 @@ Status NewOlapScanner::_init_tablet_reader_params(
return Status::OK();
}
+vectorized::PathInData NewOlapScanner::_build_path(SlotDescriptor* slot) {
Review Comment:
warning: method '_build_path' can be made static
[readability-convert-member-functions-to-static]
be/src/vec/exec/scan/new_olap_scanner.h:95:
```diff
- vectorized::PathInData _build_path(SlotDescriptor* slot);
+ static vectorized::PathInData _build_path(SlotDescriptor* slot);
```
##########
be/src/vec/columns/column_object.cpp:
##########
@@ -1390,78 +1411,20 @@ Status ColumnObject::extract_root(const PathInData&
path, MutableColumnPtr& dst)
return Status::OK();
}
-template <typename ColumnInserterFn>
-void align_variant_by_name_and_type(ColumnObject& dst, const ColumnObject&
src, size_t row_cnt,
- ColumnInserterFn inserter) {
- CHECK(dst.is_finalized());
- if (!src.is_finalized()) {
- const_cast<ColumnObject&>(src).finalize();
- }
- // Use rows() here instead of size(), since size() will check_consistency
- // but we could not check_consistency since num_rows will be upgraded even
- // if src and dst is empty, we just increase the num_rows of dst and fill
- // num_rows of default values when meet new data
- size_t num_rows = dst.rows();
- for (auto& entry : dst.get_subcolumns()) {
- const auto* src_subcol = src.get_subcolumn(entry->path);
- if (src_subcol == nullptr) {
- entry->data.get_finalized_column().insert_many_defaults(row_cnt);
- } else {
- // It's the first time alignment, so that we should build it
- if (entry->data.get_least_common_type()->get_type_id() ==
TypeIndex::Nothing) {
-
entry->data.add_new_column_part(src_subcol->get_least_common_type());
- }
- // TODO handle type confict hereļ¼ like ColumnObject before
- CHECK(entry->data.get_least_common_type()->equals(
- *src_subcol->get_least_common_type()));
- const auto& src_column = src_subcol->get_finalized_column();
- inserter(src_column, &entry->data.get_finalized_column());
- }
- dst.set_num_rows(entry->data.get_finalized_column().size());
- }
- for (const auto& entry : src.get_subcolumns()) {
- // encounter a new column
- const auto* dst_subcol = dst.get_subcolumn(entry->path);
- if (dst_subcol == nullptr) {
- auto type = entry->data.get_least_common_type();
- auto new_column = type->create_column();
- new_column->insert_many_defaults(num_rows);
- inserter(entry->data.get_finalized_column(), new_column.get());
- dst.set_num_rows(new_column->size());
- dst.add_sub_column(entry->path, std::move(new_column));
- }
- }
- num_rows += row_cnt;
- if (dst.empty()) {
- dst.incr_num_rows(row_cnt);
- }
-#ifndef NDEBUG
- // Check all columns rows matched
- for (const auto& entry : dst.get_subcolumns()) {
- DCHECK_EQ(entry->data.get_finalized_column().size(), num_rows);
- }
-#endif
-}
-
void ColumnObject::append_data_by_selector(MutableColumnPtr& res,
const IColumn::Selector& selector)
const {
- // append by selector with alignment
- ColumnObject& dst_column = *assert_cast<ColumnObject*>(res.get());
- align_variant_by_name_and_type(dst_column, *this, selector.size(),
- [&selector](const IColumn& src, IColumn*
dst) {
- auto mutable_dst =
dst->assume_mutable();
-
src.append_data_by_selector(mutable_dst, selector);
- });
+ return append_data_by_selector_impl<ColumnObject>(res, selector);
}
void ColumnObject::insert_indices_from(const IColumn& src, const int*
indices_begin,
const int* indices_end) {
- // insert_indices_from with alignment
- const ColumnObject& src_column = *check_and_get_column<ColumnObject>(src);
- align_variant_by_name_and_type(*this, src_column, indices_end -
indices_begin,
- [indices_begin, indices_end](const IColumn&
src, IColumn* dst) {
- dst->insert_indices_from(src,
indices_begin, indices_end);
- });
+ for (auto x = indices_begin; x != indices_end; ++x) {
Review Comment:
warning: 'auto x' can be declared as 'const auto *x'
[readability-qualified-auto]
```suggestion
for (const auto *x = indices_begin; x != indices_end; ++x) {
```
##########
be/src/vec/exec/scan/new_olap_scanner.cpp:
##########
@@ -408,6 +412,55 @@
return Status::OK();
}
+vectorized::PathInData NewOlapScanner::_build_path(SlotDescriptor* slot) {
+ PathInDataBuilder path_builder;
+ const std::string& col_name = slot->col_name_lower_case();
+ auto delimeter_index = col_name.find(".");
+ std::string_view root_name = delimeter_index == std::string::npos
+ ? col_name
+ : std::string_view(col_name.data(),
delimeter_index);
+ path_builder = path_builder.append(root_name, false);
+ for (const std::string& path : slot->column_paths()) {
+ path_builder.append(path, false);
+ }
+ return path_builder.build();
+}
+
+Status NewOlapScanner::_init_variant_columns() {
Review Comment:
warning: method '_init_variant_columns' can be made static
[readability-convert-member-functions-to-static]
be/src/vec/exec/scan/new_olap_scanner.h:96:
```diff
- [[nodiscard]] Status _init_variant_columns();
+ [[nodiscard]] static Status _init_variant_columns();
```
##########
be/src/vec/exec/scan/new_olap_scan_node.cpp:
##########
@@ -416,6 +421,48 @@
return fmt::format("VNewOlapScanNode({0})", _olap_scan_node.table_name);
}
+void NewOlapScanNode::_filter_and_collect_suspended_eliminate_cast_column(
+ const VExpr* expr,
+ phmap::flat_hash_map<std::string, std::vector<PrimitiveType>>&
colname_to_cast_types) {
+ auto* cast_expr = dynamic_cast<const VCastExpr*>(expr);
Review Comment:
warning: 'auto *cast_expr' can be declared as 'const auto *cast_expr'
[readability-qualified-auto]
```suggestion
const auto* cast_expr = dynamic_cast<const VCastExpr*>(expr);
```
##########
be/src/vec/exec/scan/new_olap_scan_node.cpp:
##########
@@ -416,6 +421,48 @@ std::string NewOlapScanNode::get_name() {
return fmt::format("VNewOlapScanNode({0})", _olap_scan_node.table_name);
}
+void NewOlapScanNode::_filter_and_collect_suspended_eliminate_cast_column(
Review Comment:
warning: method '_filter_and_collect_suspended_eliminate_cast_column' can be
made static [readability-convert-member-functions-to-static]
```suggestion
static void
NewOlapScanNode::_filter_and_collect_suspended_eliminate_cast_column(
```
--
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]
