-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v2 segment support string encode(#1766) #1816
Conversation
keep dict page handle in FileColumnIterator;
for (int j = 0; j < 4; ++j) { | ||
auto cell = row.cell(j); | ||
cell.set_not_null(); | ||
set_column_value_by_type(tablet_schema->_cols[j]._type, i * 10 + j, (char*)cell.mutable_cell_ptr(), tablet_schema->_cols[j]._length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, how can you access private field _cols
and _type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
me too;not FRIEND_TEST,not "#define private public"
} else if (_type_info->type() == OLAP_FIELD_TYPE_CHAR) { | ||
Slice *min_value = (Slice *)_zone_map.min_value; | ||
min_value->data = _max_char_value; | ||
min_value->size = OLAP_CHAR_MAX_LENGTH; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why set char type's max length to 255? I think it should be equal to OLAP_STRING_MAX_LENGTH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fe defines max char and varchar length,see ScalarType;this is a temporary define,later I will use columns real length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, for char type, it is a bit complex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM generally. One thing I'm not sure is the change to varchar's set_to_max since it's been used by segment v1's zonemap. Would it break backward compatibility? @kangpinghuang Could you verify it?
be/src/olap/types.h
Outdated
@@ -587,13 +604,19 @@ struct FieldTypeTraits<OLAP_FIELD_TYPE_VARCHAR> : public FieldTypeTraits<OLAP_FI | |||
} | |||
static void set_to_max(void* buf) { | |||
auto slice = reinterpret_cast<Slice*>(buf); | |||
slice->size = 1; | |||
memset(slice->data, 0xFF, 1); | |||
memset(slice->data, 0xFF, slice->size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should first reset slice->size to OLAP_STRING_MAX_LENGTH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
2. interpret not cast 3.remove useless var 4. set_to_min/max consist with master
I have check set_to_max in olap, I think original code has bug and this pr's code has fix it.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
current commit is old version:in varchar's set_to_max,slice size set to 1 |
Core dump stack
core dump happends in wrapper_field.cpp
before that in column_data_writer.cpp
in line:68,we get a char[64] I prepare to solve it with a new pr which allocate ColumnZoneMapBuilder max/min value with real column length |
@imay please review again
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
#1766
major change
0919 commit major change
3.rollback BinaryDictPage
0926 17 commit major change
set char/varchar column_zone_map'max value size to 0
0929 10 commit major change