eldenmoon commented on code in PR #50027:
URL: https://github.com/apache/doris/pull/50027#discussion_r2044154466
##########
be/src/olap/rowset/segment_v2/variant_column_writer_impl.cpp:
##########
@@ -89,10 +89,13 @@ Status _create_column_writer(uint32_t cid, const
TabletColumn& column,
<< " need_bitmap_index: " << opt->need_bitmap_index;
// init inverted index
- if (index != nullptr &&
+ if ((index != nullptr || subcolumn_index != nullptr) &&
Review Comment:
add comment to descibe this behavior when to use index or subcolumn_index
##########
be/src/olap/rowset/segment_v2/variant_column_writer_impl.cpp:
##########
@@ -113,7 +116,7 @@ Status _create_column_writer(uint32_t cid, const
TabletColumn& column,
#undef DISABLE_INDEX_IF_FIELD_TYPE
#undef CHECK_FIELD_TYPE
-
+ LOG(INFO) << "create column writer for " << column.name() << " type: " <<
(int)column.type();
Review Comment:
VLOG_DEBUG
##########
be/src/olap/rowset/segment_v2/inverted_index_writer.cpp:
##########
@@ -74,10 +74,8 @@ const int DIMS = 1;
bool InvertedIndexColumnWriter::check_support_inverted_index(const
TabletColumn& column) {
// bellow types are not supported in inverted index for extracted columns
static std::set<FieldType> invalid_types = {
- FieldType::OLAP_FIELD_TYPE_DOUBLE,
- FieldType::OLAP_FIELD_TYPE_JSONB,
- FieldType::OLAP_FIELD_TYPE_FLOAT,
- };
+ FieldType::OLAP_FIELD_TYPE_DOUBLE,
FieldType::OLAP_FIELD_TYPE_JSONB,
+ FieldType::OLAP_FIELD_TYPE_FLOAT,
FieldType::OLAP_FIELD_TYPE_DECIMAL};
Review Comment:
why add OLAP_FIELD_TYPE_DECIMAL
##########
be/src/olap/types.cpp:
##########
@@ -101,6 +101,7 @@ const TypeInfo* get_scalar_type_info(FieldType field_type) {
get_scalar_type_info<FieldType::OLAP_FIELD_TYPE_IPV4>(),
get_scalar_type_info<FieldType::OLAP_FIELD_TYPE_IPV6>(),
nullptr};
+ LOG(INFO) << "field_type: " << (int)field_type;
Review Comment:
VLOG_DEBUG
##########
gensrc/proto/segment_v2.proto:
##########
@@ -204,7 +204,7 @@ message ColumnMetaPB {
optional int32 be_exec_version = 20; // used on agg_state type
optional VariantStatisticsPB variant_statistics = 21; // only used in
variant type
optional int32 variant_max_subcolumns_count = 22 [default = 0];
- optional uint32 none_null_size = 23;
+ optional uint64 none_null_size = 23;
Review Comment:
use none_null_size to distinguish typed path may not be good solution. Maybe
we should generate type info from variant pattern field like
VariantColumnWriter does
##########
regression-test/suites/variant_p0/predefine/load.groovy:
##########
@@ -17,14 +17,18 @@
suite("regression_test_variant_predefine_schema", "p0"){
sql """DROP TABLE IF EXISTS test_predefine"""
+ def count = "0"
Review Comment:
need test predefined fields along with sparse column data and subcolumn data
##########
be/src/vec/columns/column_object.cpp:
##########
@@ -1757,6 +1757,9 @@ Status
ColumnObject::serialize_one_row_to_json_format(int64_t row_num, BufferWri
size_t ColumnObject::Subcolumn::get_non_null_value_size() const {
size_t res = 0;
for (const auto& part : data) {
+ if (!part->is_nullable()) {
Review Comment:
为什么加这个逻辑，很奇怪
##########
be/src/vec/columns/column_object.cpp:
##########
@@ -1840,7 +1843,7 @@ Status ColumnObject::finalize(FinalizeMode mode) {
ENABLE_CHECK_CONSISTENCY(this);
return Status::OK();
}
- DCHECK(_max_subcolumns_count >= 0) << "max subcolumns count is: " <<
_max_subcolumns_count;
Review Comment:
why remove this logic
##########
gensrc/proto/segment_v2.proto:
##########
@@ -204,7 +204,7 @@ message ColumnMetaPB {
optional int32 be_exec_version = 20; // used on agg_state type
optional VariantStatisticsPB variant_statistics = 21; // only used in
variant type
optional int32 variant_max_subcolumns_count = 22 [default = 0];
- optional uint32 none_null_size = 23;
+ optional uint64 none_null_size = 23;
Review Comment:
maybe use generate_sub_column_info to generated subcolumn?so the later
`alter modify` operation could be more easy to implement
--
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]
