Skip to content

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

Merged
imbajin merged 9 commits into
apache:masterfrom
contrueCT:task/3201-source-index-coverage
Oct 19, 2026
Merged

imbajin merged 9 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.

Global property queries can silently omit records from labels without compatible indexes. For type-compatible values, this change requires complete coverage for every query-visible candidate label and raises NoIndexException for partial coverage. Adding hasLabel(...) or compatible indexes on all candidates provides an explicit remedy.

Main changes

  • Select labels using the outer request's hidden/deleting visibility. Each flattened branch validates complete coverage and every selected index before opening its backend iterators. Empty labels still require coverage.
  • Close holders in partially constructed index branches as well as earlier OR branches. Joint planning closes unselected cursors and transfers only the selected holder; SEARCH decoding failures retain the original error when cleanup also fails.
  • Return an empty result when every candidate is excluded by visibility, including after index removal. Paged queries return an empty first page with no continuation token. Visible candidates without compatible indexes still fail.
  • Preserve shared OLAP indexes, base/sub-edge index coverage, the existing invalid-value short circuit after index matching, and maintenance index cleanup. Base edge labels have no direct records.
  • Resolve explicit sub-edge labels through a query-visible parent index when no compatible direct index exists. Filter sibling edges using SUB_LABEL, apply non-paged limits after filtering, and preserve page continuation and index-status validation.
  • Cover composite and joint index plans together in one unlabeled query, including nonmatching records and an unavailable joint index.
  • Complete the TinkerPop fixture indexes and scope Example1's cleanup queries to person. Integrate current master, including shared query/schema imports and empty label intersections.

Explicit-ID lookups and bounded-adjacency filtering retain their existing paths. Concurrent schema snapshot isolation and changing index layouts within a paging session remain outside this change.

Concept illustration: complete index coverage for query-visible labels

Concept illustration: for type-compatible values, global property-index queries require compatible indexes for every query-visible candidate label. Incomplete coverage raises NoIndexException; candidates excluded by visibility yield an empty result with no continuation token.

Verifying these changes

  • Need tests and can be verified as follows:
Check Result
Java 17 mvn clean compile -Dmaven.javadoc.skip=true All 38 reactor modules passed
Memory: complete SourceIndexCoverageTest 39 tests, 3 skipped, zero failures/errors
RocksDB: complete SourceIndexCoverageTest, VertexCoreTest, EdgeCoreTest 523 tests, 29 skipped, zero failures/errors
GraphTransactionTest, QueryListTest, QueryResultsTest, IdHolderTest 43 tests, zero skipped/failures/errors
Exact SEARCH read/close and decode/close regressions on fresh RocksDB data 2 tests, zero skipped/failures/errors
mvn editorconfig:format and git diff --check Passed

The full backend suites used fresh local data directories. The coverage class has
39 methods; all ran on RocksDB. Its parent-index regressions exercise explicit
sub-label secondary, range, joint and search queries, sibling filtering with a
limit, both sub-labels' pagination, deleting-parent visibility, index rebuilding,
and direct child-index preference.

Cursor regressions use real schema, index matching, serialization and backend
queries. Wrappers count actual backend iterator opens/closes and inject failures.
They cover earlier OR branches, a partially built current branch, joint querying
and reading, cleanup errors, and successful ownership transfer. The decode test
reads a real backend entry before throwing outside the iterator wrapper; it failed
before the BatchIdHolder.all() fix because the close error replaced the decode
error. Both native-read and decode cases now preserve the original error and attach
cleanup errors as suppressed exceptions.

The root negative-label regression verifies that the reported traversal keeps both
filters local and opens no property-index cursor. It does not assert indexed
execution for all negative-label compositions. Invalid-value, mixed composite/joint,
invisible-only candidate, empty-page and sub-label deletion regressions remain in
the complete coverage class. HStore and the full TinkerPop suites were not rerun
locally; remote CI should be checked on the exact pushed head before merge.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API: incomplete property-index coverage raises an index error; candidates excluded by visibility return an empty result. REST shapes and method signatures are unchanged.
  • Other affects
  • Nope

Documentation Status

Coordinate the website documentation merge with this code PR. Earlier design and validation evidence remains in the historical Gist.

@codecov

codecov Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.37748% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.24%. Comparing base (92af2f8) to head (3b597b7).
⚠️ Report is 34 commits behind head on master.

Files with missing lines Patch % Lines
...he/hugegraph/backend/tx/GraphIndexTransaction.java 93.65% 0 Missing and 8 partials ⚠️
...va/org/apache/hugegraph/backend/page/IdHolder.java 90.00% 0 Missing and 1 partial ⚠️
.../apache/hugegraph/backend/tx/GraphTransaction.java 93.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3243      +/-   ##
============================================
+ Coverage     54.31%   59.24%   +4.93%     
- Complexity     2250     4277    +2027     
============================================
  Files           770      518     -252     
  Lines         67952    50342   -17610     
  Branches       9213     7586    -1627     
============================================
- Hits          36905    29826    -7079     
+ Misses        27470    17642    -9828     
+ Partials       3577     2874     -703     
Flag Coverage Δ
hstore 35.80% <23.84%> (?)
server_memory 54.44% <88.07%> (?)
server_rocksdb 42.55% <93.37%> (?)

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

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

Preserve empty label-intersection handling.
Retain complete index coverage validation.
Integrate current server query and test APIs.
- Return empty results and pages for visibility-excluded candidates.
- Cover unlabeled composite and joint plans in one query.
- Adapt merged traversal fixtures and shared schema imports.
- Document property-index coverage and visibility rules.
@imbajin

imbajin commented Oct 15, 2026

Copy link
Copy Markdown
Member

Review of 15d87e19: 8/10 after two independent source reviews and cross-checking.

P2 — Close earlier OR-branch cursors when coverage validation fails. For a non-paged vertex ConditionQuery with p='hit' OR q='hit', let labels A/B both declare p/q, index p on both, and index q only on A. GraphTransaction.optimizeQueries() eagerly opens the first branch’s index iterators, then the new checkIndexCoverage() rejects the second branch. The partially populated QueryList is abandoned before fetch() establishes cleanup ownership. On RocksDB, native cursors are already allocated; transaction rollback does not close them.

This exposes an existing construction-cleanup gap through the new partial-coverage rejection path. Please close the accumulated QueryList when planning fails, preserving the original exception, and add an open/close-count regression. Relevant locations: GraphTransaction.java:1517–1542, GraphIndexTransaction.java:403–404,588–591, and QueryList.java:87–117.

Source-confirmed finding, not a runtime reproduction. The previous visibility and mixed-index concerns are addressed in the current source; CI and full runtime acceptance remain separate.

- Close cursors accumulated from earlier OR branches on failure.
- Preserve the planning error and suppress cleanup failures.
- Verify real-backend open/close counts and successful ownership.
- Clarify per-branch coverage and cursor cleanup documentation.
@imbajin

imbajin commented Oct 15, 2026

Copy link
Copy Markdown
Member

Follow-up on #issuecomment-6191363696 at 4a9427c: the reported earlier-OR-branch cursor leak is resolved in the source. Planning failure now closes accumulated holders, continues cleanup after a close throws, and preserves the original exception with cleanup failures suppressed. The three regression tests retain real index/backend planning and cover failure cleanup, a throwing close, and successful-result ownership.

Independent cross-review score for this fix delta: 9/10. No new blocking finding in this delta. This is source verification; I did not rerun the author’s Java 17 tests. Current-head CI should still be evaluated separately. This delta score is not a new whole-PR rating.

@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: Score 9/10. At 4a9427c a global property query now needs a compatible index on every query-visible candidate label, labels hidden from the query return an empty result, and cursors opened by earlier OR branches are closed when planning fails. Every earlier thread (OLAP-only queries, invalid values, the TinkerPop fixtures, DELETING and hidden labels, Example1, the mixed composite and joint plan, and the cursor leak) is fixed. I found no new problems. Evidence: read the full diff plus collectMatchedIndex/matchJointIndexes, MatchedIndex equality, updateEdgeIndex, SchemaTransaction.removeEdgeLabel (a base label cannot be removed while it has sub-labels), Query.rootOriginQuery and QueryResultContext (both use the outer query's visibility), ConditionQueryFlatten (label IN is always split, because no in-tree backend supports IN), QueryList.close, and the internal hidden-label callers, which all set showHidden(true). All latest-head checks pass. On the library host at this exact head with the memory backend, run-tinkerpop-test.sh process passed: 1295 tests, 0 failures, 124 skipped, including all 361 HugeGraphFeatureTest scenarios under TinkerPop 3.8.1, and "Incomplete index coverage" never appears in the log. Not verified at this head: the TinkerPop 3.8.1 structure suite was stopped after about 30 minutes of slow fixture setup with no failures, and TinkerPop was not run on RocksDB. hugegraph-doc#503 should be merged together with this PR.

@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: A root-level negative label filter can still require coverage from an excluded label, so valid property queries can fail with NoIndexException. Evidence: Exact-head static trace from TraversalUtil label extraction through GraphIndexTransaction.checkIndexCoverage; current-head CI checks pass.

Comment thread docs/server-runtime.md Outdated
- Reproduce the root negative-label query with an unindexed excluded label
- Assert local filtering, exact results and zero index cursor opens
- Clarify property-type short circuit in the query coverage documentation

@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: no. Summary: Two non-blocking edge cases remain in explicit sub-label index planning and cleanup of a partially built failing index branch. Evidence: exact-head static traces through querySchemaLabels, updateEdgeIndex, optimizeQueries and doIndexQueryBatch; latest-head visible checks pass.

@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. Score: 10/10.

Production code is unchanged since 4a9427c. 83ac535 adds the negative-label regression and qualifies the docs row for type-compatible values; both match queryByUserprop(), where a matched index plus an invalid value returns empty before the coverage check.

Earlier threads:

  • OLAP-only queries and the invalid-value short circuit: fixed in f435a1f.
  • TinkerPop fixtures and the early-return coverage test: fixed in 0de33ed.
  • DELETING labels and Example1: fixed in 7f5a917.
  • Mixed composite and joint plan, and an empty result when every candidate is hidden or deleting: fixed in 15d87e1.
  • Earlier OR-branch cursors on planning failure: fixed in 4a9427c.
  • Docs row qualification: fixed in 83ac535.
  • Negative label at GraphIndexTransaction.java:772: does not reproduce. At this head, on memory, hasLabel(P.neq(...)), P.without, not(hasLabel(...)), within followed by neq, or(hasLabel(neq)) and the edge variants all return only the allowed matches. The unlabeled controls still raise the coverage error.
  • Explicit sub-label query with only a base-label index (line 692): still open, but master 6f98193 gives the same NoIndexException with the same message for the same fixture. Label resolution and collectMatchedIndex() are unchanged here, so I see it as a follow-up and not a defect of this PR.
  • Holders of a partially built branch on a backend error (GraphTransaction.java:1538): still open. Master has the same gap, and also leaked earlier branches and earlier holders on an invalid index label. This PR validates the whole plan before opening anything and closes earlier branches, so the remaining case is narrower than on master. Follow-up, not deducted.

Evidence: memory backend at this head, SourceIndexCoverageTest 30 run / 3 skipped (paging and OLAP), VertexCoreTest 300 / 48 skipped, EdgeCoreTest 184 / 26 skipped, no failures. All checks pass at this head.

Not verified: the TinkerPop process suite passed at 4a9427c but was not rerun here. Production code and TestGraph have not changed since. The structure suite and TinkerPop on RocksDB were not run. hugegraph-doc#503 should merge together with this PR.

- Resolve query-visible parent indexes for explicit sub-edge labels
- Filter sibling edges before limits and preserve page continuation
- Close partial plans and unselected joint cursors
- Preserve original search failures when cursor cleanup also fails
- Add backend counting regressions and update the query contract

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

Fresh whole-PR review at 3b597b7: 9/10. No new blocking finding.

Both nonblocking findings from my October 16 review are addressed. Explicit sub-label queries can now use a query-visible parent index while filtering siblings and retaining direct-index preference. Partially built branches close their owned cursors; joint plans close unselected cursors and transfer the selected one; read/decode failures retain the original error when cleanup also fails.

I reviewed the full 10-file diff and current discussion, with an independent cross-check of sub-label planning. An isolated cloud Java harness using the unmodified IdHolder and exact joint-planning/cleanup method bodies passed 11 cases and 30 assertions at this head. The same harness fails seven cases at 83ac535. Graph/backend dependencies were test doubles, so these are bounded lifecycle checks, not independent Memory/RocksDB integration acceptance or performance evidence. The checked-in backend-spy regressions retain real index planning and test the relevant failures meaningfully.

Current-head CI and the author-reported backend results remain separate evidence; the exact-head run is still running/queued (5 successful checks, 9 skipped, 3 running/queued). Please keep the documentation merge coordinated with 🔗 hugegraph-doc #503.

Exact-version locations: sub-label filtering, joint-plan ownership, primary-error preservation.

@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: Score 9/10. At 3b597b7 an explicit sub-edge-label property query with no direct index now falls back to the query-visible parent index, filters sibling sub-labels before offset, limit and count are applied, and still prefers a compatible direct index. Partially built index branches and unselected joint cursors are closed, and the original error survives a failing close. Both open threads from the October 16 review (sub-label parent index at GraphIndexTransaction.java:742, partial-branch holders at GraphTransaction.java:1538) are addressed, and I found no new problems. Evidence: read the full 10-file diff against merge-base 4b77b7b, and traced prepareIndexQuery() through QueryResultContext (the nearest INDEX_FILTER and results filter come from the rewritten query, so siblings are rejected without left-index cleanup and offsets are not copied into the id holders), BaseEdge.sysprop(LABEL/SUB_LABEL), constructSearchQuery(), JointQuery.originQuery(), the count fallback in queryNumber(), and the close paths in queryByUserprop(), doSearchIndex(), doJointIndex() and BatchIdHolder.all(). On the library host at this exact head, SourceIndexCoverageTest, ParentAndSubEdgeCoreTest, CountStrategyCoreTest and four throwaway probes (sub-label count, range/skip/limit with interleaved siblings, OR branches and a range plus secondary joint plan through the parent, drop by sub-label keeping siblings) ran 135 tests with 0 failures on memory (3 skipped: paging and OLAP) and 135 with 0 failures and 0 skipped on RocksDB. All 10 non-skipped checks pass at this head, including the memory, RocksDB, HStore and cluster jobs. Not verified at this head: the TinkerPop suites (they last passed at 4a9427c; TestGraph is unchanged since, and the new code only changes explicit sub-edge-label planning and cursor cleanup). hugegraph-doc#503 should merge together with this PR.

@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: no. Summary: The selected head's index coverage, visibility, sub-edge planning and cursor ownership changes have no remaining actionable issues after full review. Evidence: All 10 non-skipped exact-head checks passed and five independent review lanes found no new code defect. Score: 8.8/10.

@imbajin
imbajin merged commit 86c05ef into apache:master Oct 19, 2026
19 checks passed
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