throw ExistedException if create schemas with same name and properties different #1009

houzhizhen · 2020-05-28T10:39:24Z

implement #870

javeme · 2020-05-28T21:05:14Z

hugegraph-test/src/main/java/com/baidu/hugegraph/core/VertexLabelCoreTest.java

+        super.initPropertyKeys();
+        SchemaManager schema = graph().schema();
+        schema.vertexLabel("person").properties("name", "age", "city")
+                .primaryKeys("name").create();


align with ".vertexLabel"

javeme · 2020-05-28T21:06:03Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/VertexLabelBuilder.java

+        return true;
+
+
+


remove empty lines

javeme · 2020-05-28T21:06:23Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/VertexLabelBuilder.java

+
+//        if (this.idStrategy != existedVertexLabel.idStrategy()) {
+//            return false;
+//        }


remove unused code

javeme · 2020-05-28T21:07:01Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/VertexLabelBuilder.java

+        }
+        for (String propertyName : this.properties) {
+            PropertyKey propertyKey = this.transaction.getPropertyKey(propertyName);
+            if (! existedProperties.contains(propertyKey.id())) {


remove space in "! existedProperties"

javeme · 2020-05-28T21:17:33Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/EdgeLabelBuilder.java

@@ -141,6 +141,72 @@ public EdgeLabel create() {
        });
    }

+    private boolean hasSameProperties(EdgeLabel existedEdgeLabel) {


prefer to move this method to class EdgeLabel, and process parent fields by each super class like SchemaLabel/SchemaElement

If move hasSameProperties to EdgeLabel, and parent class, it will reduce some duplicate check.
SchemaLabel has the following fields used by VertexLabel and EdgeLabel.

private final Set<Id> properties; private final Set<Id> nullableKeys; private final Set<Id> indexLabels; private boolean enableLabelIndex;

But the parameter should be Builder, and in should has proper method to retrieve correspond information. There is not common parent Builder with these information.

javeme · 2020-05-28T21:18:18Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/IndexLabelBuilder.java

+        }
+
+        SchemaLabel schemaLabel = this.loadElement();
+        if (! schemaLabel.id().equals(existedIndexLabel.baseValue())) {


javeme · 2020-05-28T21:18:28Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/IndexLabelBuilder.java

+    private boolean hasSameProperties(IndexLabel existedIndexLabel) {
+        // baseType is null, it means HugeType.SYS_SCHEMA
+        if (this.baseType == null && existedIndexLabel.baseType() != HugeType.SYS_SCHEMA
+        || this.baseType != existedIndexLabel.baseType()) {


javeme · 2020-05-28T21:18:40Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/IndexLabelBuilder.java

+        }
+
+        if (this.indexType == null && existedIndexLabel.indexType() != IndexType.SECONDARY
+                || this.indexType != existedIndexLabel.indexType()) {


…dgeLabel. If the two or more create statements as the code follows are identical, the system will only create one schema quietly.

codecov · 2020-05-29T03:46:19Z

Codecov Report

Merging #1009 into master will decrease coverage by 0.03%.
The diff coverage is 42.57%.

@@             Coverage Diff              @@
##             master    #1009      +/-   ##
============================================
- Coverage     69.35%   69.31%   -0.04%     
- Complexity     5275     5304      +29     
============================================
  Files           325      325              
  Lines         25870    25967      +97     
  Branches       3647     3685      +38     
============================================
+ Hits          17941    17998      +57     
- Misses         6207     6227      +20     
- Partials       1722     1742      +20

Impacted Files	Coverage Δ	Complexity Δ
...idu/hugegraph/schema/builder/EdgeLabelBuilder.java	`81.88% <40.00%> (-5.73%)`	`75.00 <4.00> (+7.00)`	⬇️
...du/hugegraph/schema/builder/IndexLabelBuilder.java	`85.80% <42.85%> (-1.04%)`	`114.00 <3.00> (+7.00)`	⬇️
...u/hugegraph/schema/builder/VertexLabelBuilder.java	`81.85% <43.75%> (-5.71%)`	`79.00 <4.00> (+5.00)`	⬇️
...u/hugegraph/schema/builder/PropertyKeyBuilder.java	`89.60% <50.00%> (+0.61%)`	`49.00 <2.00> (+5.00)`
...om/baidu/hugegraph/api/filter/ExceptionFilter.java	`66.66% <0.00%> (-4.31%)`	`0.00% <0.00%> (ø%)`
...du/hugegraph/backend/tx/GraphIndexTransaction.java	`79.58% <0.00%> (-0.25%)`	`189.00% <0.00%> (-1.00%)`
...hugegraph/backend/serializer/BinarySerializer.java	`85.43% <0.00%> (+1.29%)`	`119.00% <0.00%> (+4.00%)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb8fa1c...eab1896. Read the comment docs.

Linary · 2020-06-01T02:04:05Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/EdgeLabelBuilder.java

+                return false;
+            }
+        } else { // means false
+            if (existedEdgeLabel.enableLabelIndex() == false) {


here this. enableLabelIndex = false, seems only existedEdgeLabel.enableLabelIndex() == true should return false.

Good job, this is a bug.

Linary · 2020-06-01T02:05:00Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/EdgeLabelBuilder.java

+    private boolean hasSameProperties(EdgeLabel existedEdgeLabel) {
+        HugeGraph graph = this.graph();
+        Id sourceId = graph.vertexLabel(this.sourceLabel).id();
+        if (! existedEdgeLabel.sourceLabel().equals(sourceId)) {


There should no space between ! and existedEdgeLabel.sourceLabel().equals(sourceId) in our code style

…dex is true, it should return false

zhoney · 2020-06-01T05:06:36Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/EdgeLabelBuilder.java

+        }
+
+        Id targetId = graph.vertexLabel(this.targetLabel).id();
+


delete empty line

zhoney · 2020-06-01T05:07:18Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/IndexLabelBuilder.java

+
+    /**
+     * Check whether this has same properties with existedIndexLabel.
+     * Only baseType,baseValue,indexType, indexFields are checked.


more or less empty

Only baseType, baseValue, indexType, indexFields are checked.

zhoney · 2020-06-01T05:09:17Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/PropertyKeyBuilder.java

+
+    /**
+     * Check whether this has same properties with propertyKey.
+     * Only dataType, cardinality, aggregateType  are checked.


zhoney · 2020-06-01T05:09:25Z

hugegraph-core/src/main/java/com/baidu/hugegraph/schema/builder/VertexLabelBuilder.java

@@ -137,6 +137,62 @@ public VertexLabel create() {
        });
    }

+    /**
+     * Check whether this has same properties with existedVertexLabel.
+     * Only  properties,  primaryKeys, nullableKeys, enableLabelIndex  are checked.


javeme reviewed May 28, 2020

View reviewed changes

houzhizhen added 2 commits May 29, 2020 10:47

Check for 4 types of schema: propertyKey,vertexLabel,indexLabel,and e…

65c2f35

…dgeLabel. If the two or more create statements as the code follows are identical, the system will only create one schema quietly.

tiny improvement

97bc6bb

houzhizhen force-pushed the schemaDuplicateCheck branch from 82dc4e0 to 97bc6bb Compare May 29, 2020 03:30

javeme previously approved these changes May 29, 2020

View reviewed changes

houzhizhen changed the title ~~schemaDuplicateCheck~~ throw ExistedException if create schemas with same name and properties different May 30, 2020

Linary requested changes Jun 1, 2020

View reviewed changes

bug fix: current enableLabelIndex is false, and existed enableLabelIn…

a114c11

…dex is true, it should return false

houzhizhen dismissed javeme’s stale review via a114c11 June 1, 2020 02:59

zhoney requested changes Jun 1, 2020

View reviewed changes

fix space problem in methods comments

eab1896

zhoney approved these changes Jun 1, 2020

View reviewed changes

houzhizhen requested a review from Linary June 1, 2020 06:27

Linary approved these changes Jun 1, 2020

View reviewed changes

javeme merged commit b0202d7 into master Jun 1, 2020

javeme deleted the schemaDuplicateCheck branch June 1, 2020 13:19

Linary mentioned this pull request Jul 13, 2020

Fix bug: range indexlabel check not correct #1090

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

throw ExistedException if create schemas with same name and properties different #1009

throw ExistedException if create schemas with same name and properties different #1009

houzhizhen commented May 28, 2020 •

edited by javeme

javeme May 28, 2020

javeme May 28, 2020

javeme May 28, 2020

javeme May 28, 2020

javeme May 28, 2020

houzhizhen May 29, 2020

javeme May 28, 2020

javeme May 28, 2020

javeme May 28, 2020

codecov bot commented May 29, 2020 •

edited

Linary Jun 1, 2020

houzhizhen Jun 1, 2020

Linary Jun 1, 2020

zhoney Jun 1, 2020

zhoney Jun 1, 2020

zhoney Jun 1, 2020

zhoney Jun 1, 2020

throw ExistedException if create schemas with same name and properties different #1009

throw ExistedException if create schemas with same name and properties different #1009

Conversation

houzhizhen commented May 28, 2020 • edited by javeme

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 29, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houzhizhen commented May 28, 2020 •

edited by javeme

codecov bot commented May 29, 2020 •

edited