bitflicker64 commented on code in PR #3193:
URL: https://github.com/apache/hugegraph/pull/3193#discussion_r3949905190
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/page/QueryList.java:
##########
@@ -344,10 +354,11 @@ public boolean hasNextPage() {
}
public Query query() {
- List<Query> queries = this.results.queries();
- E.checkState(queries.size() == 1,
- "Expect query size 1, but got: %s", queries);
- return queries.get(0);
+ return this.query;
Review Comment:
🧹 `query()` and `get()` have no callers left after this change.
`query()` existed at base, deriving from `this.results.queries()` behind an
`E.checkState(queries.size() == 1)`. This commit reworks it onto a new `query`
field (line 339) and a third constructor parameter (line 341), with
`emptyIterator()` passing `null` for it (line 374).
But the rewritten `PageEntryIterator` is the only `PageResults` consumer in
main code (`PageEntryIterator.java:62`), and it reads `results()`,
`hasNextPage()`, `page()` and `total()` only. Base `PageEntryIterator:79`
called `pageResults.query()`, and `:60/81/97/106` called `pageResults.get()`;
both are gone. `git grep` over this head finds no call to either accessor, and
`QueryListTest:49-50` uses `.results()`.
Suggest dropping `get()`, `query()`, the field and the constructor
parameter, returning `PageResults` to `(results, pageState)`, which also
removes the `null` argument in `emptyIterator()`.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedGraphTransaction.java:
##########
@@ -316,135 +318,143 @@ private boolean needCacheVertex(HugeVertex vertex) {
@Override
@Watched(prefix = "graphcache")
- protected Iterator<HugeVertex> queryVerticesFromBackend(Query query) {
- if (this.enableCacheVertex() &&
- query.idsSize() > 0 && query.conditionsSize() == 0 &&
- !queryNeedsPostFilter(query)) {
- return this.queryVerticesByIds((IdQuery) query);
- } else {
- return super.queryVerticesFromBackend(query);
+ protected QueryResults<HugeVertex> fetchVertexBatch(Query query) {
+ if (!this.enableCacheVertex() || query.paging() ||
+ query.idsSize() == 0 || query.conditionsSize() != 0) {
+ return super.fetchVertexBatch(query);
}
- }
-
- @Watched(prefix = "graphcache")
- private Iterator<HugeVertex> queryVerticesByIds(IdQuery query) {
- if (query.idsSize() == 1) {
- Id vertexId = query.ids().iterator().next();
- HugeVertex vertex = (HugeVertex) this.verticesCache.get(vertexId);
- if (vertex != null) {
- if (!vertex.expired()) {
- return QueryResults.iterator(vertex);
- }
- this.verticesCache.invalidate(vertexId);
- }
- Iterator<HugeVertex> rs = super.queryVerticesFromBackend(query);
- vertex = QueryResults.one(rs);
- if (vertex == null) {
- return QueryResults.emptyIterator();
- }
- if (needCacheVertex(vertex)) {
- this.verticesCache.update(vertex.id(), vertex);
- }
- return QueryResults.iterator(vertex);
- }
-
- IdQuery newQuery = new IdQuery(HugeType.VERTEX, query);
+ QueryResultContext context = new QueryResultContext(query);
+ IdQuery missing = new IdQuery(query.resultType(), query);
List<HugeVertex> vertices = new ArrayList<>();
- for (Id vertexId : query.ids()) {
- HugeVertex vertex = (HugeVertex) this.verticesCache.get(vertexId);
- if (vertex == null) {
- newQuery.query(vertexId);
- } else if (vertex.expired()) {
- newQuery.query(vertexId);
- this.verticesCache.invalidate(vertexId);
+ for (Id id : query.ids()) {
+ HugeVertex vertex = (HugeVertex) this.verticesCache.get(id);
+ if (vertex == null || vertex.expired()) {
+ missing.query(id);
+ if (vertex != null) {
+ this.verticesCache.invalidate(id);
+ }
} else {
vertices.add(vertex);
}
}
-
- // Join results from cache and backend
- ExtendableIterator<HugeVertex> results = new ExtendableIterator<>();
- if (!vertices.isEmpty()) {
- results.extend(vertices.iterator());
- } else {
- // Just use the origin query if find none from the cache
- newQuery = query;
- }
-
- if (!newQuery.empty()) {
- Iterator<HugeVertex> rs = super.queryVerticesFromBackend(newQuery);
- // Generally there are not too much data with id query
- ListIterator<HugeVertex> listIterator = QueryResults.toList(rs);
- for (HugeVertex vertex : listIterator.list()) {
- // Skip large vertex
- if (needCacheVertex(vertex)) {
+ if (!missing.empty()) {
+ QueryResults<HugeVertex> fetched =
super.fetchVertexBatch(vertices.isEmpty() ? query : missing);
+ if (vertices.isEmpty() && !fetched.batches().hasNext()) {
+ return fetched;
+ }
+ ListIterator<HugeVertex> candidates =
QueryResults.toList(fetched.iterator());
+ for (HugeVertex vertex : candidates.list()) {
+ if (this.needCacheVertex(vertex)) {
this.verticesCache.update(vertex.id(), vertex);
}
+ vertices.add(vertex);
}
- results.extend(listIterator);
}
-
- return results;
+ // Keep hits and misses in one logical batch for filtering and ID
ordering.
+ return this.filterExpiredBatches(new
QueryResults<>(vertices.iterator(), context));
}
@Override
- @Watched(prefix = "graphcache")
- protected Iterator<HugeEdge> queryEdgesFromBackend(Query query) {
+ protected QueryResults<HugeEdge> queryEdgesFromMemory(Query query) {
RamTable ramtable = this.params().ramtable();
if (ramtable != null && ramtable.matched(query)) {
- return ramtable.query(query);
+ return new QueryResults<>(ramtable.query(query), query);
}
+ return null;
+ }
- if (!this.enableCacheEdge() || query.empty() || query.paging() ||
- query.bigCapacity() || queryNeedsPostFilter(query)) {
- // Don't cache all-edge, paging, large, or post-filtered queries
- return super.queryEdgesFromBackend(query);
+ @Override
+ @Watched(prefix = "graphcache")
+ protected QueryResults<HugeEdge> fetchEdgeBatch(Query query) {
+ QueryResultContext context = new QueryResultContext(query);
+ List<Query> chain = context.queries();
+ Query request = chain.get(chain.size() - 1);
+ if (!this.enableCacheEdge() || request.empty() || request.paging() ||
request.bigCapacity()) {
Review Comment:
🧹 Was dropping the `queryNeedsPostFilter` exclusion from this guard
intentional?
Base `:399` had it in the same condition, plus a re-check after the backend
call at `:424` ("re-check before caching"). Base `queryNeedsPostFilter`
returned true for `hasSearchCondition()`, and for `optimized() != NONE` unless
the edge query also carried a LABEL condition. So at base `g.E().has(<indexed
prop>, v)` and primary-key/sort-key optimized edge queries were never cached,
while `g.E().hasLabel(L).has(p, v)` was.
Here only `empty`/`paging`/`bigCapacity` remain. Since `fetchEdgeBatch` is
now the `QueryList` fetcher rather than a `queryEdgesFromBackend` override, the
index-derived `IdQuery` leaf reaches it with `request` (line 371, the chain
root) equal to the user `ConditionQuery`: non-empty, non-paging, default
capacity. Line 404 then caches it.
Results stay correct, because `processBatches` re-applies
`filterUnmatchedRecord` on hits as well as misses. The cost is key count:
`edge.cache_capacity` defaults to 1,000,000 entries
(`CoreOptions.java:566-572`) on a per-graph cache, so high-cardinality indexed
lookups now mint one guaranteed-miss key per predicate value and evict the
adjacency entries the cache exists to serve. `fetchVertexBatch` at `:322`
dropped the same guard, which looks harmless there since `verticesCache` is
id-keyed.
If the widening is deliberate, a line in the PR body would save the next
reader this trace. If not, `context.conditionFilterRequired()` (the context is
already built on line 369) is the ready-made predicate, and the base's LABEL
carve-out is worth keeping.
--
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]
