INDEX: index key and data serialization API #834
Conversation
| @@ -78,6 +79,31 @@ index_key_t index_builder_t::make_key(gaia_id_t index_id, gaia_type_t type_id, c | |||
| return k; | |||
| } | |||
|
|
|||
| void index_builder_t::serialize_key(const index_key_t& key, data_write_buffer_t& buffer) | |||
| { | |||
| for (const auto& k : key.values()) | |||
There was a problem hiding this comment.
Unclear what auto is at this point, maybe vector<data_holder_t> won't hurt.
| { | ||
| ASSERT_PRECONDITION(index_id != c_invalid_gaia_id, "Invalid gaia id."); | ||
|
|
||
| index_key_t k; |
There was a problem hiding this comment.
nit: key or index_key.
|
|
||
| // These classes provide serialization/deserialization interfaces over flatbuffers type [byte]. | ||
|
|
||
| class data_write_buffer_t |
There was a problem hiding this comment.
Does the word buffer refers to Faltbuffers or to the fact that the writing is buffered? A class comment would help.
| @@ -91,6 +94,8 @@ struct data_holder_t | |||
| int compare(const data_holder_t& other) const; | |||
| data_hash_t hash() const; | |||
|
|
|||
| void serialize(data_write_buffer_t& buffer) const; | |||
|
|
|||
| // Convenience implicit conversions to native types. | |||
There was a problem hiding this comment.
These seems more like explicit conversions...
There was a problem hiding this comment.
We should update the comment to mentionexplicit.
| @@ -68,7 +69,7 @@ index_key_t index_builder_t::make_key(gaia_id_t index_id, gaia_type_t type_id, c | |||
| auto schema = table.binary_schema(); | |||
| auto index_view = index_view_t(id_to_ptr(index_id)); | |||
|
|
|||
| auto& fields = *(index_view.fields()); | |||
| const auto& fields = *(index_view.fields()); | |||
| for (auto field_id : fields) | |||
There was a problem hiding this comment.
This can be gaia_id_t.. (same below)
| DEPENDS ${PROJECT_SOURCE_DIR}/tests/test_serialization.fbs | ||
| DEPENDS ${CMAKE_BINARY_DIR}/flatbuffers/flatc | ||
| VERBATIM) | ||
|
|
There was a problem hiding this comment.
Extra empty line.
There was a problem hiding this comment.
Note that it still shows - did you miss merging the change?
| using serialization_write_buffer_t = std::vector<serialization_byte_t>; | ||
| using serialization_output_t = flatbuffers::Offset<flatbuffers::Vector<serialization_byte_t>>; | ||
|
|
||
| // These classes provide serialization/deserialization interfaces over the flatbuffers type [byte] |
There was a problem hiding this comment.
We should be consistent in comments: we have flatbuffers, FlatBuffer and Flatbuffer. I think we should stick with flatbuffers, which is also the folder name.
| case common::data_type_t::e_string: | ||
| return "e_string"; | ||
| default: | ||
| return "unknown"; |
There was a problem hiding this comment.
We can use ASSERT_UNREACHABLE here.
| case common::data_type_t::e_string: | ||
| return reflection::String; | ||
| default: | ||
| throw data_type_not_handled_t(type); |
There was a problem hiding this comment.
I think we can also use ASSERT_UNREACHABLE here. The FDW can catch all exceptions and print them.
LaurentiuCristofor
dismissed
their stale review
To remove the change request, because a simple comment does not do that.
No description provided.