Skip to content

fix(server): reject incomplete cross-label index coverage - #3243

Open
contrueCT wants to merge 3 commits into
apache:masterfrom
contrueCT:task/3201-source-index-coverage
Open

contrueCT wants to merge 3 commits into
apache:masterfrom
contrueCT:task/3201-source-index-coverage

Conversation

@contrueCT

@contrueCT contrueCT commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Purpose of the PR

Refs #3201 (the independent source-query correction; the fallback optimization remains open).

On master 2f827d6e, a property query across two labels can silently omit the label without a compatible index. The regression reproduced one returned vertex when two matched, without any #2994 code or negative-label predicate.

This PR makes incomplete coverage an explicit NoIndexException. Complete coverage still returns the indexed result, and entirely unindexed queries retain their existing error. It does not introduce an automatic full scan.

Main Changes

  • Check every candidate schema label with the existing index matcher at source-query execution, including parent/sub-edge indexes and predicate compatibility. Preserve the separate stale-index cleanup path.
  • Validate all selected indexes and paging support before opening backend iterators. Recheck coverage after schema/index changes and on each page request; do not cache an optimizer-time promise.
  • Add cross-label regressions for equality, RANGE, SEARCH, composite/joint indexes, index lifecycle, traversal context and paging. Qualify older label-specific tests explicitly, while retaining unconstrained multi-label and empty-index paging fixtures.
  • Preserve the existing empty result for invalid property values when an index matches, and accept OLAP-only queries through their shared index in GraphReadMode.ALL.
  • Complete the TinkerPop Modern, Basic and Sink test schemas' shared-property index coverage, including empty candidate labels. Replace an exception-accepting test with an unconditional full-result assertion and add fixture-query regressions.

Compatibility: queries intended for one label should use hasLabel(...); genuine cross-label property queries need compatible indexes on every candidate label, including empty labels. A downstream hop or filter does not narrow the source's candidate set.

Design diagram, migration examples and validation evidence.

This branch starts directly from master and does not depend on #2994. Its integration with #2994 is tested separately; selective pushdown inside #2994's conservative local-filtering path is not part of this PR.

The earlier compatibility-only integration with ae90ad2 passed Memory (497 run, 76 skipped), RocksDB (18 coverage/fallback cases plus the RANGE-context case), and HStore (19 cases). With the current #3243 head 0de33eda and #2994 head 2cbd5d71 combined in a disposable tree, targeted source/adjacency cases passed on Memory (21 run, 2 skipped) and RocksDB (21 run, 0 skipped), and TraversalUtilOptimizeTest passed 48/48; there were zero failures/errors. This does not certify the full combined suites or HStore at these heads. The Gist distinguishes the earlier reproducible combined patch from this current-head validation and records their exact boundaries; none of that integration history is included here.

Verifying these changes

  • Need tests and can be verified as follows:
    • At ae90ad2, Java 11 complete RocksDB vertex/edge/coverage classes passed (452 run, 29 skipped); all 14 original HStore coverage cases passed after an external same-JVM transport-readiness check.
    • At f435a1f5, the two new regressions passed on RocksDB (2/2). The coverage class passed 16/16 before its HStore-only skip was added. On a fresh HStore graph, 16 ran with one OLAP test skipped, zero failures/errors; the invalid-value regression executed. On unmodified master, the existing HStore OLAP secondary-property test also returned zero instead of one, as recorded in the Gist.
    • Query/cache unit selection passed 42/42; isolated TaskCoreTest passed 17/17.
    • The full Memory Core suite ran 830 cases with 97 skipped and five Task-only failures/errors. Four reproduced on unmodified master; all 17 Task cases passed in the isolated patched run. This is not a green full-suite claim; details and startup-failure records are in the Gist.
    • At 0de33eda, SourceIndexCoverageTest passed on Memory (19 run, 2 skipped) and RocksDB (19 run, none skipped), with zero failures/errors. New tests exercise the Modern, Basic and Sink fixture schemas and assert complete results without accepting an exception.
    • At this head, formatting and the Java 17 all-module clean compile passed (38 modules). A test-* push started the full TinkerPop CI; no result is claimed yet.
    • A manual HStore retry reached no test method because PD at 127.0.0.1:8686 was unreachable (19 setup errors). A manual Structure run was stopped before completion after 30 minutes of slow fixture setup; Process was not manually run. HStore and the full TinkerPop suites remain unverified at this head.

Concurrent DDL snapshot isolation, multi-store failover and the full distributed Gremlin/HTTP matrix are not certified by these checks. No backend format, cursor format or query-resource limit is changed.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API: previously partial source queries now raise an index error; method signatures and REST shapes are unchanged.
  • Other affects
  • Nope

Documentation Status

The documentation change is in the separate hugegraph-doc repository and should merge with or after this code PR. The Gist keeps the design diagram and validation evidence; no explanatory document is committed into this code repository.

@codecov

codecov Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 41.37931% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.40%. Comparing base (2f827d6) to head (0de33ed).

Files with missing lines Patch % Lines
...he/hugegraph/backend/tx/GraphIndexTransaction.java 41.37% 12 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3243   +/-   ##
=========================================
  Coverage     41.40%   41.40%           
- Complexity     7337     7341    +4     
=========================================
  Files           802      802           
  Lines         69792    69819   +27     
  Branches       9312     9324   +12     
=========================================
+ Hits          28897    28910   +13     
- Misses        37604    37615   +11     
- Partials       3291     3294    +3     

☔ 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.

@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: Valid GraphReadMode.ALL OLAP-only vertex queries fail on multi-label schemas under the new coverage check. The low-level invalid-value case and missing in-repository compatibility guidance are also noted below. Evidence: Existing VertexCoreTest.testAddOlapSecondaryProperties and exact-head GraphIndexTransaction.MatchedIndex equality behavior; latest-head Actions passed, with codecov/project non-blocking.

}
// Authorization filters elements after the backend query. Do not
// disclose an implicitly discovered schema label in this error.
throw new NoIndexException("Incomplete index coverage for properties %s; " +

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.

🧹 The new compatibility rule is user-visible, and repository guidance requires matching in-repository documentation for behavior changes. The PR description links a Gist but this branch adds no query/index documentation. Please document that unlabeled property queries require compatible indexes for every candidate label, including the explicit-label and index remedies.

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.

I added the user-facing guidance in apache/hugegraph-doc#503, alongside the existing properties parameter descriptions in the English and Chinese Vertex and Edge REST API pages. It explains unlabeled candidate labels (including empty labels), no/partial/complete index coverage, and the label or compatible-index remedies. The Edge page also distinguishes vertex_id-bounded adjacency filtering from a global edge search. I linked the docs PR in this PR description; it should merge with or after #3243. The design Gist remains for implementation details, with no explanatory document added to the code repository.

@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 OLAP-only regression already raised on GraphIndexTransaction.java:831 is confirmed, and the green CI at this head does not cover it: every OLAP core test is skipped in all backend jobs, so nothing in the build would catch it. The invalid-value short circuit (line 483) and the missing in-repository documentation (line 853) from the existing review are also confirmed and are not repeated here. Evidence: static reading of checkIndexCoverage(), collectMatchedIndex() and MatchedIndex.equals()/hashCode() at ae90ad2; hugegraph-test/pom.xml sets the backend system property only for the api-test execution, while testAddOlap*Properties require System.getProperty("backend") to equal "hstore"; the hstore job log shows CoreTestSuite with 830 run, 0 failures, 48 skipped; all latest-head checks pass except codecov/project.

@contrueCT contrueCT changed the title fix(core): reject incomplete cross-label index coverage fix(server): reject incomplete cross-label index coverage Sep 28, 2026

@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 coverage check works as described for the core cases, but the TinkerPop modern schema used by the process suite declares name and age on the unindexed animal label, so every unlabeled has("name", ...) lookup there now throws NoIndexException; that suite only runs on release-* and test-* branches, so the green PR checks do not cover it. One new test also passes whether the code throws or not. Evidence: static review of the full diff against merge-base 2f827d6; TestGraph.initModernSchema() and gremlin-test 3.5.1 AbstractGremlinTest.convertToVertex() bytecode; server-ci.yml gates Run TinkerPop test on RELEASE_BRANCH; a throwaway memory-backend core test that loads initModernSchema(AUTOMATIC) and runs V().has("name", "marko") and V().has("age", 29) throws NoIndexException at f435a1f and returns the vertex with master's GraphIndexTransaction.

@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: Global property queries can fail during vertex or edge label deletion after the indexes are removed but before the deleting label leaves schema candidates. Evidence: exact-head GraphIndexTransaction.checkIndexCoverage, SchemaTransactionV2.getAllSchema, and VertexLabelRemoveJob/EdgeLabelRemoveJob removal ordering.

for (MatchedIndex index : indexes) {
covered.add(index.schemaLabel().id());
}
for (SchemaLabel label : labels) {

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.

⚠️ Important: getVertexLabels() / getEdgeLabels() still returns a label after it is marked DELETING. The removal jobs delete that label's index labels before deleting its data and schema, while normal query results filter out DELETING records (GraphTransaction.invalidRecord). During that interval, this loop throws Incomplete index coverage for a global query whose remaining visible labels are fully indexed. Please exclude labels hidden by the query's deleting visibility from this coverage check, or synchronize the check with label removal.

@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 earlier OLAP, invalid-value, TinkerPop fixture and test findings are resolved at 0de33ed, and the fork's TinkerPop run at this SHA logged no NoIndexException. One in-repository caller was missed: Example1.testLeftIndexProcess() runs unlabeled property queries against a schema with unindexed candidate labels, so the shipped example now aborts with the new NoIndexException. The open DELETING-label thread on line 845 is not repeated. Evidence: static trace of Example1.loadSchema() and testLeftIndexProcess() through GraphTransaction.optimizeQuery(), collectMatchedIndex() and checkIndexCoverage() at 0de33ed; git grep for unlabeled V().has(/E().has( across hugegraph-example (Example2 and PerfExample3/4 are fully covered); contrueCT/hugegraph run 36529087648 (test-3243-index-coverage, same SHA): Structure 940 run with 0 failures on memory and hbase, Process 815 run with 1 failure on memory and hbase (EventStrategyProcessTest listener count at line 215, not an index query), RocksDB errors are rocksdb-data/m LOCK failures, 0 occurrences of NoIndexException in the failed-job logs; PR checks green except codecov/patch.

}
// Authorization filters elements after the backend query. Do not
// disclose an implicitly discovered schema label in this error.
throw new NoIndexException("Incomplete index coverage for properties %s; " +

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: This throw breaks the shipped hugegraph-example Example1, which the PR does not update. Example1.main() calls testLeftIndexProcess() (Example1.java:64), and that method runs two unlabeled property queries against the schema from loadSchema():

  • line 482, V().has("age", 28): person matches personByAge, but author also declares age (line 137) and only has the search index authorByLived. collectMatchedIndex() returns null for author, author.properties() contains age, so this loop throws.
  • line 485, V().has("city", "Hangzhou"): person matches personByCity, but FridgeSensor declares city (line 150) with no index. The primary-key shortcut in GraphTransaction.optimizeQuery() needs a label condition, so the query reaches this check and throws.

On master both queries return the indexed person result (empty, which is what the method's asserts check). At 0de33ed main() stops with Incomplete index coverage before thread(graph) runs. The example module has no CI job, so the green checks do not cover it. Example2 and PerfExample3/4 are fully indexed and are not affected.

Requested change: add .hasLabel("person") to the two queries at Example1.java:482 and :485 (the method tests left-index cleanup on person), or add age and city indexes on author and FridgeSensor in loadSchema(), the same way this PR completed the TinkerPop fixtures in TestGraph.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants