fix(server): clarify condition resolution semantics for label queries - #2994
fix(server): clarify condition resolution semantics for label queries#2994contrueCT wants to merge 46 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2994 +/- ##
============================================
- Coverage 37.77% 37.76% -0.01%
- Complexity 6560 6621 +61
============================================
Files 800 800
Lines 68960 69323 +363
Branches 9166 9274 +108
============================================
+ Hits 26052 26183 +131
- Misses 39841 40042 +201
- Partials 3067 3098 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4c42786 to
cc9af24
Compare
There was a problem hiding this comment.
I found one correctness issue in the latest revision. The CI failures were posted separately as a PR-level reminder.
CI/status checks are failing on the latest head (cc9af24929e42af1c90e1f55f3e60adc351e0318). Could you check the failed jobs before the next review round?
Failed checks include:
build-server (memory, 11): https://github.com/apache/hugegraph/actions/runs/26448131941/job/77861497015
cc9af24 to
2e82f83
Compare
There was a problem hiding this comment.
I don't see a clear blocking correctness issue in the latest head, and the previous LABEL-resolution comments look addressed. One remaining merge risk is that the latest checks are still red: hstore failed in VertexCoreTest#testQueryByDateProperty.
Since this PR also touches HstoreStore, could you rerun or clarify whether the hstore failure is an existing flaky/environment issue?
Add explicit condition resolution APIs to ConditionQuery while preserving the legacy condition() behavior. Introduce containsCondition(Object), conditionValues(Object), and conditionValue(Object) so callers can distinguish missing, empty, unique, and multi-value results without overloading null semantics. Migrate LABEL-specific consumers in graph/index transactions, serializers, traversers, and stores to use the new APIs for unique-label resolution and conservative fallback behavior. Extend QueryTest and VertexCoreTest to cover absent, conflicting, and multi-value label conditions as well as collectMatchedIndexes() behavior for multi-label and conflicting label queries.
94408b7 to
b10e3c2
Compare
801923a to
ebc31c8
Compare
|
Thanks for your patience. The hstore CI failure exposed an existing latent issue in hstore's range-index query path. For range-index scans with limit/paging, the upper layer assumed that backend scan results were globally ordered by the range-index key and that the returned page state could be reused as a HugeGraph range cursor. In hstore, multi-node/tablet scans can return entries in backend iterator order, and the page state is an internal storage cursor, so those assumptions may lead to unstable ordering or skipped results. This PR keeps the fix intentionally scoped: hstore range-index queries whose visible result depends on limit/offset/paging are sorted and sliced in the index layer, while unbounded scans still use the original streaming path to avoid disturbing count, joint-index, and cleanup paths. I think this is enough for the current PR, but the underlying hstore scan/page-state contract should be handled in a dedicated follow-up, ideally by defining whether range scans must be globally ordered and fixing the hstore iterator/page-state semantics at the storage-client layer. |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: HStore range-index offset queries can skip too many sorted results. Evidence: static review of GraphIndexTransaction/query offset handling.
|
Thanks. I fixed this by resetting |
|
CI follow-up: the Store retry passed completely (11m40s), confirming the earlier failure was the pre-checkout Zulu JDK download timeout. The HStore lane then failed twice, including repository-side rerun attempt 2, at the exact failure already tracked in #3180: |
|
Cross-backend results for this branch (head Before — master Both throw on RocksDB as well, so this is server-side condition resolution, not a backend issue. The other 33 cases are identical sets on both backends already on master. After — master + this branch (+ #3184 and the since-merged #3182): 35/35 identical sets; the two queries above return 0 and 100 vertices respectively, identically on both backends. Version axis (RocksDB master vs RocksDB with the branch, backend held constant): 168 of 174 queries across all four sections (sort-key pushdown, range-index paging, label semantics, Caveat: the "after" run was the combined branch, not #2994 alone; #3184 only touches the HStore pushdown path and #3182 is now in master, so attribution of the two fixes to this PR is by elimination. I can run #2994 alone on request. Raw reports (ids included) and the case list: https://github.com/SebastianGruza/hugegraph-oracle-suite — |
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The condition-resolution split reads well and all 35 earlier review threads are resolved at this head; I have one error-handling regression in the new local-filter path, one gap in the unsafe-label gate that the PR does not close, and one minor redundancy. Evidence: mvn -o -q compile -pl hugegraph-server/hugegraph-core -am passed; all 24 GitHub checks are green at 0ce61d0; findings below come from static review of git diff 98477f0f..0ce61d0 and the surrounding TraversalUtil / GraphIndexTransaction code. Not verified by a local test run.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The ConditionQuery decomposition and the LABEL call-site migration are sound, and they fix a real family of latent ClassCastExceptions on multi-label queries. All three findings are in the new TraversalUtil unsafe-label gate; the first one turns g.V().hasId(x).limit(n).hasLabel(P.neq(...)) from a point lookup into a full table scan that cannot terminate early, which fails outright above Query.DEFAULT_CAPACITY.
Evidence: static review of head 15e052f4 against merge base 36811483, plus the TinkerPop 3.5.1 HasContainer sources (testLabel is protected and non-final and clone() preserves the subclass, so LocalLabelHasContainer is safe). I did not build or run the tests, so findings 1 and 2 are reasoning over HugeGraphStep.makeQuery() and BackendEntryIterator.checkCapacity() rather than measured queries; an execution-plan assertion in TraversalUtilOptimizeTest would settle both.
| holder.removeHasContainer(has); | ||
| continue; | ||
| } | ||
| if (T.id.getAccessor().equals(has.getKey())) { |
There was a problem hiding this comment.
~id never reaches the source step, so a point lookup becomes an unbounded full scan
Evidence: on this path no has container reaches newStep.hasContainers (only ~page at line 690 and the paging range at line 734 are pushed). HugeGraphStep.makeQuery() then takes the this.hasContainers.isEmpty() branch and builds new Query(type), and vertices() takes the !hasIds() branch. At the merge base the same container went through GraphStep.processHasContainerIds(), which set graphStep.ids and gave graph.vertices(this.ids).
So g.V().hasId(one).limit(10).hasLabel(P.neq("other")), the shape asserted at VertexCoreTest.java:9268, scans the whole vertex table. The surviving HasStep also blocks extractRange(), so limit(10) is not pushed either, and since exactly one vertex can match, the RangeGlobalStep drains the scan to exhaustion. Query starts at DEFAULT_CAPACITY = 800000 (Query.java:51, 97) and BackendEntryIterator.checkCapacity() throws LimitExceedException past it, so above that size this query stops returning the vertex at all. The neighbouring g.V().hasId(one).toList() at line 9266 keeps its point lookup, so the test is green either way.
The gate's rationale does not apply to ~id: an id fetch is label-agnostic and complete, so keeping it local buys nothing.
Requested change: for HugeGraphStep sources, still call GraphStep.processHasContainerIds(newStep, has) on T.id containers and fall back to this local rewrite only when it returns false. Please assert the resulting step ids in a regression, so the plan is pinned and not just the result set.
| // steps don't reliably expose element identity, so stay conservative. | ||
| // FIXME: Restore selective pushdown when every candidate schema label | ||
| // has compatible index coverage for extracted property predicates. | ||
| List<Step> steps = traversal.getSteps(); |
There was a problem hiding this comment.
This is the cost side of what I asked for on 2026-09-02 and 2026-09-05, not a request to undo it. The traversal-level condition and the getParent() walk are the right shape; the ask here is to bound their breadth.
Evidence: the walk enumerates every remaining step of the traversal, plus each ancestor's remaining steps and all their children, and hasUnsafeLabelPredicate() (line 916) inspects only the key and the predicate. Nothing tracks which elements a step filters. For
g.V().has("city", "Beijing").out().has(T.label, P.neq("author"))the label predicate applies to the out() results and never to the g.V() candidates, yet the gate fires, prepareLocalHasContainers() leaves city local, and HugeGraphStep.makeQuery() builds new Query(VERTEX). An indexed lookup becomes a full vertex scan, with the same DEFAULT_CAPACITY ceiling noted above. The .out().where(__.not(__.hasLabel("author"))) variant behaves the same way through hasUnsafeLabelInChildren() line 884.
Requested change: bound the forward scan to steps that still operate on the source step's own elements, stopping at element-changing steps such as VertexStep/EdgeVertexStep/PropertiesStep, while keeping the walk alive for select()/path()-shaped steps that can reintroduce earlier elements. If that is follow-up work, please say in the FIXME above that a negative label anywhere in the traversal disables all pushdown at every source step, so the cost is recorded rather than implied to be narrow.
|
|
||
| ConditionQuery cq = (ConditionQuery) query; | ||
| if (cq.condition(HugeKeys.LABEL) != null && cq.resultType().isEdge()) { | ||
| if (cq.singleConditionValueOrNull(HugeKeys.LABEL) != null && |
There was a problem hiding this comment.
🧹 This is migrated, but its sibling gate below is the one production condition(HugeKeys.LABEL) left
Evidence: queryNeedsPostFilter() at line 2024 still reads
boolean edgeIndexWithLabel =
cq.resultType().isEdge() &&
cq.optimized() == OptimizedType.INDEX &&
cq.condition(HugeKeys.LABEL) != null;It is byte-identical at the merge base and outside every hunk of this PR, but it now disagrees with the branch you migrated here. For a sole LABEL IN [a, b], condition() returns the raw list (ConditionQuery.java:293-297), so edgeIndexWithLabel is true and the query is reported as needing no post-filter, while this branch correctly falls through to cq.test(elem). condition() can also throw IllegalStateException (ConditionQuery.java:305) from inside that caching decision where singleConditionValueOrNull() returns null.
No wrong data is served today: the production consumer is CachedGraphTransaction (lines 322, 399, 424), and what it caches was already filtered by super.queryEdgesFromBackend(). This is about the two gates reading the same way and about the exception surface.
Requested change: as a follow-up, since line 2024 is outside this diff, migrate it to singleConditionValueOrNull(HugeKeys.LABEL) != null, or leave a comment there recording that the legacy semantics are deliberate.
|
Measurement of head 1. Backend axis (hstore vs rocksdb, same server code). 174 suite cases plus 155 new shapes taken from this review (barrier-like steps, mixed-key 2. Version axis (rocksdb master vs rocksdb pr2994). Suite: 168 OK and exactly two cases where master throws and the PR answers (the same
Master pushes 3. Plan and cost axis (the two findings of @bitflicker64 from 2026-09-06). On top of the suite data I loaded 1 000 000 Point lookups (finding 1): at Gate breadth (finding 2), on 1 M vertices:
Row 1 is the deliberate trade-off the PR description records (a complete answer instead of a silently incomplete 8 ms one) and looks right to me. Rows 2 and 3 are a regression: the negative label applies to the In short: label semantics are correct and identical on both backends, the PR fixes a real class of silently incomplete answers, point lookups are preserved. The one thing I would close before merging is bounding the gate to steps that still operate on the source step's own elements, exactly in the spirit of finding 2. Happy to re-run the measurement on the next head, same script, same cluster. |
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: On the new local-filter fallback, hasKey() and hasValue() containers are left in the local HasStep untranslated, so combining them with a label predicate the new check treats as unsafe returns an empty result set with no error. Evidence: prepareLocalHasContainers skips them at TraversalUtil.java:716 while canExtractHasContainer still pushes them down on the normal path, and TinkerPop 3.5.1 HasContainer.test(Element) then evaluates them as a property literally named key or value, which HugeVertex.properties(String...) never resolves.
|
Re-measurement on Finding 2 of 2026-09-06 is closed. On 1 M vertices, Backend axis: suite 170 OK / 4 BOTH-ERR, 115/115 J8 shapes identical on hstore and rocksdb. Version axis (rocksdb): suite unchanged since 2026-09-06; the 18
A bare Two observations on the new code, not blocking since master throws in both cases: In short: from my side there is nothing left I would close before merging. At this head the label semantics are complete and identical on both backends, finding 2 is closed by measurement, and the PR fixes two classes of silently empty answers on master ( |
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The ConditionQuery decomposition is behaviour-preserving (collectConditionValues/resolveConditionValues are lifted verbatim and every legacy fast path is intact) and the LABEL call-site migration removes a real family of latent ClassCastExceptions, because Id label = cq.condition(HugeKeys.LABEL) compiles to a checkcast Id and constructEdgesQuery() (GraphTransaction.java:1379) builds exactly the sole Condition.in(HugeKeys.LABEL, ...) that trips it. The earlier open threads look addressed at this head: ~id now reaches the source step via GraphStep.processHasContainerIds() (TraversalUtil.java:702-711), hasKey()/hasValue() go through the new LocalContainsStep, HugeGraph.searchPredicate is a default method, the unsafe-label gate walks ancestors and short-circuits on changesCurrentElement, and queryNeedsPostFilter() line 2024 is migrated. Three notes inline: the local-filter fallback also drops the NoIndexException guard, localSearchPredicate() builds a P around a capturing lambda, and RamTable now declines multi-label edge queries it could still serve. Evidence: mvn -o -q -pl hugegraph-server/hugegraph-core -am compile -DskipTests -Dcheckstyle.skip=true -Djacoco.skip=true at fefe3ca gives BUILD SUCCESS; gh -R apache/hugegraph pr checks 2994 shows all 24 functional lanes green with only codecov/patch and codecov/project red (28.66% patch coverage, 234 uncovered lines); the rest is static tracing of the exact-head diff against merge base 3681148. I could not run the PR's regression suites here — hugegraph-test does not build in this environment because hg-store-client fails Lombok annotation processing (cannot find symbol: variable log), unrelated to this PR — so all three findings are static and the paging/full-scan behaviour below is unreproduced at runtime.
|
|
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The explicit condition-resolution API and the LABEL call-site migrations read correctly, and the negative-label gate now escapes at element-changing steps; the substantive point left is that the local-filtering path demotes positive hasLabel() containers along with the unsafe ones, turning a label-index lookup into a full scan, plus two nits on the new SEARCH and CONTAINS filters. Evidence: static read of the exact-head diff against merge base 3681148, and of TraversalUtil, HugeGraphStep, GraphIndexTransaction, RamTable, ConditionQueryFlatten and Condition at e32a75f.
| if (predicates.stream().anyMatch(p -> | ||
| p.getBiPredicate() == Condition.RelationType.TEXT_CONTAINS)) { | ||
| HugeGraph graph = tryGetGraph(source); | ||
| if (graph == null) { |
There was a problem hiding this comment.
🧹 The new doc promises analyzer matching that this branch does not give
docs/negative-label-queries.md states without qualification that local Text.contains() filters use the graph's SEARCH analyzer and exact term matcher. On this branch the container keeps Condition.RelationType.TEXT_CONTAINS, whose tester is ((String) v1).contains((String) v2) (Condition.java:94-97), so Text.contains("(alpha)") looks for the literal substring (alpha), parentheses included. The behaviour itself predates the PR (at the merge base canExtractHasContainer returned false on graph == null and left the same raw container behind), but the doc shipped here now describes the analyzed path as the rule.
Worth noting the gate is not what keeps the rewrite working: LocalSearchHasContainer takes no graph at construction (lines 914-916) and resolves one from property.element().graph() in testValue (lines 918-932), so installing it unconditionally would not reintroduce the getGraph() failure raised earlier in review.
Requested change: either drop the tryGetGraph gate so both cases match the doc, or qualify the doc's SEARCH paragraph and say here that matching degrades to substring on this path.
There was a problem hiding this comment.
Addressed in 2d53a55. Removed the graph-null gate: unbound child traversals now receive LocalSearchHasContainer too, and it resolves the analyzer from the runtime element as intended. Replaced the old keep-the-raw-predicate test with assertions that distinguish explicit-term SEARCH from substring matching. Tests cover graph/adjacency source shapes without a graph during extraction, mixed/nested predicates, serialization, cloning and graph rebinding, plus a core test using the real graph analyzer. Original predicate trees remain intact. The final targeted unit and Memory/RocksDB regressions passed; CI for the pushed commit is running.
|
Correction to my comment of 2026-09-08: the observation about
On the two notes of @bitflicker64 from 2026-09-08:
From my side unchanged: nothing left to close before merging. |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: One added unit regression does not run successfully with the repository's Mockito setup. Evidence: exact-head unit-test execution reproduced the failure; core Memory/Edge regressions passed.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the ConditionQuery decomposition is behaviour-preserving (collectConditionValues and resolveConditionValues are lifted verbatim, every legacy fast path in condition() survives) and it removes a real family of latent ClassCastExceptions, for example BinarySerializer.java:677, where Id label = cq.condition(HugeKeys.LABEL) met a sole IN relation and got the raw List back. The four items below are maintainability, not correctness. Evidence: static review of the full diff at 2d53a556 against merge-base 36811483; gh api repos/apache/hugegraph/commits/2d53a556.../check-runs reports 22 green check runs at this head, with only codecov/patch and codecov/project red. Nothing was compiled or executed locally, so this is a static pass, and the open threads on this head (the TraversalUtilOptimizeTest.java:721 Mockito failure and the SEARCH predicates doc section) are acknowledged rather than re-litigated.
| /** | ||
| * Returns whether there is any top-level relation for the specified key. | ||
| */ | ||
| public boolean containsCondition(Object key) { |
There was a problem hiding this comment.
🧹 This overload is public but has one caller in the tree, and it widens an overload set whose members disagree about what the argument means.
git grep 'containsCondition(' at this head finds a single call to it: line 407, inside containsCondition(HugeKeys). The set now reads:
containsCondition(Object key)here andcontainsCondition(HugeKeys key)at line 406: is there a top-level relation on this keycontainsCondition(Condition.RelationType type)at line 460: is there a top-level relation using this operator
Both pre-existing overloads take enums, so no other argument type compiled. With Object in the set, any reference type does and silently resolves to key matching, including Condition.Relation.key(), which is declared Object (Condition.java:739).
Requested change: make this method private. If a public key accessor is wanted for Id userprop keys, give it a distinct name such as containsConditionKey(Object), matching the containsConditionValues naming you introduced, rather than another overload. containsConditionValues(Object) at line 350 has the same single-delegate-caller shape but no competing overload, so it is only extra surface.
There was a problem hiding this comment.
Addressed in 5a7048f: containsCondition(Object) is now private. The HugeKeys and RelationType overloads remain public and retain key-based/operator-based behavior respectively. Added a regression test for visibility and both public overloads; it failed before the change and passes after it. All 43 targeted query/serializer/transaction tests and the full-module clean compile passed on the SSH test host. No renaming or broader API change is included.
| return false; | ||
| } | ||
|
|
||
| private Set<Object> resolveConditionValues(List<Object> valuesEQ, |
There was a problem hiding this comment.
🧹 A second ConditionQuery in this repo keeps the legacy semantics, and its copy of this loop drops conflicts.
hugegraph-struct/src/main/java/org/apache/hugegraph/query/ConditionQuery.java carries a near-duplicate condition(Object) (lines 463-524 at this head). It is the class the store side deserializes: hg-store-common/.../StoreQueryParam.java, hg-store-core/.../business/FilterIterator.java (ConditionQuery.fromBytes, line 49) and hg-store-node/.../grpc/query/stages/FilterStage.java all import org.apache.hugegraph.query.ConditionQuery.
Its intersection loop (lines 494-513) gates on intersectValues.isEmpty() instead of the initialized flag this method uses, so an emptied set is read as "not seeded yet". For three conflicting relations on one key, EQ a, EQ b, EQ c, the walk is {a}, then intersect [b] gives {}, then the third iteration takes the seed branch and re-seeds {c}; it returns c where this file returns null. Two conflicting relations happen to coincide, which is why it is easy to miss. The struct copy also has none of the explicit accessors you added here.
Nothing is broken today: git grep '\.condition(' over hugegraph-struct and hugegraph-store finds only Condition.java:770 and ConditionQuery.java:440, so there is no production caller. This PR is scoped to server, so I am not asking you to widen it.
Requested change: open a follow-up to port collectConditionValues/resolveConditionValues and the explicit accessors to the struct copy, and add a short comment on each copy naming the other so the two do not drift further.
There was a problem hiding this comment.
Tracked separately in #3200. It covers the empty-intersection reseeding bug, explicit accessor parity, compatibility boundaries, and regression fixtures for both implementations. Commit 5a7048f adds a short cross-reference and the issue number on each copy. The struct implementation itself is intentionally unchanged in this PR.
| // Scan the remaining traversal, its children and each ancestor's | ||
| // remaining steps for filters on child output. Arbitrary extension | ||
| // steps don't reliably expose element identity, so stay conservative. | ||
| // FIXME: Restore selective pushdown when every candidate schema label |
There was a problem hiding this comment.
🧹 This FIXME marks the fallback as provisional but gives a reader no issue to follow.
It is the one tracked exit from the conservative behaviour the rest of the PR builds on: while it stands, a downstream unsafe label predicate disables property pushdown across element changes, ancestors and unknown extension steps, which is exactly the full-scan cost docs/negative-label-queries.md warns about. You already split the convContains2Relation caching out to #3196, so the convention is established here.
Requested change: file an issue for restoring selective pushdown once every candidate schema label can be shown to have compatible index coverage, and put its number on this line, // FIXME(#NNNN): ..., so the fallback has a tracked path back.
There was a problem hiding this comment.
Opened #3201 for selective property pushdown with complete, predicate-compatible coverage of all candidate labels. Commit 5a7048f links it as FIXME(#3201). The issue includes traversal context/identity, schema lifecycle, incomplete-coverage counterexamples, paging/count/side-effect correctness, and RocksDB/HStore performance validation. This revision does not change the conservative fallback.
| @@ -0,0 +1,53 @@ | |||
| # Negative-label queries and local filtering | |||
There was a problem hiding this comment.
🧹 Two fork URLs in the PR description point at this content and will break on merge.
Description line 98 links github.com/contrueCT/hugegraph/blob/e32a75ff.../docs/negative-label-queries.md, and line 37 embeds raw.githubusercontent.com/contrueCT/hugegraph/b3f5642.../docs/images/pr-2994-condition-resolution.png. Both die once that branch is deleted, and they are what a reader following the merged commit lands on. Requested change: point the link at the in-repo path, and either commit the diagram under docs/images/ or drop it.
Separately, docs/ at this head is BUILDING.md, CONTRIBUTING.md and this file, so it is a contributor-docs folder; HugeGraph user documentation is published from apache/hugegraph-doc. What this file describes is user-visible: g.V().has("unindexedProp", "x") still raises NoIndexException while the same query followed by hasLabel(P.neq("author")) now scans candidates, and the paging note changes what a correct client has to do. Once the SEARCH predicates section is settled on the other open thread, a follow-up in hugegraph-doc would put this in front of the users who hit it, which is what the checked Doc - Done box implies for an API-affecting change.
There was a problem hiding this comment.
Removed both contributor-fork URLs from the PR description. The existing diagram and the query-behavior note now live in this standalone gist: https://gist.github.com/contrueCT/1e44ef501d0db6d82dfe2b95849770a3 . The PR embeds its image/png raw URL, verified accessible and byte-identical to the original image; the note is identical to the repository document. The diagram caption clarifies the eligible positive-label exception. I also changed Doc - Done to Doc - TODO and explicitly noted that publication to the HugeGraph documentation website is still pending. The SHA-pinned fork URLs are currently accessible, so deletion of a branch was not itself proof of immediate breakage; the fork dependency is now removed regardless.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the LABEL migration holds at 5a7048fd. queryIndex() calls query.checkFlattened() before any single-label routing (GraphIndexTransaction.java:386) and IN is in UNFLATTEN_TYPES (Condition.java:143-144), while ConditionQueryFlatten.convIn2Or() preserves IN only for OWNER_VERTEX/ID (:152-158), so no multi-value LABEL can reach conditionValue() or singleConditionValueOrNull() on the RamTable, serializer or index paths, including on backends with supportsQueryWithInCondition(). git grep 'condition(HugeKeys.LABEL)' -- '*/src/main/*' ':(exclude)*hugegraph-test*' returns no hits at this head, and the queryNeedsPostFilter() holdout is migrated at GraphTransaction.java:2024. I also checked that leaving a HasStep in place is what keeps extractRange() (:1288-1290), extractCount() (:1303) and HugeCountStepStrategy from stepping over the new local filters. Two comments below, neither blocking. Evidence: static review of the exact-head diff (23 files, +2789/-80) against merge-base 36811483; gh api repos/apache/hugegraph/commits/5a7048fd.../check-runs is green on every workflow, with codecov/patch (23.38%) and codecov/project the only red checks. No local build or test run.
| public static void extractHasContainer(HugeVertexStep<?> newStep, | ||
| Traversal.Admin<?, ?> traversal) { | ||
| if (hasUnsafeLabelInTraversal(traversal, newStep)) { | ||
| prepareLocalHasContainers(newStep, traversal); |
There was a problem hiding this comment.
Evidence:
- It is reached for
g.V(v).outE().has('~page','').hasLabel(P.neq('knows')).limit(10): the foldedHasStepfailsisEqInLabelPredicate, sohasUnsafeLabelInTraversalreturns true. prepareLocalHasContainers()sends~pagethroughquery.addHasContainer(has)(line 701), whichHugeVertexStep.addHasContainer()turns intosetPage()and returns (HugeVertexStep.java:213-217). The label takes theelseat line 729 becausesource instanceof HugeGraphStepis false, soHugeVertexStep.hasContainersstays empty.withEdgeCondition()andwithVertexCondition()are!this.hasContainers.isEmpty()(HugeVertexStep.java:185-191), soE.checkArgument(!this.queryInfo().paging(), "Can't query by paging and filtering")(:173-176) cannot fire on this path. At merge-base36811483it did fire:canExtractHasContainer()returned true for any sysprop key (:658-660there), so both~pageand~labelwere pushed into the step and the traversal was rejected outright.- The short-page contract is deliberate and documented (
docs/negative-label-queries.md:41-45, plus the range bound at lines 760-765), but the only regression for it isVertexCoreTest#testPageBeforeDownstreamNegativeLabel. The five newEdgeCoreTest#testQueryEdgesByNonEqLabel*cases cover barrier, range, mixed-keyorandsideEffect, none with~page.
Requested change: add an edge-side paging regression next to testPageBeforeDownstreamNegativeLabel that pages g.V(v).outE().has('~page', cursor) with a downstream negative label to cursor exhaustion and asserts no missing or duplicate edge ids, so the shape that used to be rejected is pinned.
There was a problem hiding this comment.
Addressed in 459a2b2. Added two outgoing-edge paging regressions for the negative label before and after limit(). Both traverse to cursor exhaustion, reject duplicate edge IDs and repeated cursors, assert the exact expected IDs, and explicitly require an empty filtered page with a continuation cursor. On the SSH test host, all 14 targeted RocksDB edge/vertex paging and negative-label tests passed with no skips; editorconfig formatting and full-module clean compile also passed. No optimizer behavior was changed. CI for the pushed commit is running.
|
|
||
| ## SEARCH predicates | ||
|
|
||
| Local `Text.contains()` filters use the graph's SEARCH analyzer and exact term |
There was a problem hiding this comment.
🧹 This sentence never says which Text.contains() predicates count as local, and one ordinary position is not covered. The unbound-child cause raised on the earlier head is fixed in 2d53a556; this is a different position.
Evidence:
prepareLocalHasContainers()walks onlywhile (step instanceof HasStep || step instanceof NoOpBarrierStep)(TraversalUtil.java:694) and installsLocalSearchHasContaineronly for containers found in that walk (:742-751).- A
Text.contains()sitting after aRangeGlobalSteporOrderGlobalStep, for exampleg.V().hasLabel(P.neq('author')).limit(10).has('body', Text.contains('(alpha)')), is never reached by that walk and keeps the rawCondition.RelationType.TEXT_CONTAINSpredicate, which is a plain substring test with no(word)handling:return v1 != null && ((String) v1).contains((String) v2);(Condition.java:94-97). - This PR's own test pins the difference:
Assert.assertFalse(Text.contains("(alpha)").test("alpha"))(TraversalUtilOptimizeTest.java:534).
Requested change: name the scope, for example "Text.contains() predicates in the filter chain directly following the source step use the graph's SEARCH analyzer and exact term matcher, including explicit (word) and (word1|word2) expressions; one placed after range() or order() keeps plain substring semantics."
There was a problem hiding this comment.
Addressed in 459a2b2. The SEARCH section now limits analyzer-based matching to the filter chain directly following the source, including barriers, and states that range/limit/order boundaries stop this adaptation. It includes the reported post-limit example and explains its literal substring semantics. The same clarification is included in the PR description and standalone gist; no SEARCH runtime behavior was changed.
Purpose of the PR
ConditionQuery.condition()historically combines several meanings in one API:INrelationThis PR preserves that legacy behavior, adds explicit condition-resolution APIs, and
migrates the high-risk
LABELcall sites to semantics that match each caller.Visual overview
The diagram contrasts strict and tolerant single-value resolution and shows why
negative or ambiguous label predicates stay local to preserve complete results.
Eligible positive EQ/IN label predicates can still use label indexes; the diagram
is a summary, not an exhaustive query-plan description.
Main Changes
Make condition resolution explicit
containsCondition(HugeKeys key)reports any top-level relation for the system key.Its
Objectimplementation is private; the existing operator-based overload remains public.containsConditionValues(key)reports whether a top-levelEQ/INrelation exists,including an empty
INrelation.conditionValues(key)returns the resolvedEQ/INintersection. Pair it withcontainsConditionValues(key)when absence and an empty intersection must differ.conditionValue(key)returnsnullfor an empty result, returns the value for asingleton, and rejects a multi-value result.
singleConditionValueOrNull(key)returns the value only for a singleton and returnsnullfor both empty and multi-value results.condition(key)remains backward-compatible, including returning the raw list for asole
INrelation.Migrate label-sensitive callers
conditionValue()semantics where serializers and sort-key paths requireone resolved label.
singleConditionValueOrNull()where an optimization is valid only for exactly oneresolved label.
conflicting-label queries.
flatten them into per-label queries and reject empty or unsupported conditions.
Preserve correctness for negative-label predicates
A downstream unsafe label predicate, including one after a range, side effect, or
inside a child traversal, keeps candidate-filtering predicates local. This avoids
losing matches from labels without equivalent index coverage.
The fallback handles query controls and SEARCH predicates separately:
~pageis consumed as query metadata. The backend page is bounded while localfilters and range steps keep their order. A filtered page can be empty while its
cursor still points to more data; callers must follow the cursor to exhaustion.
Text.contains()in the filter chain directly following the source(including barriers, but before range/limit/order boundaries) uses the same analyzer
and term matcher as SEARCH indexes,
including
(word),(word1|word2), and analyzed text. For example, searchingbody = "alpha"withText.contains("(alpha)")still matches when a negative labelfollows
limit()while the SEARCH predicate precedes it. AText.contains()afterrange/limit/order keeps plain substring semantics. The adapted local container
preserves the original predicate tree;
its graph-specific runtime matcher is transient and rebuilt after cloning,
deserialization, predicate changes, or graph rebinding.
HugeGraph.searchPredicate(text)creates this matcher without exposing graphconfiguration; the authorization proxy verifies graph access before delegation.
This fallback intentionally changes missing-index behavior: a defined but unindexed
property query such as
g.V().has("unindexedProp", "x").hasLabel(P.neq("author"))can scan candidates and filter locally instead of raising
NoIndexException.This preserves complete results, but may increase latency and backend work. Existing
capacity checks are not a universal scan-work bound, and a final limit bounds matches,
not all examined candidates. Explicit-ID and adjacency queries can retain narrower
candidate sources.
See negative-label query behavior and limits
for the user-facing contract, capacity exceptions, and paging guidance. The image and
note are hosted in gist and do not depend on the contributor's fork.
Follow-ups are tracked separately:
ConditionQuerywith the server semantics.coverage for every candidate schema label.
Verifying these changes
Regression coverage includes:
INrelation and non-EQ/INlabel predicatesconnectives or multiple label containers
filtered pages without missing or duplicate IDs
non-admin access to the SEARCH matcher
NoIndexException, candidate-capacity enforcement,and explicit-ID lookups
clone isolation, predicate mutation, and graph rebinding
Targeted verification (run on the SSH test host; no coverage/style skips):
Does this PR potentially affect the following parts?
The public Java API of
ConditionQuerygains explicit resolution methods, andHugeGraph.searchPredicate(text)exposes the existing SEARCH matching semantics forlocal traversal filters. No REST API, configuration, or dependency changes are included.
Documentation Status
Doc - TODODoc - DoneDoc - No NeedThe API semantics are documented in Javadocs. The linked user note documents the
full-scan fallback, missing-index behavior, capacity limits, paging, and local SEARCH.
Publication to the HugeGraph documentation website is still pending; the gist and
repository note do not imply that the website has been updated.