Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
imbajin
left a comment
There was a problem hiding this comment.
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; " + |
There was a problem hiding this comment.
🧹 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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
bitflicker64
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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; " + |
There was a problem hiding this comment.
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):personmatchespersonByAge, butauthoralso declaresage(line 137) and only has the search indexauthorByLived.collectMatchedIndex()returns null forauthor,author.properties()containsage, so this loop throws. - line 485,
V().has("city", "Hangzhou"):personmatchespersonByCity, butFridgeSensordeclarescity(line 150) with no index. The primary-key shortcut inGraphTransaction.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.
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
GraphReadMode.ALL.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
ae90ad2passed Memory (497 run, 76 skipped), RocksDB (18 coverage/fallback cases plus the RANGE-context case), and HStore (19 cases). With the current #3243 head0de33edaand #2994 head2cbd5d71combined in a disposable tree, targeted source/adjacency cases passed on Memory (21 run, 2 skipped) and RocksDB (21 run, 0 skipped), andTraversalUtilOptimizeTestpassed 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
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.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.TaskCoreTestpassed 17/17.0de33eda,SourceIndexCoverageTestpassed 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.test-*push started the full TinkerPop CI; no result is claimed yet.127.0.0.1:8686was 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?
Documentation Status
Doc - TODODoc - Done: Bilingual REST API documentation PR #503; Gist migration note and design.Doc - No NeedThe 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.