Skip to content

[Improve](Tablet Schema) Use deterministic way to serialize protobuf … #30906

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.

Sign up for GitHub

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

Merged
eldenmoon merged 1 commit into from
Feb 18, 2024
Merged

[Improve](Tablet Schema) Use deterministic way to serialize protobuf … #30906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion be/src/olap/rowset/rowset_meta.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ void RowsetMeta::set_tablet_schema(const TabletSchemaPB& tablet_schema) {
if (_handle) {
TabletSchemaCache::instance()->release(_handle);
}
auto pair = TabletSchemaCache::instance()->insert(tablet_schema.SerializeAsString());
auto pair = TabletSchemaCache::instance()->insert(
TabletSchema::deterministic_string_serialize(tablet_schema));
_handle = pair.first;
_schema = pair.second;
}
Expand Down
17 changes: 15 additions & 2 deletions be/src/olap/tablet_schema.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,17 @@
#include <gen_cpp/Descriptors_types.h>
#include <gen_cpp/olap_file.pb.h>
#include <glog/logging.h>
#include <google/protobuf/io/coded_stream.h>
#include <google/protobuf/io/zero_copy_stream.h>
#include <google/protobuf/io/zero_copy_stream_impl_lite.h>

#include <algorithm>
#include <cctype>
// IWYU pragma: no_include <bits/std_abs.h>
#include <cmath> // IWYU pragma: keep
#include <memory>
#include <ostream>
#include <vector>

#include "common/compiler_util.h" // IWYU pragma: keep
#include "common/consts.h"
Expand Down Expand Up @@ -770,7 +774,7 @@ void TabletIndex::to_schema_pb(TabletIndexPB* index) const {
index->add_col_unique_id(col_unique_id);
}
index->set_index_type(_index_type);
for (auto& kv : _properties) {
for (const auto& kv : _properties) {
(*index->mutable_properties())[kv.first] = kv.second;
}
index->set_index_suffix_name(_escaped_index_suffix_path);
Expand Down Expand Up @@ -929,7 +933,7 @@ void TabletSchema::copy_from(const TabletSchema& tablet_schema) {
std::string TabletSchema::to_key() const {
TabletSchemaPB pb;
to_schema_pb(&pb);
return pb.SerializeAsString();
return TabletSchema::deterministic_string_serialize(pb);
}

void TabletSchema::build_current_tablet_schema(int64_t index_id, int32_t version,
Expand Down Expand Up @@ -1382,4 +1386,13 @@ bool operator!=(const TabletSchema& a, const TabletSchema& b) {
return !(a == b);
}

std::string TabletSchema::deterministic_string_serialize(const TabletSchemaPB& schema_pb) {
std::string output;
google::protobuf::io::StringOutputStream string_output_stream(&output);
google::protobuf::io::CodedOutputStream output_stream(&string_output_stream);
output_stream.SetSerializationDeterministic(true);
Copy link
Contributor

@yiguolei yiguolei Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the deterministic serialize will take affect on map value?
void CodedOutputStream::SetSerializationDeterministic(
bool value)
Indicate to the serializer whether the user wants derministic serialization.

The default when this is not called comes from the global default, controlled by SetDefaultSerializationDeterministic.

What deterministic serialization means is entirely up to the driver of the serialization process (i.e. the caller of methods like WriteVarint32). In the case of serializing a proto buffer message using one of the methods of MessageLite, this means that for a given binary equal messages will always be serialized to the same bytes. This implies:

Repeated serialization of a message will return the same bytes.
Different processes running the same binary (including on different
machines) will serialize equal messages to the same bytes.

Note that this is not canonical across languages. It is also unstable across different builds with intervening message definition changes, due to unknown fields. Users who need canonical serialization (e.g. persistent storage in a canonical form, fingerprinting) should define their own canonicalization specification and implement the serializer using reflection APIs rather than relying on this API.

Copy link
Member Author

@eldenmoon eldenmoon Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes , map serialization is none deterministic by default, and SetSerializationDeterministic to true will make it deterministic.From the document above This means that for a given binary equal messages will always be serialized to the same bytes.

I've also tested and it's deterministic as expected

schema_pb.SerializeToCodedStream(&output_stream);
return output;
}

} // namespace doris
11 changes: 7 additions & 4 deletions be/src/olap/tablet_schema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ class TabletSchema {
// void create_from_pb(const TabletSchemaPB& schema, TabletSchema* tablet_schema).
TabletSchema() = default;
void init_from_pb(const TabletSchemaPB& schema, bool ignore_extracted_columns = false);
// Notice: Use deterministic way to serialize protobuf,
// since serialize Map in protobuf may could lead to un-deterministic by default
static std::string deterministic_string_serialize(const TabletSchemaPB& schema_pb);
void to_schema_pb(TabletSchemaPB* tablet_meta_pb) const;
void append_column(TabletColumn column, ColumnType col_type = ColumnType::NORMAL);
void append_index(TabletIndex index);
Expand Down Expand Up @@ -363,17 +366,17 @@ class TabletSchema {
// Dump [(name, type, is_nullable), ...]
string dump_structure() const {
string str = "[";
for (auto p : _field_name_to_index) {
for (auto p : _cols) {
Copy link
Contributor

@yiguolei yiguolei Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change this code?

Copy link
Member Author

@eldenmoon eldenmoon Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_field_name_to_index is an unorded_map. change it to make output ordered

if (str.size() > 1) {
str += ", ";
}
str += "(";
str += p.first;
str += p.name();
str += ", ";
str += TabletColumn::get_string_by_field_type(_cols[p.second].type());
str += TabletColumn::get_string_by_field_type(p.type());
str += ", ";
str += "is_nullable:";
str += (_cols[p.second].is_nullable() ? "true" : "false");
str += (p.is_nullable() ? "true" : "false");
str += ")";
}
str += "]";
Expand Down