bitflicker64 commented on code in PR #3182:
URL: https://github.com/apache/hugegraph/pull/3182#discussion_r3923381662
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedGraphTransaction.java:
##########
@@ -318,7 +318,8 @@ private boolean needCacheVertex(HugeVertex vertex) {
@Watched(prefix = "graphcache")
protected Iterator<HugeVertex> queryVerticesFromBackend(Query query) {
if (this.enableCacheVertex() &&
- query.idsSize() > 0 && query.conditionsSize() == 0) {
+ query.idsSize() > 0 && query.conditionsSize() == 0 &&
+ !queryNeedsPostFilter(query)) {
Review Comment:
🧹 This guard decides before the backend call. The edge path re-checks after
`super.queryEdgesFromBackend(query)` at line 424, and its new comment gives the
reason: that call can promote `query.optimized()` through origin-query
propagation. Nothing records why the vertex path needs no equivalent.
It does not need one, and the reason is worth stating, because the clause is
live rather than defensive. `queryNumber` falls back to `this.queryVertices(q)`
at GraphTransaction.java:597-598 with `q` being whatever `optimizeQueries`
handed its fetcher (561), so `q` arrives here as an `IdQuery` whose origin
`ConditionQuery` already carries `PRIMARY_KEY`, set at 1547 before the
`IdQuery` is built at 1556, or `INDEX`, set at GraphIndexTransaction.java:402
while `indexQuery` runs. The PR's own
`testQueryByPrimaryValuesAndPropsWithCachedVertex` covers the first shape.
`optimized()` is therefore already fixed when this line runs, and
`queryVerticesByIds` cannot promote it further: `GraphTransaction.query(Query)`
(535-540) short-circuits to `super.query(query)` for a non-`ConditionQuery`, so
`optimizeQueries` never runs beneath it.
Requested change: add a one-line comment here mirroring the one at 424. The
asymmetry currently reads as an oversight.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -1870,33 +1882,67 @@ private void removeLeftIndexIfNeeded(Map<Id,
HugeVertex> vertices) {
}
}
+ private <T extends HugeElement> Iterator<T> filterInvalidRecords(
+ Iterator<T> results,
+ Query query) {
+ // Filter unused records
+ return new FilterIterator<>(results, elem -> {
+ warnLeftRecord(elem);
+ return !invalidRecord(elem, query);
+ });
+ }
+
private <T extends HugeElement> Iterator<T> filterUnmatchedRecords(
Iterator<T> results,
Query query) {
- // Filter unused or incorrect records
+ /*
+ * Filter against the current index sub-query before restoring input
+ * order, since the order iterator may prefetch the next sub-query and
+ * update the results filter of the origin query.
+ * FilterIterator tests and buffers each element before its upstream
+ * iterator is advanced again, so the element is always tested with
+ * the results filter of the sub-query that produced it.
+ */
return new FilterIterator<>(results, elem -> {
- // TODO: Left vertex/edge should to be auto removed via async task
- if (elem.schemaLabel().undefined()) {
- LOG.warn("Left record is found: id={}, label={},
properties={}",
- elem.id(), elem.schemaLabel().id(),
- elem.getPropertiesMap());
- }
- // Filter hidden results
- if (!query.showHidden() && Graph.Hidden.isHidden(elem.label())) {
- return false;
- }
- // Filter vertices/edges of deleting label
- if (elem.schemaLabel().status().deleting() &&
- !query.showDeleting()) {
- return false;
+ /*
+ * Preserve the original predicate order: hidden records and
+ * records of deleting labels must be handled by the downstream
+ * invalid-record filter without triggering left-index cleanup.
+ */
+ if (invalidRecord(elem, query)) {
+ return true;
}
// Process results that query from left index or primary-key
// Only index query will come here
- return query.resultType().isVertex() != elem.type().isVertex() ||
- rightResultFromIndexQuery(query, elem);
+ boolean matched =
+ query.resultType().isVertex() != elem.type().isVertex() ||
+ rightResultFromIndexQuery(query, elem);
+ if (!matched) {
+ warnLeftRecord(elem);
Review Comment:
🧹 The `LOG.warn` behind this call cannot fire for a query that leaves
`showHidden` at its default, which is narrower than the block above reads.
A left record's label name is `SchemaElement.UNDEF`, `"~undefined"`
(SchemaElement.java:57), returned by `HugeVertex.label()` (198-200) and
`HugeEdge.label()` (117-119), and `Graph.Hidden.isHidden` is a `"~"` prefix
test. So `invalidRecord` at 1912 is true for every left record when
`query.showHidden()` is false, and the early `return true` runs before control
reaches 1920.
The warning is not lost. On `queryVertices(Query)` and `queryEdges(Query)`
it comes from `filterInvalidRecords` at 1890, which is where master emitted it.
Line 1921 is reached only by the `showHidden(true)` callers, such as
`EntityManager.queryEntity` (158) and `traverseByLabel` (2336). On the internal
call sites this PR routes through `filterUnmatchedRecords` with no downstream
`filterInvalidRecords` (380, 806, 983, 990, 1809, 1874) it adds nothing, since
all six build their own query and leave `showHidden` false (Query.java:99).
Requested change: record that split in the comment at 1907-1911, naming 1890
as the call that covers ordinary queries. As written the block reads as though
this line restores left-record logging for index queries in general.
--
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]
