fix(hstore): don't push sysprop-only range queries down to the store - #3184
Conversation
A sort-key prefix/range traversal reaches HstoreTable.queryByRange() with sysprop conditions only (owner vertex, direction, label, sort values) -- all already enforced by the scan key range. queryByRange() still pushed the serialized ConditionQuery down unconditionally, so the store tried to decode row property values it cannot parse (the server writes raw values, the store-side reader expects a self-describing (cardinality<<6)|dataType byte) and every such query failed with errors like: 'Can't construct Cardinality from code 0' / 'Unsupported data type UNKNOWN'. Apply the same prepareConditionQuery() guard that queryByPrefix() already uses: push the query down only when user-prop conditions remain. Also make both prepare methods operate on a copy of the origin query instead of mutating it via resetConditions() -- core still uses the origin query for its own result filtering after the scan returns. Part of apache#3090 (interim mitigation; the versioned sinking codec is tracked separately there). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Live A/B validation — single-node PD + Store + Server cluster built from source (Temurin 17), fresh graph, schema/data/queries from the reproducer linked in the PR description. Same running cluster, Before — master: full responses with stack traces (2 queries, 2 crashes)After — this branch (847fab9): the same queries return the correct edges (the remaining 4 reproducer queries also pass — 3/1/2/0 edges as expected): |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The guard is the right shape and I did not find a correctness regression. For sort-key edge queries core already resets user props on the copy it sends down (GraphTransaction.java:1591-1602), the scan key range encodes owner vertex, direction, label, sub-label and sort values (BinarySerializer.java:667-717), and core still filters with cq.test() because the query is marked OptimizedType.SORT_KEYS (GraphTransaction.java:1590, 1933), so dropping the pushdown does not widen the result set. Three notes inline: the sibling guard in prepareConditionQueryList() still lets label-only queries through, the pushdown copy adds a nesting level to the serialized payload, and the new user-prop test does not check what was pushed. Evidence: mvn -o -pl hugegraph-server/hugegraph-hstore -am test -Dtest=HstoreTableTest on JDK 11 at 847fab9 gives BUILD SUCCESS with 7/7 passing, matching the PR description (the same command on JDK 17 fails in hugegraph-commons on Lombok, in files this PR does not touch). Payload sizes come from probes compiled against this head. gh -R apache/hugegraph pr checks 3184 reports no checks on the branch, so the HStore integration lanes are unverified here, and I did not start a cluster, so the store-side decode failure itself is unreproduced.
…tent in test Review follow-up (thanks @bitflicker64): - setOriginQuery(null) on the pushdown copy in both prepare methods -- the copy() back reference nested the origin query into the serialized payload (measured by reviewer: 2275 -> 657 bytes); nothing store-side reads it - test now decodes the pushed bytes via ConditionQuery.fromBytes and asserts the user-prop condition survives, OWNER_VERTEX is dropped and the back reference is cleared Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The guard is sound: the pushed-down query is filter-only on the store side, every sysprop that can reach the edge range path is already encoded in the scan key, and core re-tests the results regardless. The copy-not-mutate change additionally stops the backend mutating a query its caller still owns. Evidence: built this head with JDK 11, mvn -o -pl hugegraph-server/hugegraph-hstore -am test -Dtest=HstoreTableTest gives 7/7. Store side is filter-only: hg-store-node/.../grpc/HgStoreWrapperEx.java hands the query bytes straight to FilterIterator.of, which returns the raw iterator when they are empty (hg-store-core/.../business/FilterIterator.java:45-47) and otherwise runs parseEdge/parseVertex then query.test() (lines 60-72). Core re-tests: the sort-keys branch sets optimized(OptimizedType.SORT_KEYS) (GraphTransaction.java:1590), and queryEdges(Query) feeds filterUnmatchedRecords (line 1014) into rightResultFromIndexQuery, which calls cq.test(elem) (line 1933). Key coverage: BinarySerializer.writeQueryEdgeRangeCondition() (line 667) writes owner vertex, direction, label and sub-label into the key, and the only server-code producer of an OTHER_VERTEX condition (EdgeExistenceTraverser.java:48) uses an EQ sort value, so hasRangeCondition() (line 660) is false for it and it never reaches that method. Payload shape: ConditionQuery.copy() sets originQuery(this) and resets optimizedType/resultsFilter (ConditionQuery.java:575-585), which is why setOriginQuery(null) is needed before bytes() (line 980).
…QueryList via queryByPrefixList Review follow-ups for apache#3184: - queryByRange(): compute the pushed query bytes in one place and end with a single session.scan(...) call; the ConditionQuery is block-local now. - HstoreTableTest: ScanRecordingSession records the owner-key list scan instead of throwing, and testPrefixListQueryPushesCopyAndKeepsOrigin drives prepareConditionQueryList() through queryByPrefixList() with a shared origin query: the origin keeps all conditions (including OWNER_VERTEX), the pushed payload drops OWNER_VERTEX, keeps LABEL and the user-prop condition, and has no back reference.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Both cleanups asked for on 1072872 landed without changing behaviour and I found no correctness defect at this head; the single note withdraws the reachability half of my own earlier comments.
Evidence: on JDK 11 at this head, mvn -o -pl hugegraph-server/hugegraph-hstore -am test is BUILD SUCCESS with 11 tests (HstoreTableTest 8/8) and mvn -o -pl hugegraph-server/hugegraph-hstore checkstyle:check is clean; git diff 1072872 0ecc10a is the queryBytes hoist plus the new test, with both former exits of queryByRange() collapsing into the same session.scan(...) call.
On the second behaviour change, which the title does not advertise: with user props present the range path now pushes a copy stripped of the OWNER_VERTEX conditions instead of the untrimmed origin. onlyOwnerVertex() (HstoreTable.java:612) drops only conditions whose relations are all on OWNER_VERTEX, and writeQueryEdgeRangeCondition() already pins that vertex in the scan key (writePartitionedId(HugeType.EDGE, vertex, start), BinarySerializer.java:680), so the trimmed filter cannot admit a row the key range would not return anyway. Suppressing a sysprop-only pushdown also puts hstore where RocksDBTable.queryByRange() (RocksDBTable.java:258-269) already sits: it pushes no query at all.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3184 +/- ##
============================================
+ Coverage 37.78% 37.90% +0.12%
- Complexity 6556 6604 +48
============================================
Files 800 800
Lines 68929 69033 +104
Branches 9157 9185 +28
============================================
+ Hits 26046 26170 +124
+ Misses 39824 39787 -37
- Partials 3059 3076 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@imbajin @contrueCT, a request for a decision on this PR. It is a 12-line guard in The reason I ask about the order: without this guard #2994 cannot be verified on HStore. Our backend axis for #2994 (the same queries on hstore and rocksdb, compared as id sets) gave 38 TARGET-ERR failures from #3090 before #3184 was on the branch, and OK=170 with it. Anyone reviewing #2994 on hstore will hit the same wall; @contrueCT noted this in #3090 on Sep 3. CodecovThe only red check is What this bug blocks on 1.7.0 with HStoreThe reproducer in the description has 3 edges. Below is the same failure on 5 M edges, on a schema with four sort keys (
On master with HStore every partial sort-key prefix and every range on a key other than the last one fails, including Script, raw results and log: |
|
@SebastianGruza @imbajin I think it makes sense to merge #3184 before #2994. The change here is relatively isolated: it avoids pushing sysprop-only range conditions to HStore when those constraints are already covered by the key range, while user-property residual conditions are still pushed down. Also, copying the More importantly, #3184 removes an existing HStore failure unrelated to #2994. Without it, the HStore regression for #2994 is partially blocked by #3090, which makes failures harder to attribute. So I suggest merging #3184 first, then rebasing #2994 on the latest master and rerunning the HStore/RocksDB regression matrix. #3090 should still remain open since #3184 is only a mitigation for the underlying codec issue. |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The range-path guard is sound and I found no correctness regression at this head: suppressing the pushdown only removes store-side filtering, every sysprop reaching the edge range path is already encoded in the scan key, and core re-tests the results; the copy-not-mutate change stops the backend stripping conditions from a query its caller still owns. One note: the same unguarded pushdown is still live in queryAll(). Evidence: built this head with JDK 17, mvn -o -pl hugegraph-server/hugegraph-hstore -am test -Dtest=HstoreTableTest gives 8/8. Store side is filter-only: FilterIterator.of returns the raw iterator when the query bytes are empty (hg-store-core/.../business/FilterIterator.java:44-50) and otherwise runs parseEdge/parseVertex then query.test() (lines 52-84). Key coverage: BinarySerializer.writeQueryEdgeRangeCondition() (line 667-717) writes owner vertex, direction, label and sub-label into the key. Core re-tests: optimizeQuery() sets optimized(OptimizedType.SORT_KEYS) (GraphTransaction.java:1589) and queryEdges(Query) feeds filterUnmatchedRecords (line 1014) into rightResultFromIndexQuery, which calls cq.test(elem) (line 1933). setOriginQuery(null) is payload-only: the store deserializes into org.apache.hugegraph.query.ConditionQuery (hugegraph-struct 1.7.0), whose test(BaseElement) reads ids, resultsFilter, conditions and element2IndexValueMap and never originQuery or optimizedType (verified with javap -c). manual_review_needed: no check runs are reported for eb5e05ab (combined status pending, no contexts), and no live PD + HStore cluster was available, so the author's A/B result was not reproduced.
|
@contrueCT thanks, agreed on the order. Once #2994 is rebased on a master that contains this PR, I will run the same HStore/RocksDB matrix (the same queries on both backends, compared as id sets, 170+ shapes) on the new head and post the result under #2994. @bitflicker64 the |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The range-path guard is sound. Sort-key edge queries already have their sysprops encoded in the scan key and core still re-tests results with cq.test(), so dropping the sysprop-only pushdown cannot widen results. Building the pushdown from a copy also stops the backend stripping conditions from a query core still uses. Earlier review points are addressed at this head, and the remaining queryAll() and shard-scan gap is tracked in #3090. Evidence: on this head with JDK 11, mvn -o -pl hugegraph-server/hugegraph-hstore -am test -Dtest=HstoreTableTest passes 8/8. All required checks and workflow runs are green. codecov/patch is red but not a required check, and the changed lines are covered by HstoreTableTest.
|
+1, agreed with the scope and merge order. @SebastianGruza @contrueCT @bitflicker64 this PR can go in ahead of #2994. The range guard is appropriately scoped, and the review threads are now resolved. After merging, please rebase #2994 and post the HStore/RocksDB regression results for the new head there, as planned. Keep #3090 open for the codec and remaining scan-path follow-ups, and update both threads to distinguish what #3184 fixes from what remains outstanding. |
There was a problem hiding this comment.
Follow-up context and merge order are noted in #3184 (comment).
4 participants
Purpose of the PR
On 1.7.x with HStore, every sort-key prefix-equality or range traversal fails, e.g.
g.V('a').outE('flow').has('asset','ETC')→Can't construct Cardinality from code 0, or with a range on the second sort key →Unsupported data type UNKNOWN. Minimal REST+Gremlin reproducer: https://gist.github.com/SebastianGruza/616f81e915f00f08c4be3dc447ca77c7 (analysis in #3090 (comment) — see the September comment).Root cause of the crash path: such a query reaches
HstoreTable.queryByRange()with sysprop conditions only (owner vertex, direction, label, sort values) — all already enforced by the scan key range — butqueryByRange()pushed the serializedConditionQueryto the store unconditionally. The store-side row decoder then tries to parse property values it cannot parse (server writes raw values, store reader expects a self-describing(cardinality<<6)|dataTypebyte) and crashes on the first row in range.Main Changes
queryByRange()now applies the sameprepareConditionQuery()guard thatqueryByPrefix()already uses: the query is pushed down only when user-prop conditions remain. No on-disk format change, no store-side change; the proper versioned sinking codec remains tracked in Track HStore core-test exclusions and property codec mismatch #3090.prepareConditionQuery()/prepareConditionQueryList()now operate on a copy of the origin query instead of mutating it viaresetConditions()— core still uses the origin query for its own result filtering after the scan returns.Verifying these changes
HstoreTableTest(a scan-recordingSessionfake):scan()receivesnullquery bytes, origin query untouched;mvn -pl hugegraph-server/hugegraph-hstore -am test -Dtest=HstoreTableTest: 7/7 pass.master(98477f0): sort-key prefix query fails withCan't construct Cardinality from code 0, range on the second sort key fails withUnsupported data type UNKNOWN— exactly as reported;Does this PR potentially affect the following parts?
🤖 Generated with Claude Code
Note on
codecov/patch: the red patch check is an artifact — the surefire run of thehugegraph-hstoremodule is not part of the coverage upload, so the report shows 0 % for the changed lines althoughHstoreTableTest(8 tests) covers them. Scale measurement (13 sort-key shapes on 5 M edges, HStore vs RocksDB id sets): https://github.com/SebastianGruza/hugegraph-validation/tree/master/results/pr-3184-sortkeys