Skip to content

fix(server): clarify condition resolution semantics for label queries - #2994

Open
contrueCT wants to merge 55 commits into
apache:masterfrom
contrueCT:task/improve-condition-query-semantics
Open

contrueCT wants to merge 55 commits into
apache:masterfrom
contrueCT:task/improve-condition-query-semantics

Conversation

@contrueCT

@contrueCT contrueCT commented Apr 13, 2026 •

Copy link
Copy Markdown
Contributor

Purpose of the PR

ConditionQuery.condition() historically combines several meanings in one API:

  • no matching condition
  • EQ/IN conditions whose intersection is empty
  • one resolved value
  • the raw list from a sole IN relation
  • an exception when several relations still resolve to multiple values

This PR preserves that legacy behavior, adds explicit condition-resolution APIs, and
migrates the high-risk LABEL call sites to semantics that match each caller.

Visual overview

Condition resolution semantics and label-query optimization for PR #2994

The diagram contrasts strict and tolerant single-value resolution and shows why
negative or ambiguous label predicates use the local-filtering fallback described below.
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 Object implementation is private; the existing operator-based overload remains public.
  • containsConditionValues(key) reports whether a top-level EQ/IN relation exists,
    including an empty IN relation.
  • conditionValues(key) returns the resolved EQ/IN intersection. Pair it with
    containsConditionValues(key) when absence and an empty intersection must differ.
  • conditionValue(key) returns null for an empty result, returns the value for a
    singleton, and rejects a multi-value result.
  • singleConditionValueOrNull(key) returns the value only for a singleton and returns
    null for both empty and multi-value results.
  • condition(key) remains backward-compatible, including returning the raw list for a
    sole IN relation.

Migrate label-sensitive callers

  • Use strict conditionValue() semantics where serializers and sort-key paths require
    one resolved label.
  • Use singleConditionValueOrNull() where an optimization is valid only for exactly one
    resolved label.
  • Resolve label intersections when collecting matched indexes, including multi-label and
    conflicting-label queries.
  • Apply the same semantics to graph/index transactions, traversers, and HStore.
  • Keep RamTable's multi-label adjacency fast path for nonempty valid label candidates;
    flatten them into per-label queries and reject empty or unsupported conditions.

Preserve correctness for negative-label predicates

Unsafe label predicates and their sibling filters stay local where needed to
avoid losing candidates through partial index coverage. The fallback also handles
barriers and child/ancestor traversal contexts.

To bound this PR's scope, vertex adjacency steps (out()/in()/both() and
outE()/inE()/bothE()) followed by the supported suffix retain the source
property query's existing index plan and missing-index errors. This applies to
root and child traversals. Self-loops and later hops can return to the source;
this boundary preserves existing behavior and is not a proof of different
element identity. Current-element suffixes such as dedup, order, valueMap,
elementMap, fold, groupCount, and project retain that plan when their
by() children cannot recover earlier elements. Explicit edge-endpoint paths
and suffixes containing select/path/repeat or unknown extensions remain conservative.

General candidate completeness for source property queries across labels with
unequal index coverage is outside this PR. In particular, a label-unconstrained
has("city", "Beijing") can still omit a label without a city index; a later
negative label after ordinary adjacency does not repair that source query.
#3201 tracks complete candidate coverage and selective property pushdown.
Within the retained fallback, an indexed lookup or NoIndexException can
still become local filtering of scanned candidates.

The fallback handles query controls and SEARCH predicates separately:

  • ~page is consumed as query metadata. The backend page is bounded while local
    filters 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.
  • In this fallback, 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, searching
    body = "alpha" with Text.contains("(alpha)") still matches when a negative label
    follows limit() while the SEARCH predicate precedes it. A Text.contains() after
    range/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 graph
    configuration; 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:

Verifying these changes

Regression coverage includes:

  • absent, empty, singleton, conflicting, and multi-value condition resolution
  • a sole raw IN relation and non-EQ/IN label predicates
  • single-label and multi-label edge sort-key queries
  • negative-label queries next to indexed properties, across barriers, and with mixed
    connectives or multiple label containers; preservation of source query behavior
    across self-loops and adjacency paths, including missing-index errors
  • matched-index collection for joint labels with indexed properties
  • vertex and outgoing-edge paging with negative labels, including continuation through empty
    filtered pages without missing or duplicate IDs
  • SEARCH positive controls, explicit terms, analyzed text, and matching unindexed labels
  • real offset/limit ordering, aggregate contents, and indexed interference labels
  • singleton/duplicate IN compatibility, serializer and RamTable contracts, and
    non-admin access to the SEARCH matcher
  • unindexed-property fallback versus NoIndexException, candidate-capacity enforcement,
    and explicit-ID lookups
  • SEARCH predicate structure/hash stability, Java serialization before and after use,
    clone isolation, predicate mutation, and graph rebinding
  • RamTable multi-label IN/OUT/BOTH results, duplicate labels, and unsafe candidate rejection

Targeted verification (run on the SSH test host; no coverage/style skips):

mvn test -pl hugegraph-server/hugegraph-test -am \
  -P unit-test,memory -Dsurefire.failIfNoSpecifiedTests=false \
  -Dtest='TraversalUtilOptimizeTest,QueryTest,BinarySerializerTest,TextSerializerTest,GraphTransactionTest,CachedGraphTransactionTest,HugeGraphAuthProxyTest#testSearchPredicateDoesNotRequireAdminConfigAccess,org.apache.hugegraph.unit.cache.RamTableTest#testMatchedLabelCandidateContract+testQueryByMultipleLabels+testMatchedRejectsUnsafeLabelCandidates'

# Repeat with core-test,rocksdb for a persistent backend.
mvn test -pl hugegraph-server/hugegraph-test -am \
  -P core-test,memory -Dsurefire.failIfNoSpecifiedTests=false \
  -Dtest='VertexCoreTest#testCollectMatchedIndexesByJointLabelsWithIndexedProperties+testQueryByUserpropWithConflictingLabels+testUnindexedPropertyBeforeNegativeLabel+testLocalConnectiveStringIds+testLocalContainsBeforeNegativeLabel+testSearchBeforeDownstreamNegativeLabel+testPageBeforeDownstreamNegativeLabel+testNegativeLabelPreservesRangeAndAggregate,EdgeCoreTest#testQueryEdgesByNonEqLabel*+testQueryOutEdgesByMultiLabelsAndSortKey+testPageOutEdgesWithNegativeLabel*'

# Full vertex/edge regression classes (repeat with core-test,rocksdb).
mvn test -pl hugegraph-server/hugegraph-test -am \
  -P core-test,memory -Dsurefire.failIfNoSpecifiedTests=false \
  -Dtest=VertexCoreTest,EdgeCoreTest

mvn editorconfig:format
mvn clean compile -Dmaven.javadoc.skip=true

At 2cbd5d71 on the SSH test host, TraversalUtilOptimizeTest passed 48/48.
Complete VertexCoreTest + EdgeCoreTest passed on Memory (479 run, 74 skipped)
and RocksDB (479 run, 29 skipped), with zero failures/errors. The Java 17
all-module clean compile passed (38 modules). In a disposable integration tree combining
this head with #3243 0de33eda, targeted source/adjacency cases passed on Memory
(21 run, 2 skipped) and RocksDB (21 run, 0 skipped), and the optimizer suite
passed 48/48; there were zero failures/errors. HStore and full TinkerPop suites
were not rerun at these combined heads.

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as shown above.

Does this PR potentially affect the following parts?

The public Java API of ConditionQuery gains explicit resolution methods, and
HugeGraph.searchPredicate(text) exposes the existing SEARCH matching semantics for
local traversal filters. No REST API, configuration, or dependency changes are included.

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

The API semantics are documented in Javadocs. The linked user note documents the
full-scan fallback, missing-index behavior, capacity limits, paging, and local SEARCH.
The linked Gist is the sole copy of this PR's explanatory note; it is not included
in the repository. Publication to the HugeGraph documentation website is still
pending; the Gist does not imply that the website has been updated.

@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 13, 2026
@contrueCT contrueCT changed the title improve(query): clarify condition resolution semantics for label queries fix(query): clarify condition resolution semantics for label queries Apr 19, 2026
@codecov

codecov Bot commented May 6, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.24038% with 311 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.29%. Comparing base (83ef9f3) to head (2cbd5d7).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
...he/hugegraph/traversal/optimize/TraversalUtil.java 14.95% 238 Missing and 18 partials ⚠️
...apache/hugegraph/backend/query/ConditionQuery.java 73.77% 7 Missing and 9 partials ⚠️
...he/hugegraph/backend/tx/GraphIndexTransaction.java 23.80% 12 Missing and 4 partials ⚠️
...g/apache/hugegraph/backend/store/ram/RamTable.java 0.00% 13 Missing ⚠️
...he/hugegraph/backend/store/hstore/HstoreStore.java 0.00% 3 Missing ⚠️
.../org/apache/hugegraph/auth/HugeGraphAuthProxy.java 0.00% 2 Missing ⚠️
.../src/main/java/org/apache/hugegraph/HugeGraph.java 0.00% 1 Missing ⚠️
...n/java/org/apache/hugegraph/StandardHugeGraph.java 0.00% 1 Missing ⚠️
...hugegraph/backend/serializer/BinarySerializer.java 50.00% 1 Missing ⚠️
...e/hugegraph/backend/serializer/TextSerializer.java 50.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2994      +/-   ##
============================================
- Coverage     41.35%   41.29%   -0.06%     
- Complexity     7299     7371      +72     
============================================
  Files           802      802              
  Lines         69688    70140     +452     
  Branches       9291     9419     +128     
============================================
+ Hits          28816    28964     +148     
- Misses        37576    37859     +283     
- Partials       3296     3317      +21     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@contrueCT
contrueCT force-pushed the task/improve-condition-query-semantics branch 2 times, most recently from 4c42786 to cc9af24 Compare May 26, 2026 12:29

@imbajin imbajin left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

@contrueCT
contrueCT force-pushed the task/improve-condition-query-semantics branch from cc9af24 to 2e82f83 Compare May 30, 2026 10:20

@imbajin imbajin left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

contrueCT added 5 commits June 5, 2026 12:59
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.
@contrueCT
contrueCT force-pushed the task/improve-condition-query-semantics branch from 94408b7 to b10e3c2 Compare June 5, 2026 05:10
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jun 5, 2026
@contrueCT
contrueCT force-pushed the task/improve-condition-query-semantics branch from 801923a to ebc31c8 Compare June 5, 2026 18:06
@contrueCT

contrueCT commented Jun 5, 2026 •

Copy link
Copy Markdown
Contributor Author

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 imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: HStore range-index offset queries can skip too many sorted results. Evidence: static review of GraphIndexTransaction/query offset handling.

@contrueCT

Copy link
Copy Markdown
Contributor Author

Thanks. I fixed this by resetting scanQuery.offset(0L) before the full sorted range-index scan, so the fallback now reads the complete matched range first and lets the original query apply offset/limit only once after sorting. I also added range-offset coverage to VertexCoreTest#testQueryByDateProperty to guard the double-skip case. Local checks passed with git diff --check, hugegraph-core compile, and VertexCoreTest#testQueryByDateProperty under the rocksdb core-test profile.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs/negative-label-queries.md Outdated

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: The LABEL resolution migration itself looks sound at this head (serializers, GraphTransaction, GraphIndexTransaction, HstoreStore and RamTable all now distinguish absent / empty / single / multi-value label conditions, and CI is green apart from codecov), but the new negative-label fallback in TraversalUtil suppresses property pushdown far more broadly than the stated goal, and two label-resolution call sites keep fallbacks that no longer match the new API's semantics. Evidence: reviewed the exact-head diff git diff 36811483a2040f70ca6923a288a8c28cad0086c0 459a2b2f45e7e8c6a13fa533f76f7343bc57ae97 in the local checkout (23 files, +2860/-80); traced ConditionQueryFlatten.flatten/isFlattened to confirm LABEL IN is always expanded before RamTable.query and GraphIndexTransaction.queryIndex; confirmed no condition(HugeKeys.LABEL) call sites remain outside tests (git grep -n "condition(HugeKeys.LABEL)" 459a2b2 -- '*/main/java/*'); checked TinkerPop 3.5.1 HasContainer sources (clone()/testingIdString) against the new Local*HasContainer subclasses; gh -R apache/hugegraph pr checks 2994 shows only codecov/project and codecov/patch failing.

@SebastianGruza

Copy link
Copy Markdown
Contributor

#3184 is on master as 1a15e762. This head (459a2b2f) merges onto it without conflicts, so while the rebase is pending I am running the HStore/RocksDB regression matrix (same queries on both backends compared as id sets, the 170-case suite plus the 115 J8 shapes) on master 1a15e762 + this head, with plain 1a15e762 as the baseline, and will post the result here; once the branch is rebased I can repeat it on the actual head.

@SebastianGruza

Copy link
Copy Markdown
Contributor

Results for this head on a master that carries #3184, as asked in #3184 (comment). The branch is not rebased yet, so the tree under test is a local merge: 1a15e762 + 459a2b2f (merge a4c024d4, no conflicts); baseline is plain 1a15e762. Same lab and method as the earlier reports (PD + 3 stores, hstore server vs rocksdb oracle, results compared as id sets, full wipe between sides). Report with every file: https://github.com/SebastianGruza/hugegraph-validation/blob/master/reports/pr-2994/459a2b2f/README.md

Backend axis (hstore vs rocksdb, same build)

build suite (174 cases) hg_j8 (207 cases)
1a15e762 OK=168, MISMATCH=0, TARGET-ERR=0, BOTH-ERR=6 OK=182, MISMATCH=0, TARGET-ERR=0, BOTH-ERR=25
1a15e762 + this head OK=170, MISMATCH=0, TARGET-ERR=0, BOTH-ERR=4 OK=190, MISMATCH=6, TARGET-ERR=6, BOTH-ERR=5

The 37 store-side decode errors that section S used to produce on master are gone on plain 1a15e762 (#3184 confirmed on a fresh cluster). The PR turns two without() suite shapes and 21 J8 shapes from errors into results, and the 60 hasLabel(neq('person')) J8 shapes from 0 rows to the right rows, identically on both backends; hasKey('nope').hasLabel(neq(...)) becomes the documented capacity fallback error. All as in the 2d53a55 report.

New on this head, hstore only: paged range-index queries from Gremlin fail with a sandbox error. Six G PAGE positive controls (g.V().has('score',gte(40)) without a label and hasLabel('robot').has('age',gte(60)), page sizes 7 / 50 / 500 through ~page) fail on hstore with the PR and pass on rocksdb with the PR, on master hstore warm, and on master hstore fresh with J8 as the first traffic (0 sandbox warnings in that log). A second run on the warm PR server reproduces the same six. The server answers 500 Not allowed to access thread group via Gremlin; stack:

HugeSecurityManager.checkAccess(HugeSecurityManager.java:169)
java.lang.Thread.<init>
org.apache.hugegraph.store.client.util.ExecutorPool$DefaultThreadFactory.newThread(ExecutorPool.java:64)
ThreadPoolExecutor.execute / ExecutorCompletionService.submit
org.apache.hugegraph.store.client.OrderedKvIterator.initialize(OrderedKvIterator.java:200)
org.apache.hugegraph.store.client.OrderedKvIterator.hasNext(OrderedKvIterator.java:93)
HstoreSessionsImpl$ColumnIterator.<init>(HstoreSessionsImpl.java:245)
HstoreSessionsImpl$HstoreSession.scanOrdered(HstoreSessionsImpl.java:745)
HstoreTable.queryByRange(HstoreTable.java:643) <- queryBy <- query <- HstoreStore.query

With the PR these queries reach HstoreSession.scanOrdered(), whose OrderedKvIterator creates the store client's ExecutorPool worker threads lazily on first use, and HugeSecurityManager.checkAccess(ThreadGroup) forbids thread creation from a Gremlin thread; the whitelist there (callFromCaffeine, callFromAsyncTasks, callFromEventHubNotify, callFromBackendHbase, callFromRaft, callFromSofaRpc) has no entry for org.apache.hugegraph.store.client. On master the same shapes do not take the ordered scan from a Gremlin thread, so the pool is never created under the sandbox. Two fixes come to mind, both outside this PR's diff: start the ExecutorPool eagerly when the store client is created, or whitelist the store client package the way raft and sofa-rpc are. @contrueCT I can retest the moment the branch is rebased, and if you prefer the whitelist route I can send that as a small PR on master so it does not block this one.

@contrueCT

Copy link
Copy Markdown
Contributor Author

Follow-up to the ordered-scan sandbox failure: I synced this PR with master 1a15e762 (current head bcb8c1f3) and reproduced Not allowed to access thread group via Gremlin with the real Gremlin engine, security manager, and ordered iterator using controlled scan sources.

The independent fix is #3213, based directly on master. It falls back to caller-thread initialization when worker submission is denied, including after idle workers expire, without relaxing the sandbox. Its description includes a flow diagram and validation details: 15 security tests, 20 unique iterator tests, formatting, and all-module clean compilation passed on Java 11.

Proposed integration flow: merge #3213 into master → sync this PR with master again → rerun the six reported paged range-index queries and the HStore/RocksDB validation matrix. The complete PD + three-store HTTP matrix has not been rerun here; the isolated regression tests should not be read as that acceptance result. This PR's LABEL changes remain separate from the sandbox fix.

@contrueCT

Copy link
Copy Markdown
Contributor Author

Tracking issue for the independent HStore ordered-scan sandbox failure: #3214. Fix PR #3213 now links it with a closing reference. The issue records the reproduction, validation boundary, and merge → sync → distributed-query retest workflow.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: this head is a pure master sync — git diff 36811483 0cebc418 and git diff 1a15e762 bcb8c1f3 are identical apart from blob hashes and one EdgeCoreTest hunk offset, so no PR-authored code changed since 0cebc418, every production condition(HugeKeys.LABEL) call site is still migrated, and latest-head CI is green except the two informational codecov checks. My one substantive point is about the rebase that is still pending: master's #3193 rewrote the two GraphTransaction methods this PR migrates, so resolving those conflicts in master's favour would silently revert two of the migrations. Evidence: git grep -n "condition(HugeKeys.LABEL)" bcb8c1f3 -- '*/src/main/java/*' (production-clean); git merge-tree --write-tree bcb8c1f3 origin/master (three conflict hunks in GraphTransaction.java, two on migrated lines); git grep -n "condition(HugeKeys.LABEL)" origin/master (legacy accessor back at GraphTransaction.java:1086 and :1930); gh pr checks 2994 (22 pass, codecov/patch + codecov/project fail).

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The a07f6aae merge dropped the empty-label-intersection guard from GraphIndexTransaction.collectMatchedIndexes() while keeping the regression test that asserts it, so this PR's own VertexCoreTest#testCollectMatchedIndexesByJointLabelsWithIndexedProperties fails on memory, rocksdb and hstore; separately, the new local-filter fallback silently replaces NoIndexException with a candidate scan for a broad class of traversals. Evidence: git diff 83ef9f3fa23cd65284d4567f7aa734cf90aa0749 a07f6aae5a71bdceb11fc71f1d739c415c9b9411; git show bcb8c1f3:.../GraphIndexTransaction.java | sed -n '771,785p' vs the same range at a07f6aae shows the removed if (hasLabelValues && labels.isEmpty()) return Collections.emptySet();; gh api repos/apache/hugegraph/commits/a07f6aae.../check-runs reports failures for build-server (memory, 11), build-server (rocksdb, 11), hstore and both build-server-macos-rocksdb jobs, and jobs 106872307016 / 106872306860 / 106844898811 all log VertexCoreTest.testCollectMatchedIndexesByJointLabelsWithIndexedProperties:9756 expected:<0> but was:<1>.

Comment thread docs/negative-label-queries.md Outdated

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The empty-label guard is back in collectMatchedIndexes() and the testCollectMatchedIndexesByJointLabelsWithIndexedProperties failure from a07f6aa is gone. The child-traversal narrowing in a480537 now fails this PR's own VertexCoreTest#testPositiveLabelBeforeUnsafeChildUsesLabelQuery on every server backend. It also treats outE().outV() as leaving the source vertex, but that step pair returns to it. Evidence: check runs at a480537 fail build-server (memory, rocksdb, hbase), both build-server-macos-rocksdb jobs and hstore, each at VertexCoreTest.java:10050 expected:<1> but was:<2>. I reproduced that failure locally on the memory backend with JDK 11. A throwaway memory-backend probe at a480537 showed the outE().outV() result gap.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: Treating every VertexStep as a move away from the source still lets out().in(), both().both() and a self-loop out() bring the source vertex back to a negative label filter after its property was pushed down, so those queries drop matches from labels without the property index. Evidence: memory-backend probe at 559b220 (JDK 11, VertexCoreTest fixture with city indexed on person only) returned [] for g.V().has("city","Beijing").out().in().hasLabel(P.neq("person")), the where(__.out().in()...) form, both().both(), and a self-loop out(), where each should return the Beijing fan; outE().outV() on the same data returned the fan.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: 03a0c67 drops VertexStep from changesCurrentElement(), so any negative label after out(), in(), both(), outE() or inE() now disables property pushdown at the source step. Common shapes such as g.V().has("city","x").out().hasLabel(P.neq("y")) switch from an index lookup to a full vertex scan, unindexed properties stop raising NoIndexException, and the extra results it recovers do not come from the negative label: the same vertex is still missing from has().out() and has().out().hasLabel("returnFan"). My comment on 559b220 asked for this change, and the probe below shows that request was wrong. Evidence: probe test added to VertexCoreTest in a scratch worktree, memory backend, JDK 11, run at 03a0c67 and at merge base 83ef9f3 with the testNegativeLabelOnSelfLoopKeepsSource fixture plus a Beijing person. Latest-head CI passes except codecov/patch and codecov/project.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: 73c2e85 restores VertexStep as the adjacency boundary, but the early exit only applies when every later step is on the onlyCurrentElementSuffix() allowlist. Appending dedup(), order(), valueMap(), elementMap(), fold(), groupCount() or project() to g.V().has("city","Beijing").out().hasLabel(P.neq("author")) moves the source property filter out of the index query, so the result set and the NoIndexException behaviour change with the terminal step. Evidence: plan probe in TraversalUtilOptimizeTest and a runtime probe on the memory backend (JDK 11) at 73c2e85; CI server lanes and unit/core suites are green at this head, codecov/patch and codecov/project fail.

// An allowlist is deliberate: select/path, lambdas, repeat and
// extension steps may recover earlier elements. Never infer their
// provenance from the output type alone.
if (!(changesCurrentElement(step) || step instanceof HasStep ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: The adjacency boundary restored in 73c2e85 only takes effect when every later step is on this allowlist, and common trailing steps are missing. DedupGlobalStep, OrderGlobalStep, PropertyMapStep (valueMap()), ElementMapStep, FoldStep, GroupCountStep and ProjectStep all fail the check, so hasUnsafeLabelInTraversal() keeps scanning past out(), finds the negative label, and prepareLocalHasContainers() leaves the source property local.

Evidence, from probes run at 73c2e85 with JDK 11:

  • Plan, using the TraversalUtilOptimizeTest helpers: has("city","Beijing").out().hasLabel(P.neq("author")) gives HugeGraphStep(Vertex,[city.eq(Beijing)]). Adding .dedup(), .order().by("name"), .valueMap(), .elementMap(), .fold(), .groupCount() or .project("n").by("name") gives HugeGraphStep(vertex,[]) plus a local HasStep([city.eq(Beijing)]), which is a full vertex scan. .values("name") keeps the index plan.
  • Runtime on the memory backend, using the testNegativeLabelAfterAdjacencyPreservesSourceCandidates data: out().hasLabel(P.neq("author")).values("name") returns [loop-person], while the same query with .dedup() before values() returns [loop-person, loop-fan]. order().by("name") and valueMap("name") also return 2 rows.
  • For an unindexed property, g.V().has("unindexedP","x").out().hasLabel(P.neq("author")) throws NoIndexException. Adding .dedup() returns 1 row after a scan.

On master 83ef9f3 all of these shapes push city into the source query. The change description says ordinary adjacency keeps the source query's index plan and missing-index errors, but that only holds when the traversal ends in one of the listed steps. As it stands, the result set and the error depend on which terminal step the caller appends.

Requested change: add the steps that keep or project the current traverser without recovering an earlier element (at least DedupGlobalStep, OrderGlobalStep, PropertyMapStep, ElementMapStep, FoldStep, GroupCountStep, ProjectStep), and check their by() children through the existing child recursion. Then extend testLabelAfterElementChangeKeepsSourceIndexPlan and testNegativeLabelAfterAdjacencyPreservesSourceCandidates with .dedup() and .order().by(...) variants, and extend the NoIndexException regression with a .dedup() form. If some of these steps stay excluded on purpose, list them in the Javadoc and the linked note.

@contrueCT contrueCT Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 2cbd5d7. The current-element suffix allowlist now includes dedup, order, valueMap, elementMap, fold, groupCount, and project; by() child traversals are still checked recursively, so select/path-style source recovery remains conservative. Added plan assertions for all seven steps and negative by(select) controls, plus Memory/RocksDB runtime regressions for dedup/order, edge adjacency, and NoIndexException. TraversalUtilOptimizeTest ran 48 tests with zero failures/errors/skips. Complete VertexCoreTest + EdgeCoreTest ran 479 tests on each of Memory and RocksDB with zero failures/errors and 74/29 skipped, respectively. The Java 17 all-module clean compile passed. Full TinkerPop and HStore suites were not rerun for this suffix-only change.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: 2cbd5d7 adds the seven suffix steps asked for on 73c2e85 and its new tests pass, but other common current-element steps (tail(), is(), unfold(), constant(), sample(), group(), aggregate(), simplePath()) are still outside onlyCurrentElementSuffix(), so g.V().has(prop, x).out().hasLabel(P.neq(y)) still changes from an index lookup with NoIndexException to a full scan depending on the terminal step. The ConditionQuery split and the LABEL call-site migrations look correct at this head. Evidence: TraversalUtilOptimizeTest (48/48) and QueryTest (14/14) pass at 2cbd5d7 on JDK 11; a plan probe with the TraversalUtilOptimizeTest helpers and a memory-backend runtime probe in VertexCoreTest (details inline); merge base 83ef9f3 pushes source has() containers without looking at later steps; latest-head CI is green except codecov/patch and codecov/project.

step instanceof PropertyKeyStep || step instanceof PropertyMapStep ||
step instanceof PropertyValueStep || step instanceof ElementMapStep ||
step instanceof ProjectStep || step instanceof FoldStep ||
step instanceof GroupCountStep ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: The allowlist now covers the seven steps from the last round, but other common steps that keep or reduce the current traverser are still missing, so the source plan still depends on which step ends the traversal. TailGlobalStep, IsStep, UnfoldStep, ConstantStep, SampleGlobalStep, GroupStep, AggregateGlobalStep and PathFilterStep (simplePath()) all fail this check. hasUnsafeLabelInTraversal() then keeps scanning past out(), finds the negative label, and prepareLocalHasContainers() leaves the source property local.

Evidence, from probes at 2cbd5d7 on JDK 11:

  • Plan, with the TraversalUtilOptimizeTest helpers and city as a TEXT key: has("city","Beijing").out().hasLabel(P.neq("author")) followed by .limit(5) or .valueMap() gives HugeGraphStep(Vertex,[city.eq(Beijing)]). Followed by .tail(1), .values("age").is(P.gt(1)), .fold().unfold(), .constant(1), .sample(2), .group().by(T.label), .aggregate("x") or .simplePath(), it gives HugeGraphStep(vertex,[]) plus a local HasStep([city.eq(Beijing)]), which is a full vertex scan.
  • Runtime on the memory backend, with the testUnindexedPropertyBeforeNegativeLabel schema plus one scanDoc edge: g.V().has("unindexedProp","x").out().hasLabel(P.neq("scanExcluded")) throws NoIndexException. The same query with .tail(1), with .fold().unfold(), or with .values("unindexedProp").is("y") returns a row after a scan.

At the merge base 83ef9f3, extractHasContainer() pushes the source containers without looking at later steps, so on master all of these shapes keep the source index lookup, and with an unindexed property they raise NoIndexException. The PR description says ordinary adjacency keeps the source query's index plan and missing-index errors and that only select/path/repeat or unknown extensions stay conservative, but tail(), is() and unfold() are none of those.

Requested change: add the remaining standard steps that cannot make an earlier element current again (at least TailGlobalStep, IsStep, UnfoldStep, ConstantStep, SampleGlobalStep, and GroupStep with its by() children checked through the existing recursion), and add .tail(1), .is(...) and .fold().unfold() rows to testLabelAfterElementChangeKeepsSourceIndexPlan and to the NoIndexException regression. If adding steps one at a time is not sustainable, an alternative is to reject only the steps that can make an earlier element current again (select, path and tree, where() on step labels, match(), repeat(), lambda steps, and unknown step classes) instead of allowing a fixed list. Whichever list stays excluded on purpose, state it in the code comment and in the linked note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improve]: clarify ConditionQuery.condition() semantics for missing, conflicting, and multi-value conditions

5 participants