Support batch updating elements' property by multiple strategy #493

imbajin · 2019-04-30T08:08:08Z

Hard code to get elementId now
Edge's Direction is always OUT now
Other TODOs need finish later (add more unit-test also)

codecov · 2019-04-30T08:18:44Z

Codecov Report

Merging #493 into master will not change coverage.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #493   +/-   ##
=========================================
  Coverage     71.41%   71.41%           
  Complexity     3528     3528           
=========================================
  Files           216      216           
  Lines         16657    16657           
  Branches       2379     2379           
=========================================
  Hits          11895    11895           
  Misses         3512     3512           
  Partials       1250     1250

Impacted Files	Coverage Δ	Complexity Δ
...hugegraph/backend/serializer/BinarySerializer.java	`80.09% <ø> (ø)`	`104 <0> (ø)`	⬇️
...h/backend/store/rocksdbsst/RocksDBSstSessions.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️

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 ffb09f1...454e123. Read the comment docs.

javeme · 2019-05-06T08:38:35Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+    }
+
+    public static UpdateStrategy valueOf(Object strategy) {
+        E.checkState(strategy instanceof String, "UpdateStrategy must be " +


wrap line after ","

javeme · 2019-05-06T08:54:15Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+     * TODO: Adapter for createIfNotExist
+     * Batch update steps are same like vertices
+     **/
+    @POST


please use PUT and let jsonEdges = {edges: [...]}

javeme · 2019-05-06T08:55:30Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+    public String update(@Context HugeConfig config,
+                         @Context GraphManager manager,
+                         @PathParam("graph") String graph,
+                         @QueryParam("updateStrategies")


move all QueryParams to json body

javeme · 2019-05-06T09:22:51Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+                String key = kv.getKey();
+                if (oldElement.properties.get(key) != null) {
+                    Object value = UpdateStrategy.checkAndUpdateProperty(
+                            oldElement.properties.get(key),


prefer to align with UpdateStrategy

javeme · 2019-05-06T09:23:57Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+        }
+    }
+
+    protected void operateProperties(JsonElement jsonElement, boolean append,


set static and rename to updateProperties()

javeme · 2019-05-06T09:38:32Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+public enum UpdateStrategy {
+
+    //Only number support accumlate
+    ACCUMULATE {


SUM is more readable

javeme · 2019-05-06T09:40:38Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+        @Override
+        void checkPropertyType(Object oldProperty, Object newProperty) {
+            E.checkState((oldProperty instanceof Date ||
+                          oldProperty instanceof Long) &&


oldProperty instanceof Number?

have thought about it before, but the timestamp with milliseconds can only be Long, the unattached is also close to the upper limit of Integer, and it should only allow Integer or Long? The range of Number seems too wide

Maybe we can use method compareNumber() from hugegraph-common directly: com.baidu.hugegraph.util.NumericUtil.compareNumber(Object first, Number second)
and call com.baidu.hugegraph.util.NumericUtil.convertToNumber(Object value) to convert an object to a number.

javeme · 2019-05-06T09:40:46Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+            E.checkState((oldProperty instanceof Date ||
+                          oldProperty instanceof Long) &&
+                         (newProperty instanceof Date ||
+                          newProperty instanceof Long),


javeme · 2019-05-06T09:41:43Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+        }
+    },
+
+    // TODO: Merge from "action", how to handle List? (have same elements)


ELIMINATE all the same elements

javeme · 2019-05-06T09:54:23Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+                if (oldElement.property(key).isPresent()) {
+                    Object value = UpdateStrategy.checkAndUpdateProperty(
+                            oldElement.property(key).value(),
+                            element.properties.get(key),


let new property value be converted by property-key like this pkey.convValue(value, false)

how to get property-key: graph.propertyKey(name)

let new property value be converted by property-key like this pkey.convValue(value, false)

how to get property-key: graph.propertyKey(name)

is it like value = g.propertyKey(key).convValue(value, false) ? Wonder why need to convert pkValue again? or just for safety?

PS: disappear for some days, and back now~ (will fix them soon)

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

Linary · 2019-05-07T03:07:16Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+     * TODO: Should support AUTOMATIC ElementID ?
+     * TODO: How to get JsonEdge's fields ?
+     */
+    protected String getElementId(HugeGraph g, String labelId,


seems can split into two methods directly: getVertexId(g, label, JsonVertex) and getEdgeId(g, label, JsonEdge).
can avoid the method too long and reduce the number of parameters.
If need, move getVertexId into VertexAPI, getEdgeId into EdgeAPI

Linary · 2019-05-07T03:09:33Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+
+    /**
+     * Get vertex/edge ID from JsonVerex/JsonEdge
+     * TODO: Should support AUTOMATIC ElementID ?


AUTOMATIC ElementID -> AUTOMATIC Id strategy, similar in other places

Linary · 2019-05-07T03:12:37Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+                                      JsonElement newElement,
+                                      Map<String, UpdateStrategy> strategy) {
+        if (oldElement != null && newElement != null) {
+            strategy.entrySet().forEach(kv -> {


This logic-critical code uses a normal for loop instead of forEach

Linary · 2019-05-07T03:31:21Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+        checkBatchSize(config, jsonEdges);
+
+        Map<String, JsonEdge> maps = new HashMap<>(jsonEdges.size());
+        Map<String, Object> strategies = parseProperties(updateStrategies);


Define a class wrap all QueryParams and jsonEdges, then deserialize UpdateStrategies will be easy. you can refer
CustomizedPathsAPI.PathRequest

javeme · 2019-05-16T06:00:19Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+            Iterator<Edge> oldEdges = g.edges(ids.toArray());
+            oldEdges.forEachRemaining(oldEdge -> {
+                updateExistElement(oldEdge,
+                                   maps.get(oldEdge.id().toString()),


can we change Map<String, JsonEdge> to Map<Id, JsonEdge>?

first I want to use a common method called getElementId to get EdgeId & VertexId together, so I use String for both
now the methods are separated. maybe use Map<Object, JsonVertex> & Map<String, JsonEdge> is more simple because JsonVertex & JsonEdge 's id property's type isObject& String.
if use Id type for them, both of them need to convert again, but seems unnecessary?

both Edge Id and Vertex Id are instance of Id, EdgeId is a subclass of id.

javeme · 2019-05-16T06:02:29Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+        @Override
+        public String toString() {
+            return String.format("EdgeRequest{jsonEdges=%s,updateStrategies" +
+                                 "=%s,check_vertex=%s,createIfNotExist=%s}",


check_vertex => checkVertex (in java code)
and prefer to move "=%s," to the previous line

javeme · 2019-05-16T06:05:58Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+                          oldProperty instanceof Number) &&
+                         (newProperty instanceof Date ||
+                          newProperty instanceof Number),
+                         formatError(oldProperty, newProperty, "Date or " +


wrap line after ","

javeme · 2019-05-16T06:11:13Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+                                      JsonElement newElement,
+                                      Map<String, UpdateStrategy> strategy,
+                                      HugeGraph g) {
+        if (oldElement != null && newElement != null) {


E.checkArgment(oldElement != null && newElement != null, "...");

javeme · 2019-05-16T06:12:38Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+                                   oldElement.property(key).value(),
+                                   newElement.properties.get(key));
+                    newElement.properties.put(key, g.propertyKey(key).
+                                                   convValue(value,false));


missing a space
prefer to use value = g.propertyKey(key).convValue(value, false)

javeme · 2019-05-16T06:20:24Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+    protected void updateExistElement(Element oldElement,
+                                      JsonElement newElement,
+                                      Map<String, UpdateStrategy> strategy,
+                                      HugeGraph g) {


let "HugeGraph g" as first parameter

javeme · 2019-05-16T07:05:07Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+        for (Id pkId : pkIds) {
+            String propertyKey = g.propertyKey(pkId).name();
+            Object propertyValue = vertex.properties.get(propertyKey);
+            E.checkState(propertyValue != null, "The value of primary key " +


wrap line after ","

javeme · 2019-05-16T11:19:18Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+    SUM {
+        @Override
+        Object updatePropertyValue(Object oldProperty, Object newProperty) {
+            // TODO: How to accumulae universally (Byte, Long,...Double)


we can use bigdecimal: https://www.tutorialspoint.com/java/math/bigdecimal_add_augend.htm

javeme · 2019-05-16T11:23:33Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+            Iterator<Edge> oldEdges = g.edges(ids.toArray());
+            oldEdges.forEachRemaining(oldEdge -> {
+                updateExistElement(oldEdge,
+                                   maps.get(oldEdge.id().toString()),


both Edge Id and Vertex Id are instance of Id, EdgeId is a subclass of id.

javeme · 2019-05-17T03:01:58Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+
+    private static String formatError(Object oldProperty, Object newProperty,
+                                      String className) {
+        return String.format("Property must be" + className + "type, but got " +


"Property must be % type, ..."

javeme · 2019-05-17T03:04:03Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+                                    (result < 0 ? oldProperty : newProperty);
+    }
+
+    private static Set comparedSet(Object oldProperty, Object newProperty,


prefer rename to combineSet()

javeme · 2019-05-17T03:07:31Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+            });
+
+            // If return ids, the ids.size() maybe different with the origins'
+            return manager.serializer(g).


keep "." at the head of next line and align with ".serializer()"

javeme · 2019-05-19T11:40:48Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

-    @JsonIgnoreProperties(value = {"type"})
-    private static class JsonVertex implements Checkable {
+    //TODO: Should support AUTOMATIC Id strategy ?
+    private Object getVertexId(HugeGraph g, String labelId, JsonVertex vertex) {


If let getVertexId() return Id then it can be more simple and unified in the future.

javeme · 2019-05-19T11:48:45Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

@@ -350,25 +404,63 @@ public static Direction parseDirection(String direction) {
        }
    }

-    @JsonIgnoreProperties(value = {"type"})
-    private static class JsonEdge implements Checkable {
+    private String getEdgeId(HugeGraph g, Id labelId, JsonEdge newEdge) {


same as getVertexId()

javeme · 2019-05-19T11:49:37Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+                                   Directions.OUT,
+                                   g.edgeLabel(newEdge.label).id(), sortKeys,
+                                   HugeVertex.getIdValue(newEdge.target));
+        return edgeId.asString();


return edgeId itself

javeme · 2019-05-20T07:10:06Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+             // 1.Put all newEdges' properties into map (combine first)
+            req.jsonEdges.forEach(newEdge -> {
+                Id labelId = g.edgeLabel(newEdge.label).id();
+                String id = newEdge.id != null ? newEdge.id :


Id id = newEdge.id != null ? EdgeId.parse(newEdge.id)

javeme · 2019-05-20T07:12:21Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

@@ -71,4 +78,75 @@ public BatchAPI() {
            batchWriteThreads.decrementAndGet();
        }
    }
+
+    // TODO: Design more flexible for Element?


seems unused TODO

I was wondering if I could make it easier to merge JsonElement & Element by extends or implements a class, will del it

javeme · 2019-05-20T07:13:43Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+        }
+    }
+
+    // TODO: Combine multi update methond into one (Element & JsonElement)


remove TODO since we can't combine them

javeme · 2019-05-20T07:19:08Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+            req.jsonVertices.forEach(newVertex -> {
+                String labelId = g.vertexLabel(newVertex.label).id().asString();
+                Object id = newVertex.id != null ? newVertex.id :
+                            this.getVertexId(g, labelId, newVertex);


just let Id id = this.getVertexId(g, labelId, newVertex);

javeme · 2019-05-20T07:19:50Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+        }
+
+        String name = SplicingIdGenerator.concatValues(pkValues);
+        return SplicingIdGenerator.splicing(labelId, name);


Object idValue = newVertex.id != null ? newVertex.id : SplicingIdGenerator.splicing(labelId, name) return HugeVertex.getIdValue(idValue)

javeme · 2019-05-20T07:22:17Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+        // TODO: How to get Direction from JsonEdge easily? or any better way?
+        EdgeId edgeId = new EdgeId(HugeVertex.getIdValue(newEdge.source),
+                                   Directions.OUT,
+                                   g.edgeLabel(newEdge.label).id(), sortKeys,


use labelId instead

javeme · 2019-05-20T07:24:05Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+            req.jsonVertices.forEach(newVertex -> {
+                String labelId = g.vertexLabel(newVertex.label).id().asString();
+                Object id = newVertex.id != null ? newVertex.id :
+                            this.getVertexId(g, labelId, newVertex);


just let Id id = this.getVertexId(g, labelId, newVertex);

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

javeme · 2019-05-20T07:29:38Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+    }
+
+    // TODO: Combine multi update methond into one (Element & JsonElement)
+    protected void updateExistElement(HugeGraph graph,


keep consistent style HugeGraph g

Linary · 2019-05-22T02:11:04Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+            * - Consider primary-key & user-define ID mode first
+            * */
+            req.jsonVertices.forEach(newVertex -> {
+                String labelId = g.vertexLabel(newVertex.label).id().asString();


seems doesn't need to define labelId, and getVertexId(g, newVertex) is enough

Linary · 2019-05-22T02:14:09Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+        HugeGraph g = graph(manager, graph);
+        checkBatchSize(config, req.jsonVertices);
+        // Not support automatic Id strategy now (check once?)
+        E.checkArgument(g.vertexLabel(req.jsonVertices.get(0).label).


Need check all vertices id strategy

Linary · 2019-05-22T02:21:05Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+            req.jsonVertices.forEach(newVertex -> {
+                String labelId = g.vertexLabel(newVertex.label).id().asString();
+                Id id = this.getVertexId(g, labelId, newVertex);
+                this.updateExistElement(newVertex, maps.get(id),


Is the parameter reversed? I think it should be this.updateExistElement(maps.get(id), newVertex, req.updateStrategies)

Linary · 2019-05-22T02:28:05Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+    private Id getVertexId(HugeGraph g, String labelId, JsonVertex vertex) {
+        assert g.vertexLabel(labelId).idStrategy() != IdStrategy.AUTOMATIC;
+
+        List<Id> pkIds = g.vertexLabel(vertex.label).primaryKeys();


prefer using if-else according to the id strategy, like

if (CUSTOMIZED) { xxx } else { assert PRIMARY_KEY xxx }

Clear logic is better than code clean

Linary · 2019-05-22T02:33:11Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+            Iterator<Vertex> oldVertices = g.vertices(ids.toArray());
+            oldVertices.forEachRemaining(oldVertex -> {
+                updateExistElement(g, oldVertex,
+                                   maps.get(oldVertex.id()),


seems define a local var is more readable

JsonVertex newVertex = maps.get(oldVertex.id()); updateExistElement(g, oldVertex, newVertex, req.updateStrategies);

Linary · 2019-05-22T02:37:00Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+            });
+
+            // 2.Get all oldVertices and update with new vertices
+            List<Id> ids = new ArrayList<>(maps.size());


Object[] ids = maps.keySet().toArray();

Linary · 2019-05-22T02:38:31Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

                      "the maximum number is '%s'", max));
        }
    }

-    @JsonIgnoreProperties(value = {"type"})
-    private static class JsonVertex implements Checkable {
+    private Id getVertexId(HugeGraph g, String labelId, JsonVertex vertex) {


remove param labelId

Linary · 2019-05-22T02:41:18Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+
+    private static class VertexRequest {
+
+        @JsonProperty("jsonVertices")


RESTful API params use underlined style, EdgeAPI is an exception, in order to follow the specifications of tinkerpop
@JsonProperty("jsonVertices") -> @JsonProperty("vertices")
@JsonProperty("updateStrategies") -> @JsonProperty("update_strategies")
@JsonProperty("create") -> @JsonProperty("create_if_not_exist")

Linary · 2019-05-22T02:48:41Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

@@ -350,25 +404,61 @@ public static Direction parseDirection(String direction) {
        }
    }

-    @JsonIgnoreProperties(value = {"type"})
-    private static class JsonEdge implements Checkable {
+    private Id getEdgeId(HugeGraph g, Id labelId, JsonEdge newEdge) {


Most of the comments in VertexAPI also applied here.

javeme · 2019-05-24T03:08:14Z

Ugh, seems CI failed, please fix https://travis-ci.org/hugegraph/hugegraph/jobs/536220267

Linary

seems need add constructor for JsonVertex and JsonEdge

Linary · 2019-06-01T09:25:19Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

        checkBatchSize(config, req.jsonEdges);

+        HugeGraph g = graph(manager, graph);


It's a good change

Linary · 2019-06-03T02:52:47Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+        }
+    }
+
+    private class JsonVertex extends JsonElement {


private static class can pass test!!!

javeme · 2019-06-03T05:53:57Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

        Map<Id, JsonEdge> maps = new HashMap<>(req.jsonEdges.size());
        TriFunction<HugeGraph, Object, String, Vertex> getVertex =
                    req.checkVertex ? EdgeAPI::getVertex : EdgeAPI::newVertex;

        return this.commit(config, g, maps.size(), () -> {

-             // 1.Put all newEdges' properties into map (combine first)
+            // 1.Put all newEdges' properties into map (combine first)


remove empty line

javeme · 2019-06-03T05:56:15Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

-                Id id = newEdge.id != null ? EdgeId.parse(newEdge.id) :
-                        this.getEdgeId(g, labelId, newEdge);
-                this.updateExistElement(newEdge, maps.get(id),
+                Id newEdgeId = newEdge.id != null ? EdgeId.parse(newEdge.id) :


move "newEdge.id != null ? EdgeId.parse(newEdge.id)" into getEdgeId():

if (newEdge.id != null) { return EdgeId.parse(newEdge.id); }

and just let Id newEdgeId = getEdgeId(g, newEdge);

javeme · 2019-06-03T06:03:51Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+    protected void updateExistElement(HugeGraph g,
+                                      Element oldElement,
+                                      JsonElement newElement,
+                                      Map<String, UpdateStrategy> strategy) {


rename to strategies

javeme · 2019-06-03T06:04:26Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+
+    protected void updateExistElement(JsonElement oldElement,
+                                      JsonElement newElement,
+                                      Map<String, UpdateStrategy> strategy) {


rename to strategies

javeme · 2019-06-03T06:06:43Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+
+    protected static void updateProperties(JsonElement jsonElement,
+                                           boolean append,
+                                           HugeElement element) {


prefer to set HugeElement element as the first parameter

javeme · 2019-06-03T06:07:42Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+        checkBatchSize(config, req.jsonEdges);
+
+        HugeGraph g = graph(manager, graph);
+        Map<Id, JsonEdge> maps = new HashMap<>(req.jsonEdges.size());


map or edges

address the comment

javeme · 2019-06-03T06:09:42Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+        checkBatchSize(config, req.jsonVertices);
+
+        HugeGraph g = graph(manager, graph);
+        Map<Id, JsonVertex> maps = new HashMap<>(req.jsonVertices.size());


rename to vertices or map

javeme · 2019-06-03T08:37:39Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+
+        @Override
+        void checkPropertyType(Object oldProperty, Object newProperty) {
+            E.checkState(oldProperty instanceof Number &&


update to E.checkArgument(), same as other checks in UpdateStrategy

javeme · 2019-06-03T08:42:30Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+
+        @Override
+        void checkPropertyType(Object oldProperty, Object newProperty) {
+            BIGGER.checkPropertyType(oldProperty, newProperty);


it makes sense to call BIGGER.checkPropertyType

javeme · 2019-06-03T08:45:10Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+
+    private static String formatError(Object oldProperty, Object newProperty,
+                                      String className) {
+        return String.format("Property must be %s type, but got %s, %s",


change to Property type must be %s

javeme · 2019-06-03T11:52:49Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

            String key = kv.getKey();
            UpdateStrategy updateStrategy = kv.getValue();
-            if (oldElement.property(key).isPresent()) {
+            if (oldElement.property(key).isPresent() &&
+                newElement.properties.get(key) != null ) {


replace newElement.properties.get(key) != null to newElement.properties.containsKey(key)
and remove space before " )"

javeme · 2019-06-03T11:55:26Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+        checkBatchSize(config, req.jsonEdges);
+
+        HugeGraph g = graph(manager, graph);
+        Map<Id, JsonEdge> maps = new HashMap<>(req.jsonEdges.size());


address the comment

javeme · 2019-06-03T11:59:25Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+    APPEND {
+        @Override
+        Object updatePropertyValue(Object oldProperty, Object newProperty) {
+            return ((Collection) oldProperty).addAll((Collection) newProperty);


addAll() returns boolean value, seems should return oldProperty.

javeme · 2019-06-03T11:59:58Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+    ELIMINATE {
+        @Override
+        Object updatePropertyValue(Object oldProperty, Object newProperty) {
+            return ((Collection) oldProperty).removeAll


javeme · 2019-06-03T12:01:28Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+                                    (result < 0 ? oldProperty : newProperty);
+    }
+
+    private static Set combineSet(Object oldProperty, Object newProperty,


add template parameter like Set<?>

javeme · 2019-06-03T12:03:09Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+        }
+    },
+
+    // TODO: Unify "action" in Vertex/EdgeAPI later


unused TODO?

javeme · 2019-06-04T04:01:20Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+        }
+    }
+
+    private static class JsonVertex extends JsonElement {


override equals() & hashCode() method.

equals(JsonEdge other): return this.id.equals(other.id) hashCode(): return this.id.hashCode()

Note: make sure that update JsonVertex.id in getVertexId() method.

The same as JsonEdge class, maybe it's better to move id field to JsonElement class.

Linary · 2019-06-04T12:02:53Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

@@ -125,7 +125,7 @@ protected void updateExistElement(HugeGraph g,
            String key = kv.getKey();
            UpdateStrategy updateStrategy = kv.getValue();
            if (oldElement.property(key).isPresent() &&
-                newElement.properties.get(key) != null ) {


remember to modify these:

hugegraph-api pom.xml <Implementation-Version>0.39.0.0</Implementation-Version> to <Implementation-Version>0.40.0.0</Implementation-Version>

hugegraph-api ApiVersion.class 0.39 to 0.40, and add an item under [0.39] Issue-522: Add profile RESTful API, write something like [0.40] Issue-493: Support batch updating elements' property by multiple strategy

1. Hard code to get elementId now 2. Edge's Direction is always OUT now 3. Other TODOs need finish later (add more unit-test also) 4. Move params to Vertex/EdgeRequest

The type of EdgeId is still String because of complexity of conversion

Tests show that @NotNull can't intercept null-objcet successfully.

javeme · 2019-06-10T03:33:20Z

hugegraph-api/src/main/java/com/baidu/hugegraph/version/ApiVersion.java

@@ -87,10 +87,12 @@
     *
     * version 0.10:
     * [0.39] Issue-522: Add profile RESTful API
+     * [0.40] Issue-493: Support batch updating elements' property by multiple
+     *        strategy


keep on one line

javeme · 2019-06-10T03:35:06Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+        return edgeId;
+    }
+
+    protected static class EdgeRequest {


BatchEdgeRequest

javeme · 2019-06-10T03:35:27Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+        }
+    }
+
+    private static class VertexRequest {


BatchVertexRequest

javeme · 2019-06-10T03:47:34Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+    protected void updateExistElement(JsonElement oldElement,
+                                      JsonElement newElement,
+                                      Map<String, UpdateStrategy> strategies) {
+        if (oldElement != null && newElement != null) {


return if oldElement is null
and check newElement != null

javeme · 2019-06-10T03:52:07Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

@@ -114,6 +115,64 @@ public String create(@Context GraphManager manager,
        });
    }

+    /**
+     * TODO: Adapter for param("createIfNotExist") or delete it?


If we really don't use this parameter, just delete it.

@Linary any suggestion?

delete it first, consider it when inneed

javeme · 2019-07-10T13:25:00Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/BatchAPI.java

+    protected static abstract class JsonElement implements Checkable {
+
+        @JsonProperty("id")
+        public Id id;


use object instead, otherwise it will throw error as follows
Can not construct instance of com.baidu.hugegraph.backend.id.Id: abstract types either need to be mapped to concrete types

replace Id with Object

javeme · 2019-07-10T13:30:04Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+
+    // Batch update Set should use union because of higher efficiency
+    APPEND {
+        @Override


add
@SuppressWarnings({ "rawtypes", "unchecked" })

javeme · 2019-07-10T13:30:45Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+    },
+
+    ELIMINATE {
+        @Override


javeme · 2019-07-10T13:33:41Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+        Set newSet = newProperty instanceof Set ?
+                     (Set) newProperty : new HashSet((List) newProperty);
+        return strategy == UNION ? Sets.union(oldSet, newSet) :
+                                   Sets.intersection(oldSet, newSet);


Set is a raw type. References to generic type Set should be parameterized.

please update to:

Set<?> oldSet = oldProperty instanceof Set ? (Set<?>) oldProperty : new HashSet<>((List<?>) oldProperty); Set<?> newSet = newProperty instanceof Set ? (Set<?>) newProperty : new HashSet<>((List<?>) newProperty); return strategy == UNION ? Sets.union(oldSet, newSet) : Sets.intersection(oldSet, newSet);

javeme · 2019-07-12T03:21:10Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

@@ -112,6 +112,7 @@ void checkPropertyType(Object oldProperty, Object newProperty) {
    },

    // Batch update Set should use union because of higher efficiency
+    @SuppressWarnings({ "rawtypes", "unchecked" })


move to line 118

javeme · 2019-07-12T03:23:00Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

@@ -125,6 +126,7 @@ void checkPropertyType(Object oldProperty, Object newProperty) {
        }
    },

+    @SuppressWarnings({ "rawtypes", "unchecked" })


javeme · 2019-07-12T03:29:48Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+            /*
+             * 1.Put all newVertices' properties into map (combine first)
+             * - Consider primary-key & user-define ID mode first
+             * */


…hugegraph into support-update-strategy

javeme · 2019-07-13T05:31:45Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/API.java

+                          Collection<? extends Checkable> bodys) {
+        E.checkArgumentNotNull(bodys, "The request body can't be empty");
+        for (Checkable body : bodys) {
+            E.checkArgumentNotNull(body,


minor improve to shorten the length:
E.checkArgument(body != null,

javeme · 2019-07-13T05:33:55Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

@@ -157,6 +159,64 @@ public String create(@Context GraphManager manager,
        });
    }

+    /**
+     * Batch update steps are same like vertices
+     **/


javeme · 2019-07-13T05:34:52Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+     * 1. Get all newVertices' ID & combine first
+     * 2. Get all oldVertices & update
+     * 3. Add the final vertex together
+     **/


javeme · 2019-07-13T05:36:12Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+            for (Id pkId : pkIds) {
+                String propertyKey = g.propertyKey(pkId).name();
+                Object propertyValue = vertex.properties.get(propertyKey);
+                E.checkState(propertyValue != null,


E.checkArgument

javeme · 2019-07-13T05:36:40Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+            sortKeyIds.forEach(skId -> {
+                String sortKey = g.propertyKey(skId).name();
+                Object sortKeyValue = newEdge.properties.get(sortKey);
+                E.checkState(sortKeyValue != null,


E.checkArgument

javeme

some minor comments

javeme · 2019-07-20T09:12:04Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+        public boolean createIfNotExist = true;
+
+        private static void checkUpdate(BatchVertexRequest req) {
+            E.checkArgumentNotNull(req, "BatchVertexRequest cannot be null");


prefer to replace all "cannot" to "can't"

javeme · 2019-07-20T09:13:10Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

@@ -259,22 +307,70 @@ private static void checkBatchSize(HugeConfig config,
        int max = config.get(ServerOptions.MAX_VERTICES_PER_BATCH);
        if (vertices.size() > max) {
            throw new IllegalArgumentException(String.format(
-                      "Too many counts of vertices for one time post, " +
+                      "Too many vertices for one time post, " +


add check vertices.size() == 0

javeme · 2019-07-20T09:13:51Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+
+    public Object checkAndUpdateProperty(Object oldProperty,
+                                         Object newProperty) {
+        checkPropertyType(oldProperty, newProperty);


tiny improve: this.checkPropertyType()

javeme · 2019-07-20T09:14:03Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+    public Object checkAndUpdateProperty(Object oldProperty,
+                                         Object newProperty) {
+        checkPropertyType(oldProperty, newProperty);
+        return updatePropertyValue(oldProperty, newProperty);


this.updatePropertyValue()

javeme · 2019-07-20T09:27:21Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+                         @PathParam("graph") String graph,
+                         BatchVertexRequest req) {
+        BatchVertexRequest.checkUpdate(req);
+        LOG.debug("Graph [{}] update vertices: {}", graph, req.jsonVertices);


log req instead of req.jsonVertices

javeme · 2019-07-20T09:27:37Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+                         @PathParam("graph") String graph,
+                         BatchEdgeRequest req) {
+        BatchEdgeRequest.checkUpdate(req);
+        LOG.debug("Graph [{}] update edges: {}", graph, req.jsonEdges);


javeme · 2019-07-20T10:35:30Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+                            "Parameter 'update_strategies' cannot be empty");
+            E.checkArgument(req.createIfNotExist == true,
+                            "Parameter 'create_if_not_exist' " +
+                            "dose not supported false now");


dose not support

javeme · 2019-07-20T10:35:45Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/VertexAPI.java

+                            "Parameter 'update_strategies' cannot be empty");
+            E.checkArgument(req.createIfNotExist == true,
+                            "Parameter 'create_if_not_exist' " +
+                            "dose not supported false now");


dose not support

zhoney · 2019-07-22T04:13:43Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/EdgeAPI.java

+                Object sortKeyValue = newEdge.properties.get(sortKey);
+                E.checkArgument(sortKeyValue != null,
+                                "The value of sort key '%s' can't be null",
+                                skId);


skId->sortKey

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

zhoney · 2019-07-22T04:29:43Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+
+    private static String formatError(Object oldProperty, Object newProperty,
+                                      String className) {
+        return String.format("Property must be %s, but got %s, %s", className,


remove static and make error message more clear, like:

Property type must be %s for strategy %s, but got %s

I use javap -v to decompile the class, and find a interesting thing :

formatError() is referenced by a synthetic static method because the inner-class cannot call private method outside. If remove static, we have to make it be non-private (expand the access of method)

And if we choose to do so, seems we can change the other 2 methods(compareNumber & combineSet) from private static to package-default, it also reduces the method calls & number :)

Reference : java-synthetic-methods

making it protected is ok. I think clear error message is more important.

making it protected is ok. I think clear error message is more important.

done, has unified them with non-static methods, and since enum cannot be inherited ,seems default modifier is enough.

javeme · 2019-07-23T03:49:56Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

-                            formatError(oldProperty, newProperty,
-                                        "Set or List"));
+                            this.formatError(oldProperty, newProperty,
+                                             this, "Set or List"));


unnecessary to pass this

javeme · 2019-07-23T03:50:25Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

+    String formatError(Object oldProperty, Object newProperty,
+                       UpdateStrategy strategy, String className) {
+        return String.format("Property type must be %s for strategy %s, " +
+                             "but got type %s, %s", className, strategy,


just use this instead of strategy parameter

javeme · 2019-07-23T03:51:09Z

hugegraph-api/src/main/java/com/baidu/hugegraph/api/graph/UpdateStrategy.java

                             oldProperty.getClass().getSimpleName(),
                             newProperty.getClass().getSimpleName());
    }

-    private static Object compareNumber(Object oldProperty, Object newProperty,
-                                        UpdateStrategy strategy) {
+    Object compareNumber(Object oldProperty, Object newProperty,


keep private static

javeme requested changes May 6, 2019

View reviewed changes

Linary requested changes May 7, 2019

View reviewed changes

imbajin force-pushed the support-update-strategy branch 2 times, most recently from 1f2ff21 to be2b8b3 Compare May 15, 2019 16:24

javeme requested changes May 16, 2019

View reviewed changes

javeme reviewed May 16, 2019

View reviewed changes

javeme requested changes May 19, 2019

View reviewed changes

javeme requested changes May 20, 2019

View reviewed changes

Linary requested changes May 22, 2019

View reviewed changes

Linary requested changes Jun 1, 2019

View reviewed changes

Linary requested changes Jun 3, 2019

View reviewed changes

javeme requested changes Jun 3, 2019

View reviewed changes

imbajin force-pushed the support-update-strategy branch from 205fd75 to 7fd96eb Compare June 4, 2019 03:50

javeme requested changes Jun 4, 2019

View reviewed changes

Linary requested changes Jun 4, 2019

View reviewed changes

imbajin added 9 commits June 6, 2019 22:45

Support batch updating elements' property by multiple strategy

93aab3e

1. Hard code to get elementId now 2. Edge's Direction is always OUT now 3. Other TODOs need finish later (add more unit-test also) 4. Move params to Vertex/EdgeRequest

change some code alignment & Id type in vertexAPI

cf9cd6c

The type of EdgeId is still String because of complexity of conversion

tiny improve

3f44e93

unify elementId & remove unused TODOs

33d480f

fix some problems

54752a5

try to fix test failed

4114cbc

remove useless annotation & tiny improve

b1f6028

Tests show that @NotNull can't intercept null-objcet successfully.

fix return value

cde7c4e

change api-version info

e7fc6af

imbajin force-pushed the support-update-strategy branch from 7fd96eb to e7fc6af Compare June 6, 2019 16:57

javeme requested changes Jun 10, 2019

View reviewed changes

tiny improve

bf46654

javeme requested changes Jul 10, 2019

View reviewed changes

imbajin and others added 2 commits July 11, 2019 17:08

fix warnings

541f9c4

Merge branch 'master' into support-update-strategy

96178e8

javeme requested changes Jul 12, 2019

View reviewed changes

imbajin added 2 commits July 12, 2019 15:14

clean code

bd69e24

Merge branch 'support-update-strategy' of https://github.com/imbajin/…

dd9b877

…hugegraph into support-update-strategy

javeme reviewed Jul 13, 2019

View reviewed changes

clean code

83d35cf

javeme reviewed Jul 20, 2019

View reviewed changes

tiny format

a4b8271

javeme previously approved these changes Jul 22, 2019

View reviewed changes

Linary previously approved these changes Jul 22, 2019

View reviewed changes

zhoney requested changes Jul 22, 2019

View reviewed changes

error format

771ad3f

imbajin dismissed stale reviews from Linary and javeme via 771ad3f July 23, 2019 03:44

imbajin force-pushed the support-update-strategy branch from 9f7911f to 771ad3f Compare July 23, 2019 03:44

javeme requested changes Jul 23, 2019

View reviewed changes

use protected method

59722ad

imbajin force-pushed the support-update-strategy branch from 884a886 to 59722ad Compare July 23, 2019 05:33

zhoney previously approved these changes Jul 23, 2019

View reviewed changes

print right strategy

454e123

imbajin dismissed zhoney’s stale review via 454e123 July 23, 2019 10:06

javeme approved these changes Jul 23, 2019

View reviewed changes

zhoney approved these changes Jul 23, 2019

View reviewed changes

Linary approved these changes Jul 23, 2019

View reviewed changes

javeme merged commit 3fcfce2 into apache:master Jul 23, 2019

gwny mentioned this pull request Nov 20, 2019

属性支持aggregate问题 #755

Closed


		private static class VertexRequest {

		@JsonProperty("jsonVertices")

		checkBatchSize(config, req.jsonEdges);

		HugeGraph g = graph(manager, graph);

Support batch updating elements' property by multiple strategy #493

Support batch updating elements' property by multiple strategy #493

Conversation

imbajin commented Apr 30, 2019

codecov bot commented Apr 30, 2019 • 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

Choose a reason for hiding this comment

javeme May 7, 2019 • edited

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

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

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

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

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

javeme commented May 24, 2019

Linary left a 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

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 Apr 30, 2019 •

edited

javeme May 7, 2019 •

edited