Skip to content

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

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

contrueCT wants to merge 4 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 dependent optimization inside #2994's local-filtering fallback remains open.

A global property query across two labels can silently omit matching records from the label without a compatible index. This reproduces on master 2f827d6e without #2994 or a negative-label predicate.

For type-compatible property-index queries, this PR turns partial candidate coverage into NoIndexException. Complete compatible coverage returns the indexed result; no compatible index retains the existing error.

Main Changes

  • Check every query-visible candidate label with the existing single/composite/joint index matcher at execution. Candidate labels must declare the queried properties; empty visible labels still require coverage.
  • Follow the outer request's showHidden/showDeleting flags before matching or validating indexes. Normal queries exclude hidden, DELETING and UNDELETED labels; maintenance queries that expose those records retain their coverage requirement.
  • Validate selected indexes and paging support before opening backend iterators. Recheck each execution/page request and keep asynchronous stale-index cleanup independent.
  • Preserve base/sub-edge index coverage, shared OLAP indexes, and the existing empty-result path for type-incompatible values after a matching index is found. Explicit-ID, complete labeled PRIMARY_KEY and bounded-adjacency paths remain separate.
  • Complete the TinkerPop Modern/Basic/Sink fixture indexes and require unconditional full-result assertions. Scope Example1's left-index cleanup queries to person.

Compatibility: use hasLabel(...) for a query intended for one label. Cross-label property-index queries need predicate-compatible coverage for all visible candidates. A later hop or filter does not narrow the source's candidate set. There is no automatic full-scan fallback.

Design diagram, migration guidance, reproducible integration patch and detailed validation.

This PR is based directly on master and does not contain #2994's history. Selective index pushdown within #2994's conservative local-filtering path remains a separate #3201 task.

Verifying these changes

  • Need tests and can be verified as follows:

At 7f5a9173, on contrue@10.21.76.114:

Check Result
SourceIndexCoverageTest, Memory 22 run, 3 skipped, zero failures/errors
SourceIndexCoverageTest, RocksDB 22 run, 0 skipped, zero failures/errors
Complete RocksDB Vertex/Edge/Coverage classes 460 run, 29 skipped, zero failures/errors; final strengthened visibility/sub-label assertions rerun in the complete coverage class
Actual Example1 schema + left-index cleanup One external-harness test passed with Java assertions enabled; Curry's final age/city also checked
Formatting and Java 17 clean compile Passed; all 38 reactor modules compiled
Disposable integration with #2994 23cb5f07 79 unique targeted RocksDB cases passed; optimizer, coverage, adjacency/error and fallback coexistence checks

New regressions cover deletion ordering, UNDELETED, visible maintenance queries, inherited origin flags, sub-label deletion, restoration/reindexing and paging while a label remains hidden. The old code failed both new vertex/edge deletion probes.

The current test-* branch points at this exact head for TinkerPop CI; results are not claimed here. HStore and full TinkerPop suites were not manually rerun at this head. Earlier successful targeted HStore checks, startup failures and the non-green complete Memory Core run are recorded with their historical SHAs in the Gist. They are not current-head certification.

Concurrent DDL snapshot isolation, changing an index layout within a paging session, multi-store failover and the full distributed Gremlin/HTTP matrix remain outside the validation above. Backend formats, cursor formats and query-resource limits are unchanged.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API: type-compatible property-index queries with partial visible-candidate coverage now raise an index error; method signatures and REST shapes are unchanged.
  • Other affects
  • Nope

Documentation Status

The website documentation should merge with or after this code PR. Design notes, the diagram and validation evidence remain in Gist.

@codecov

codecov Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 42.85714% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.58%. Comparing base (2f827d6) to head (7f5a917).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...he/hugegraph/backend/tx/GraphIndexTransaction.java 42.85% 12 Missing and 8 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3243      +/-   ##
============================================
+ Coverage     41.40%   41.58%   +0.17%     
+ Complexity     7337     7331       -6     
============================================
  Files           802      795       -7     
  Lines         69792    69231     -561     
  Branches       9312     9283      -29     
============================================
- Hits          28897    28789     -108     
+ Misses        37604    37154     -450     
+ Partials       3291     3288       -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.

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

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

@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 coverage check, the visibility filter that follows the root query's showHidden/showDeleting flags, and the validate-before-iterate split in queryByUserprop look correct at this head, and the earlier OLAP, invalid-value, TinkerPop fixture, Example1 and DELETING-label threads are resolved. One test now covers less than its name says. Evidence: static review of the full diff against merge-base 2f827d6 at 7f5a917 (GraphIndexTransaction.collectMatchedIndexes/checkIndexCoverage, QueryResultContext visibility, GraphTransaction.invalidRecord, SchemaStatus.deleting, hidden-label callers in task/auth/variables that set showHidden); all 22 latest-head checks are green.


List<Vertex> vertices;
vertices = graph().traversal().V().has("age", 3)
vertices = graph().traversal().V().hasLabel("dog", "cat").has("age", 3)

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.

Minor: With hasLabel("dog", "cat") the query is flattened into two single-label queries, so this test no longer runs the unlabeled path where one collectMatchedIndexes() call returns a composite plan for dog and a joint plan for cat. That is the path queryByUserprop() changes here (all plans validated first, then iterated), and no case in SourceIndexCoverageTest mixes a composite and a joint plan in one unlabeled query (testCompositeCoverage and testJointIndexCoverage each use one shape on both labels). Please keep this query unlabeled and add compatible age/city indexes for the other candidate labels in this test, or add an unlabeled mixed composite/joint case to SourceIndexCoverageTest.

@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: When every candidate label is hidden or deleting, the visibility filter empties the index plan and queryByUserprop() throws instead of returning an empty result. Evidence: exact-head trace of collectMatchedIndexes() into the empty-index branch and the existing result-visibility filter.

// neither require coverage nor contribute unavailable index plans.
// Cleanup callers still need to inspect invisible labels' indexes.
Query visibility = query.rootOriginQuery();
schemaLabels = schemaLabels.stream().filter(schemaLabel ->

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: If the only candidate label for a valid property query is hidden or deleting, this filter removes it and leaves indexes empty; queryByUserprop() then throws NoIndexException at lines 483-486. Before this change, the indexed result was filtered to an empty public result by GraphTransaction.invalidRecord(). Please return an empty holder when every candidate is excluded by visibility, while retaining NoIndexException for visible candidates without usable indexes. Evidence: exact-head trace of these methods and the result-visibility check.

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