bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r4040407125
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -1957,7 +1957,8 @@ private boolean rightResultFromIndexQuery(Query query,
HugeElement elem) {
}
ConditionQuery cq = (ConditionQuery) query;
- if (cq.condition(HugeKeys.LABEL) != null && cq.resultType().isEdge()) {
+ if (cq.singleConditionValueOrNull(HugeKeys.LABEL) != null &&
Review Comment:
⚠️ The pending rebase re-introduces two of the exact LABEL call sites this
PR migrates.
`mergeable` is `CONFLICTING`: master moved from this head's merge-base
`1a15e762` to `cbc3dc5a0`, and `ee22dd7ef` (#3193, "make query result batch
boundaries explicit") rewrote the two `GraphTransaction` methods migrated here.
```
git merge-tree --write-tree bcb8c1f3 origin/master
# CONFLICT (content): .../backend/tx/GraphTransaction.java
# CONFLICT (content): .../backend/tx/GraphTransactionTest.java
```
Three conflict hunks, two of them on migrated lines:
1. `queryEdgeBatchesFromBackend()` — master replaces the `Stream` pipeline
with a `Function<ConditionQuery, QueryResults<HugeEdge>> fetcher` that carries
`Id label = cq.condition(HugeKeys.LABEL);` (`origin/master`
`GraphTransaction.java:1086`), conflicting with line 1065 here.
2. `rightResultFromIndexQuery()` — master refactors it around
`QueryResultContext` and keeps `if (cq.condition(HugeKeys.LABEL) != null &&
cq.resultType().isEdge())` (`origin/master` `GraphTransaction.java:1930`),
conflicting with this line.
3. `queryNeedsPostFilter()` — master moves it, so the `edgeIndexWithLabel`
gate at line 2024 has no home on master's side of the conflict.
Resolving those hunks in master's favour — the natural choice when the other
side is a refactor — silently reverts two migrations. `git grep -n
"condition(HugeKeys.LABEL)" origin/master` confirms the legacy accessor is back
at both sites. Both are edge paths where a multi-label query makes
`condition()` return the raw `List`, i.e. the `ClassCastException` family this
PR exists to remove, and neither has a direct regression test here to catch the
revert.
Requested change: after rebasing, re-apply
`singleConditionValueOrNull(HugeKeys.LABEL)` inside master's new `fetcher`
lambda and in `rightResultFromIndexQuery()`, and re-home the
`edgeIndexWithLabel` gate into wherever #3193 moved `queryNeedsPostFilter()`;
then re-run the edge lanes.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java:
##########
@@ -768,11 +771,17 @@ private PageIds doIndexQueryOnce(IndexLabel indexLabel,
@Watched(prefix = "index")
private Set<MatchedIndex> collectMatchedIndexes(ConditionQuery query) {
ISchemaTransaction schema = this.params().schemaTransaction();
- Id label = query.condition(HugeKeys.LABEL);
+ boolean hasLabelValues = query.containsConditionValues(HugeKeys.LABEL);
+ Set<Object> labels = query.conditionValues(HugeKeys.LABEL);
List<? extends SchemaLabel> schemaLabels;
- if (label != null) {
- // Query has LABEL condition
+ if (hasLabelValues && labels.isEmpty()) {
+ // LABEL EQ/IN conditions resolve to an empty intersection.
+ return Collections.emptySet();
Review Comment:
🧹 This empty-intersection guard is duplicated, and its second copy feeds a
dereference guarded only by `assert`.
`queryByUserprop()` already short-circuits the same condition at line 483,
so this copy is dead for that caller. It is live for the other caller,
`RemoveLeftIndexJob.findMatchedIndexLabel()` (line 1868): an empty set makes it
return `null`, and `removeIndexLeft()` then does `assert indexLabel != null;`
(line 1849) before dereferencing it. Assertions are disabled in production, so
this branch adds a second route to an NPE there.
I could not construct a query that reaches it — an unsatisfiable label
intersection produces no index results and therefore no left indexes, and two
LABEL relations are rejected earlier by `queryIndex()`'s `conds.size() > 1`
check — so this is defensive only.
Requested change: keep the guard in one place (drop it here and let
`queryByUserprop()` own it), or make `findMatchedIndexLabel()`'s `null` case an
explicit early return rather than relying on the `assert`.
--
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]
