bitflicker64 commented on code in PR #3175:
URL: https://github.com/apache/hugegraph/pull/3175#discussion_r3888191017
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -1012,121 +985,62 @@ public Edge queryEdge(Object edgeId) {
return edge;
}
- @Watched(prefix = "graph")
protected Iterator<Edge> queryEdgesByIds(Object[] edgeIds,
boolean verifyId) {
Query.checkForceCapacity(edgeIds.length);
- List<Id> ids;
- Map<Id, HugeEdge> edges;
- boolean edgesUpdated = this.edgesInTxSize() > 0;
-
if (edgeIds.length == 1) {
- EdgeId id = HugeEdge.getIdValue(edgeIds[0], !verifyId);
+ // Fast path: skip the id list, map and mapper iterator for one id
+ return this.queryEdgeById(edgeIds[0], verifyId);
+ }
+
+ // NOTE: allowed duplicated edges if query by duplicated ids
+ List<Id> ids = InsertionOrderUtil.newList();
+ Map<Id, HugeEdge> edges = new HashMap<>(edgeIds.length);
- boolean tryQueryBackend = true;
+ IdQuery query = new IdQuery(HugeType.EDGE);
+ for (Object edgeId : edgeIds) {
+ HugeEdge edge;
+ EdgeId id = HugeEdge.getIdValue(edgeId, !verifyId);
if (id == null) {
- tryQueryBackend = false;
- ids = ImmutableList.of();
- } else {
- if (id.direction() == Directions.IN) {
- id = id.switchDirection();
- }
- ids = ImmutableList.of(id);
- }
-
- HugeEdge edge = null;
- if (id != null && edgesUpdated) {
- if (this.removedEdges.containsKey(id)) {
- // The record has been deleted
- tryQueryBackend = false;
- } else if ((edge = this.addedEdges.get(id)) != null ||
- (edge = this.updatedEdges.get(id)) != null) {
- // Found from local tx
- tryQueryBackend = false;
- if (edge.expired()) {
- edge = null;
- } else {
- assert edge != null;
- }
- }
+ continue;
}
-
- if (edge != null) {
- assert !tryQueryBackend;
- edges = ImmutableMap.of(edge.id(), edge);
- } else if (!tryQueryBackend) {
- assert edge == null;
- edges = ImmutableMap.of();
- } else {
- // Query from backend store
- IdQuery query = new IdQuery.OneIdQuery(HugeType.EDGE, id);
- Iterator<HugeEdge> it = this.queryEdgesFromBackend(query);
- edge = QueryResults.one(it);
- if (edge == null) {
- edges = ImmutableMap.of();
- } else {
- edges = ImmutableMap.of(edge.id(), edge);
- }
+ if (id.direction() == Directions.IN) {
+ id = id.switchDirection();
}
- } else {
- // NOTE: allowed duplicated edges if query by duplicated ids
- ids = InsertionOrderUtil.newList();
- edges = new HashMap<>(edgeIds.length);
-
- IdQuery query = new IdQuery(HugeType.EDGE);
- for (Object edgeId : edgeIds) {
- HugeEdge edge;
- EdgeId id = HugeEdge.getIdValue(edgeId, !verifyId);
- if (id == null) {
+ if (this.removedEdges.containsKey(id)) {
+ // The record has been deleted
+ continue;
+ } else if ((edge = this.addedEdges.get(id)) != null ||
+ (edge = this.updatedEdges.get(id)) != null) {
+ if (edge.expired()) {
continue;
}
- if (id.direction() == Directions.IN) {
- id = id.switchDirection();
- }
-
- boolean foundLocal = false;
- if (edgesUpdated) {
- if (this.removedEdges.containsKey(id)) {
- // The record has been deleted
- continue;
- }
- if ((edge = this.addedEdges.get(id)) != null ||
- (edge = this.updatedEdges.get(id)) != null) {
- if (edge.expired()) {
- continue;
- }
- // Found from local tx
- foundLocal = true;
- edges.put(edge.id(), edge);
- } else {
- assert !foundLocal;
- }
- }
- if (!foundLocal) {
- // Prepare to query from backend store
- query.query(id);
- }
- ids.add(id);
+ // Found from local tx
+ edges.put(edge.id(), edge);
+ } else {
+ // Prepare to query from backend store
+ query.query(id);
}
+ ids.add(id);
+ }
- if (!query.empty()) {
- // Query from backend store
- if (edges.isEmpty() && query.idsSize() == ids.size()) {
- /*
- * Sort at the lower layer and return directly if there is
- * no local vertex and duplicated id.
- */
- Iterator<HugeEdge> it = this.queryEdgesFromBackend(query);
- @SuppressWarnings({ "unchecked", "rawtypes" })
- Iterator<Edge> r = (Iterator) it;
- return r;
- }
-
- query.mustSortByInput(false);
+ if (!query.empty()) {
+ // Query from backend store
+ if (edges.isEmpty() && query.idsSize() == ids.size()) {
Review Comment:
โ ๏ธ This is the pre-#2982 duplicate-id check, not the `distinctIds` one the
description says it is.
Master carries `Set<Id> distinctIds =
InsertionOrderUtil.newSet(edgeIds.length)` (`GraphTransaction.java:940`),
`distinctIds.add(id)` (`:967`) and `if (edges.isEmpty() && distinctIds.size()
== ids.size())` (`:972`). #2982 changed exactly this line, from
`query.idsSize() == ids.size()` to the `distinctIds` form. This head has the
old form.
Why the two are not equivalent: `IdQuery.query(Id)` skips a duplicate only
when it is *consecutive* (`IdQuery.java:98-102`). For `graph.edges(a, b, a)`
with nothing in the local tx, the query ends up holding `[a, b, a]`, so
`query.idsSize()` is 3, `ids.size()` is 3, and this branch hands the backend
iterator straight back for a query that contains a duplicate id โ the
batched-lookup case #2982 fixed.
Nothing regresses against *this* PR's base: `only-one-id-query-optimize`
predates #2982 and already reads `query.idsSize() == ids.size()` at its
`GraphTransaction.java:1115`, and the diff only re-indents the line. The
problem is the description, which says "The multi-id edge path keeps the
`distinctIds` check from #2982: `[a, b, a]` still yields three edges
(`testQueryEdgesByNonConsecutiveDuplicateIds`)". Neither `distinctIds` nor that
test exists at this head or on the base branch โ only the vertex counterpart
was added (`VertexCoreTest.java:3131
testQueryVerticesByNonConsecutiveDuplicateIds`). So the rebase onto master that
#2859 still needs has nothing to catch it if this side of the conflict wins.
Requested change: either bring master's `distinctIds` set onto this line now
and add the edge counterpart of
`testQueryVerticesByNonConsecutiveDuplicateIds`, or correct that bullet in the
PR description so whoever rebases #2859 knows this specific line has to come
from master rather than from this branch.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -784,131 +775,117 @@ protected Iterator<Vertex> queryVerticesByIds(Object[]
vertexIds, boolean adjace
return this.queryVerticesByIds(vertexIds, adjacentVertex,
checkMustExist, HugeType.VERTEX);
}
- @Watched(prefix = "graph")
protected Iterator<Vertex> queryVerticesByIds(Object[] vertexIds, boolean
adjacentVertex,
boolean checkMustExist,
HugeType type) {
Query.checkForceCapacity(vertexIds.length);
- List<Id> ids;
- Map<Id, HugeVertex> vertices;
- boolean verticesUpdated = this.verticesInTxSize() > 0;
-
if (vertexIds.length == 1) {
- Id id = HugeVertex.getIdValue(vertexIds[0]);
+ // Fast path: skip the id list, map and mapper iterator for one id
+ return this.queryVertexById(vertexIds[0], adjacentVertex,
+ checkMustExist, type);
+ }
- boolean tryQueryBackend = true;
- if (id == null) {
- tryQueryBackend = false;
- ids = ImmutableList.of();
- } else {
- ids = ImmutableList.of(id);
- }
-
- HugeVertex vertex = null;
- if (id != null && verticesUpdated) {
- if (this.removedVertices.containsKey(id)) {
- // The record has been deleted
- tryQueryBackend = false;
- } else if ((vertex = this.addedVertices.get(id)) != null ||
- (vertex = this.updatedVertices.get(id)) != null) {
- // Found from local tx
- tryQueryBackend = false;
- if (vertex.expired()) {
- vertex = null;
- } else {
- assert vertex != null;
- }
- }
- }
+ // NOTE: allowed duplicated vertices if query by duplicated ids
+ List<Id> ids = InsertionOrderUtil.newList();
+ Map<Id, HugeVertex> vertices = new HashMap<>(vertexIds.length);
- if (vertex != null) {
- assert !tryQueryBackend;
- vertices = ImmutableMap.of(vertex.id(), vertex);
- } else if (!tryQueryBackend) {
- assert vertex == null;
- vertices = ImmutableMap.of();
- } else {
- // Query from backend store
- IdQuery query = new IdQuery.OneIdQuery(type, id);
- Iterator<HugeVertex> it = this.queryVerticesFromBackend(query);
- vertex = QueryResults.one(it);
- if (vertex == null) {
- vertices = ImmutableMap.of();
- } else {
- vertices = ImmutableMap.of(vertex.id(), vertex);
- }
- }
- } else {
- // NOTE: allowed duplicated vertices if query by duplicated ids
- ids = InsertionOrderUtil.newList();
- vertices = new HashMap<>(vertexIds.length);
-
- IdQuery query = new IdQuery(type);
- for (Object vertexId : vertexIds) {
- Id id = HugeVertex.getIdValue(vertexId);
- if (id == null) {
+ IdQuery query = new IdQuery(type);
+ for (Object vertexId : vertexIds) {
+ HugeVertex vertex;
+ Id id = HugeVertex.getIdValue(vertexId);
+ if (id == null || this.removedVertices.containsKey(id)) {
+ // The record has been deleted
+ continue;
+ } else if ((vertex = this.addedVertices.get(id)) != null ||
+ (vertex = this.updatedVertices.get(id)) != null) {
+ if (vertex.expired()) {
continue;
}
- boolean foundLocal = false;
- if (verticesUpdated) {
- HugeVertex vertex;
- if (this.removedVertices.containsKey(id)) {
- // The record has been deleted
- continue;
- }
- if ((vertex = this.addedVertices.get(id)) != null ||
- (vertex = this.updatedVertices.get(id)) != null) {
- if (vertex.expired()) {
- continue;
- }
- // Found from local tx
- foundLocal = true;
- vertices.put(vertex.id(), vertex);
- } else {
- assert !foundLocal;
- }
- }
- if (!foundLocal) {
- // Prepare to query from backend store
- query.query(id);
- }
- ids.add(id);
+ // Found from local tx
+ vertices.put(vertex.id(), vertex);
+ } else {
+ // Prepare to query from backend store
+ query.query(id);
}
+ ids.add(id);
+ }
- if (!query.empty()) {
- // Query from backend store
- query.mustSortByInput(false);
- Iterator<HugeVertex> it = this.queryVerticesFromBackend(query);
- QueryResults.fillMap(it, vertices);
- }
+ if (!query.empty()) {
+ // Query from backend store
+ query.mustSortByInput(false);
+ Iterator<HugeVertex> it = this.queryVerticesFromBackend(query);
+ QueryResults.fillMap(it, vertices);
}
return new MapperIterator<>(ids.iterator(), id -> {
- HugeVertex vertex = vertices.get(id);
+ return this.resolveVertex(vertices.get(id), id,
+ adjacentVertex, checkMustExist);
+ });
+ }
+
+ /**
+ * Query a single vertex by id, with the same semantics as the multi-id
+ * path of {@link #queryVerticesByIds(Object[], boolean, boolean,
HugeType)}
+ * but without allocating the id list, the result map and the mapper
+ * iterator; only an {@link IdQuery.OneIdQuery} is created on a miss.
+ */
+ private Iterator<Vertex> queryVertexById(Object vertexId, boolean
adjacentVertex,
+ boolean checkMustExist, HugeType
type) {
+ Id id = HugeVertex.getIdValue(vertexId);
+ if (id == null) {
+ return QueryResults.emptyIterator();
+ }
+
+ HugeVertex vertex = null;
+ if (this.verticesInTxSize() > 0) {
+ if (this.removedVertices.containsKey(id)) {
+ // The record has been deleted
+ return QueryResults.emptyIterator();
+ }
+ vertex = this.addedVertices.get(id);
if (vertex == null) {
- if (checkMustExist) {
- throw new NotFoundException(
- "Vertex '%s' does not exist", id);
- } else if (adjacentVertex) {
- assert !checkMustExist;
- // Return undefined if adjacentVertex but !checkMustExist
- vertex = HugeVertex.undefined(this.graph(), id);
- } else {
- // Return null
- assert vertex == null;
- }
+ vertex = this.updatedVertices.get(id);
+ }
+ if (vertex != null && vertex.expired()) {
+ // Found from local tx but expired
+ return QueryResults.emptyIterator();
}
+ }
+
+ if (vertex == null) {
+ // Query from backend store
+ IdQuery query = new IdQuery.OneIdQuery(type, id);
+ vertex = QueryResults.one(this.queryVerticesFromBackend(query));
+ }
+
+ vertex = this.resolveVertex(vertex, id, adjacentVertex,
checkMustExist);
Review Comment:
๐งน The fast path resolves eagerly, so `NotFoundException` now fires at call
time instead of on iteration.
`queryVerticesByIds` previously returned a lazy `MapperIterator` in every
case, and the `checkMustExist` throw happened inside the mapper โ that is, on
the first `hasNext()`/`next()` (`MapperIterator.fetch()`,
`hugegraph-commons/.../iterator/MapperIterator.java:39-50`). Here
`resolveVertex` runs before the iterator is constructed, so
`queryVerticesByIds(...)` itself throws.
`queryVertex(Object)` is unaffected: it consumes with
`QueryResults.one(iter)` on the next line (`GraphTransaction.java:734-742`).
The visible difference is through `queryAdjacentVertices(Object...)`
(`:725-728`), which passes `checkMustExist = this.checkAdjacentVertexExist` โ
with `vertex.check_adjacent_vertex_exist=true` (not the default),
`graph.adjacentVertex(missingId)` raises when the iterator is built rather than
when it is read. It also leaves the one-id and many-id paths throwing at
different moments for the same call, which is the kind of difference the rest
of this change is careful to avoid.
Requested change: keep the resolution inside the returned iterator โ a small
lazy single-element iterator preserves the allocation win this method is after
โ or, if the eager throw is intentional, say so in the javadoc above so it
reads as a deliberate contract change rather than a side effect of the
optimization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
