Implement EdgeType + Property indices #1964
Conversation
|
|
|
||
| EdgeIndexQuery *Clone(AstStorage *storage) const override { | ||
| EdgeIndexQuery *object = storage->Create<EdgeIndexQuery>(); | ||
| object->action_ = action_; | ||
| object->edge_type_ = storage->GetEdgeTypeIx(edge_type_.name); | ||
| object->properties_.resize(properties_.size()); | ||
| for (auto i = 0; i < object->properties_.size(); ++i) { | ||
| object->properties_[i] = storage->GetPropertyIx(properties_[i].name); |
There was a problem hiding this comment.
Just curious. Why can't we just write object->properties_[i] = properties_[i]; ?
| @@ -15,6 +15,7 @@ | |||
| #include <cstdint> | |||
| #include <set> | |||
|
|
|||
| #include "storage/v2/edge.hpp" | |||
There was a problem hiding this comment.
Do we need this header file here?
| @@ -391,6 +399,34 @@ PullPlanDump::PullChunk PullPlanDump::CreateEdgeTypeIndicesPullChunk() { | |||
| }; | |||
| } | |||
|
|
|||
| PullPlanDump::PullChunk PullPlanDump::CreateEdgeTypePropertyIndicesPullChunk() { | |||
| // Dump all label indices | |||
There was a problem hiding this comment.
Please fix comment. We dump label property indices.
| } | ||
| auto properties_stringified = utils::Join(properties_string, ", "); | ||
|
|
||
| if (properties.size() > 1) { |
There was a problem hiding this comment.
It's better to have this validation before we allocate memory in vectors for properties and properties_string.
| @@ -64,8 +64,10 @@ enum class Marker : uint8_t { | |||
| DELTA_LABEL_PROPERTY_INDEX_STATS_CLEAR = 0x64, | |||
| DELTA_EDGE_TYPE_INDEX_CREATE = 0x65, | |||
| DELTA_EDGE_TYPE_INDEX_DROP = 0x66, | |||
| DELTA_TEXT_INDEX_CREATE = 0x67, | |||
| DELTA_TEXT_INDEX_DROP = 0x68, | |||
| DELTA_EDGE_TYPE_PROPERTY_INDEX_CREATE = 0x67, | |||
There was a problem hiding this comment.
Are we sure that we want to change marker for Text index creation?
I'm afraid that customers who uses Text Index will be surprised during restore. I would suggest to not change existing markers of durability
| if (!edge_type) throw RecoveryFailure("Invalid WAL data!"); | ||
| delta.operation_edge_type_property.edge_type = std::move(*edge_type); | ||
| auto property = decoder->ReadString(); | ||
| if (!property) throw RecoveryFailure("Invalid WAL data!"); |
There was a problem hiding this comment.
Should we throw more accurate exceptions? I mean to not throw Invalid WAL data! but Invalid WAL data! Property for edge type property index is corrupted.
| @@ -165,6 +166,8 @@ class OpChecker : public BaseOpChecker { | |||
| public: | |||
| void CheckOp(LogicalOperator &op, const SymbolTable &symbol_table) override { | |||
| auto *expected_op = dynamic_cast<TOp *>(&op); | |||
| auto debug_op_typeinfo = op.GetTypeInfo().name; | |||
| auto debug_expected_typeinfo = TOp::kType.name; | |||
There was a problem hiding this comment.
Do we still need these 2 variables? If yes, why we don't use them?
| } | ||
|
|
||
| // // NOLINTNEXTLINE(hicpp-special-member-functions) | ||
| // TEST_P(DurabilityTest, EdgeTypePropertyIndexRecoveredWithoutEdgeTypeIndices) { |
There was a problem hiding this comment.
Do we plan to uncomment this?
|
|
||
| EXPECT_THAT(this->GetIds(acc->Edges(this->edge_type_id1, this->edge_prop_id1, View::NEW), View::NEW), | ||
| UnorderedElementsAre(0, 1, 2, 3, 4)); | ||
| } |
There was a problem hiding this comment.
I think we also need to add changing property value on indexed edge type + property to test UpdateOnModifiedEdge
Source issue -> #662
Description
Implementation of EdgeType + Property indices.
Please delete either the [master < EPIC] or [master < Task] part, depending on what are your needs.
[master < Epic] PR
[master < Task] PR
CI Testing Labels
Please select the appropriate CI test labels (CI -build=build-name -test=test-suite)
Documentation checklist