You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2021/06/07 04:18:57 UTC
[GitHub] [incubator-doris] liutang123 opened a new pull request #5983: Save array's size instead of offset.
liutang123 opened a new pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983
## Proposed changes
In #5142 , chaoyli gives some suggestions:
1. ArrayColumnWriter writes null as separate column.
2. save array's size instead of child column's offset.
## Types of changes
What types of changes does your code introduce to Doris?
_Put an `x` in the boxes that apply_
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation Update (if none of the other choices apply)
- [ ] Code refactor (Modify the code structure, format the code, etc...)
## Checklist
_Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code._
- [x] chaoyli have created an issue on (Fix #5142) and described the bug/feature there in detail
- [x] Compiling and unit tests pass locally with my changes
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] If these changes need document changes, I have updated the document
- [ ] Any dependent changes have been merged
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg commented on a change in pull request #5983: Save array's size instead of offset.
Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983#discussion_r648170632
##########
File path: be/src/olap/column_vector.cpp
##########
@@ -127,8 +127,22 @@ Status ColumnVectorBatch::create(size_t init_capacity, bool is_nullable, const T
return Status::NotSupported(
"When create ArrayColumnVectorBatch, `Field` is indispensable");
}
+
+ std::unique_ptr<ColumnVectorBatch> elements;
+ auto array_type_info = reinterpret_cast<const ArrayTypeInfo*>(type_info);
+ RETURN_IF_ERROR(ColumnVectorBatch::create(init_capacity * 2, field->get_sub_field(0)->is_nullable(),
+ array_type_info->item_type_info(), field->get_sub_field(0), &elements));
+
+ std::unique_ptr<ColumnVectorBatch> offsets;
Review comment:
array element size now not offset ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] liutang123 commented on pull request #5983: Save array's size instead of offset.
Posted by GitBox <gi...@apache.org>.
liutang123 commented on pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983#issuecomment-855563882
@yangzhg cc @wangbo
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] liutang123 commented on a change in pull request #5983: [ARRAY]Save array's size instead of offset.
Posted by GitBox <gi...@apache.org>.
liutang123 commented on a change in pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983#discussion_r648829333
##########
File path: be/src/olap/column_vector.cpp
##########
@@ -127,8 +127,22 @@ Status ColumnVectorBatch::create(size_t init_capacity, bool is_nullable, const T
return Status::NotSupported(
"When create ArrayColumnVectorBatch, `Field` is indispensable");
}
+
+ std::unique_ptr<ColumnVectorBatch> elements;
+ auto array_type_info = reinterpret_cast<const ArrayTypeInfo*>(type_info);
+ RETURN_IF_ERROR(ColumnVectorBatch::create(init_capacity * 2, field->get_sub_field(0)->is_nullable(),
+ array_type_info->item_type_info(), field->get_sub_field(0), &elements));
+
+ std::unique_ptr<ColumnVectorBatch> offsets;
+ TypeInfo* bigint_type_info = get_scalar_type_info(FieldType::OLAP_FIELD_TYPE_UNSIGNED_BIGINT);
+ RETURN_IF_ERROR(ColumnVectorBatch::create(init_capacity + 1, false,
+ bigint_type_info, nullptr, &offsets));
+
std::unique_ptr<ColumnVectorBatch> local(
Review comment:
It means a local variable.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg merged pull request #5983: [ARRAY]Save array's size instead of offset.
Posted by GitBox <gi...@apache.org>.
yangzhg merged pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg commented on a change in pull request #5983: Save array's size instead of offset.
Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983#discussion_r647122518
##########
File path: be/src/olap/column_vector.cpp
##########
@@ -173,31 +186,31 @@ Status ArrayColumnVectorBatch::resize(size_t new_cap) {
if (capacity() < new_cap) {
RETURN_IF_ERROR(ColumnVectorBatch::resize(new_cap));
_data.resize(new_cap);
- _item_offsets.resize(new_cap + 1);
+ _offsets->resize(new_cap + 1);
}
return Status::OK();
}
-void ArrayColumnVectorBatch::put_item_ordinal(segment_v2::ordinal_t* ordinals, size_t start_idx,
- size_t size) {
- size_t first_offset = _item_offsets[start_idx];
- segment_v2::ordinal_t first_ordinal = ordinals[0];
- size_t i = 0;
- while (++i < size) {
- _item_offsets[start_idx + i] = first_offset + (ordinals[i] - first_ordinal);
+void ArrayColumnVectorBatch::change_sizes_to_offsets(size_t start_idx, size_t size) {
Review comment:
get_offset maybe better
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg commented on a change in pull request #5983: Save array's size instead of offset.
Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983#discussion_r648170632
##########
File path: be/src/olap/column_vector.cpp
##########
@@ -127,8 +127,22 @@ Status ColumnVectorBatch::create(size_t init_capacity, bool is_nullable, const T
return Status::NotSupported(
"When create ArrayColumnVectorBatch, `Field` is indispensable");
}
+
+ std::unique_ptr<ColumnVectorBatch> elements;
+ auto array_type_info = reinterpret_cast<const ArrayTypeInfo*>(type_info);
+ RETURN_IF_ERROR(ColumnVectorBatch::create(init_capacity * 2, field->get_sub_field(0)->is_nullable(),
+ array_type_info->item_type_info(), field->get_sub_field(0), &elements));
+
+ std::unique_ptr<ColumnVectorBatch> offsets;
Review comment:
use array element size now not offset ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg commented on a change in pull request #5983: Save array's size instead of offset.
Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983#discussion_r647123074
##########
File path: be/src/olap/column_vector.h
##########
@@ -181,14 +216,30 @@ class ArrayColumnVectorBatch : public ColumnVectorBatch {
// Get thr idx's cell_ptr for write
uint8_t* mutable_cell_ptr(size_t idx) override { return reinterpret_cast<uint8*>(&_data[idx]); }
- size_t item_offset(size_t idx) const { return _item_offsets[idx]; }
+ size_t item_offset(size_t idx) const {
+ return *(_offsets->scalar_cell_ptr(idx));
+ }
- // From `start_idx`, put `size` ordinals to _item_offsets
+ // From `start_idx`, change `size` size to offset
// Ex:
- // original _item_offsets: 0 3 5 9; ordinals to be added: 100 105 111; size: 3; satart_idx: 3
- // --> _item_offsets: 0 3 5 (9 + 100 - 100) (9 + 105 - 100) (9 + 111 - 100)
- // _item_offsets becomes 0 3 5 9 14 20
- void put_item_ordinal(segment_v2::ordinal_t* ordinals, size_t start_idx, size_t size);
+ // start_idx = 3, size = 2
+ //
+ // before change:
+ // start_idx
+ // |
+ // _offsets: 0 3 5 2 1
+ //
+ // after change:
+ // _offsets: 0 3 5 7 8
+ void change_sizes_to_offsets(size_t start_idx, size_t size);
Review comment:
Please reorganize this comment
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg commented on a change in pull request #5983: Save array's size instead of offset.
Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983#discussion_r646334458
##########
File path: be/test/olap/column_vector_test.cpp
##########
@@ -72,60 +72,70 @@ void test_read_write_scalar_column_vector(const TypeInfo* type_info, const uint8
}
template <FieldType item_type>
-void test_read_write_list_column_vector(const ArrayTypeInfo* list_type_info,
- segment_v2::ordinal_t* ordinals, // n + 1
- size_t list_size, const uint8_t* src_item_data,
- Collection* result) {
- DCHECK(list_size > 1);
+void test_read_write_array_column_vector(const ArrayTypeInfo* array_type_info, size_t array_size, Collection* result) {
+ DCHECK(array_size > 1);
using ItemType = typename TypeTraits<item_type>::CppType;
- ItemType* src_item = (ItemType*)src_item_data;
size_t ITEM_TYPE_SIZE = sizeof(ItemType);
- TabletColumn list_column(OLAP_FIELD_AGGREGATION_NONE, OLAP_FIELD_TYPE_ARRAY);
+ TabletColumn array_column(OLAP_FIELD_AGGREGATION_NONE, OLAP_FIELD_TYPE_ARRAY);
TabletColumn item_column(OLAP_FIELD_AGGREGATION_NONE, item_type, true, 0, 0);
- list_column.add_sub_column(item_column);
- Field* field = FieldFactory::create(list_column);
+ array_column.add_sub_column(item_column);
+ Field* field = FieldFactory::create(array_column);
- size_t list_init_size = list_size / 2;
+ size_t array_init_size = array_size / 2;
std::unique_ptr<ColumnVectorBatch> cvb;
- ASSERT_TRUE(ColumnVectorBatch::create(list_init_size, true, list_type_info, field, &cvb).ok());
+ ASSERT_TRUE(ColumnVectorBatch::create(array_init_size, true, array_type_info, field, &cvb).ok());
- ArrayColumnVectorBatch* list_cvb = reinterpret_cast<ArrayColumnVectorBatch*>(cvb.get());
- ColumnVectorBatch* item_cvb = list_cvb->elements();
+ auto* array_cvb = reinterpret_cast<ArrayColumnVectorBatch*>(cvb.get());
+ ColumnVectorBatch* item_cvb = array_cvb->elements();
+ ColumnVectorBatch* offset_cvb = array_cvb->offsets();
// first write
- list_cvb->put_item_ordinal(ordinals, 0, list_init_size + 1);
- list_cvb->set_null_bits(0, list_init_size, false);
- size_t first_write_item = ordinals[list_init_size] - ordinals[0];
+ for (size_t i = 0; i < array_init_size; ++i) {
+ memcpy(offset_cvb->mutable_cell_ptr(1 + i), &(result[i].length), sizeof(segment_v2::ordinal_t));
+ std::cout << array_cvb->item_offset(1 + i) << " ++++++++" << 1 + i << std::endl;
Review comment:
remove this
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg commented on a change in pull request #5983: Save array's size instead of offset.
Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5983:
URL: https://github.com/apache/incubator-doris/pull/5983#discussion_r648171433
##########
File path: be/src/olap/column_vector.cpp
##########
@@ -127,8 +127,22 @@ Status ColumnVectorBatch::create(size_t init_capacity, bool is_nullable, const T
return Status::NotSupported(
"When create ArrayColumnVectorBatch, `Field` is indispensable");
}
+
+ std::unique_ptr<ColumnVectorBatch> elements;
+ auto array_type_info = reinterpret_cast<const ArrayTypeInfo*>(type_info);
+ RETURN_IF_ERROR(ColumnVectorBatch::create(init_capacity * 2, field->get_sub_field(0)->is_nullable(),
+ array_type_info->item_type_info(), field->get_sub_field(0), &elements));
+
+ std::unique_ptr<ColumnVectorBatch> offsets;
+ TypeInfo* bigint_type_info = get_scalar_type_info(FieldType::OLAP_FIELD_TYPE_UNSIGNED_BIGINT);
+ RETURN_IF_ERROR(ColumnVectorBatch::create(init_capacity + 1, false,
+ bigint_type_info, nullptr, &offsets));
+
std::unique_ptr<ColumnVectorBatch> local(
Review comment:
what does local mean
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org