Repository navigation
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 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.
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.
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.
bitflicker64
left a comment
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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 -> |
There was a problem hiding this comment.
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.
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
2f827d6ewithout #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
showHidden/showDeletingflags before matching or validating indexes. Normal queries exclude hidden,DELETINGandUNDELETEDlabels; maintenance queries that expose those records retain their coverage requirement.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
At
7f5a9173, oncontrue@10.21.76.114:SourceIndexCoverageTest, MemorySourceIndexCoverageTest, RocksDB23cb5f07New 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?
Documentation Status
Doc - TODODoc - Done: Bilingual REST API documentation PR #503; Gist migration note and design.Doc - No NeedThe website documentation should merge with or after this code PR. Design notes, the diagram and validation evidence remain in Gist.