fix(hstore): don't push sysprop-only range queries down to the store - #3184
fix(hstore): don't push sysprop-only range queries down to the store#3184SebastianGruza wants to merge 3 commits into
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.
| conditionQuery.resetConditions(newConditions); | ||
| return conditionQuery; | ||
| // NOTE: copy before reset, see prepareConditionQuery() | ||
| ConditionQuery pushdown = conditionQuery.copy(); |
There was a problem hiding this comment.
containsLabelOrUserpropRelation() (line 588), while prepareConditionQuery() uses userpropConditions() (line 559). That call returns true for a bare HugeKeys.LABEL relation (hugegraph-core backend/query/ConditionQuery.java:249-253), so a batched edge query with a label and no user property still reaches resetConditions() with sysprop-only conditions and is pushed at line 544. Same shape this PR removes from queryByRange(). The path is live: query(Session, List<IdPrefixQuery>, String) at line 331.
The guard is pre-existing and outside this diff, so not a change request on this PR: worth a follow-up on #3090, or a line in the new comment saying why the list path is different.
Confidence: the guard mismatch is confirmed by reading the two methods. Whether this batch path actually hits the decode failure is not, since I could not reproduce it locally.
There was a problem hiding this comment.
Agreed, and thanks for confirming the guard mismatch. Leaving the list path untouched here to keep the diff minimal — will flag containsLabelOrUserpropRelation() vs userpropConditions() (and the related question of whether label-only pushes can hit the same decode path, plus the cross-module Id equality concern for LABEL/SUB_LABEL) as a follow-up under #3090.
| return conditionQuery; | ||
| // NOTE: copy before reset, the origin query is still used by core | ||
| // for result filtering after the backend scan returns | ||
| ConditionQuery pushdown = conditionQuery.copy(); |
There was a problem hiding this comment.
🧹 The copy keeps a link back to the query it came from, so the payload grows by one nesting level. ConditionQuery.copy() ends with query.originQuery(this) (hugegraph-core backend/query/ConditionQuery.java:577), Query.originQuery is non-transient (backend/query/Query.java:78), and bytes() serializes with a Gson that registers type adapters but no exclusion strategy (ConditionQuery.java:81-85, 980-983).
Measured on a build of this head, for an edge query with owner vertex, direction, label and one user-prop condition, using the shape the pipeline produces (the origin is itself a copy from GraphTransaction.java:1591, so one level of nesting predates this PR):
- before: 1466 bytes
- after: 2275 bytes
- after, with the back reference cleared: 657 bytes
No behaviour change: store-side consumers read only the top-level query (hg-store-core/.../business/FilterIterator.java:53-85, hg-store-node/.../query/stages/FilterStage.java:36-52) and nothing in hugegraph-store reads originQuery.
Requested change: pushdown.setOriginQuery(null); after the reset, in both this method and prepareConditionQueryList(). Query.setOriginQuery(Query) is public (backend/query/Query.java:142-144), and the pushdown is discarded right after bytes(), so nothing else observes the link.
There was a problem hiding this comment.
Done in 1072872 — setOriginQuery(null) on the pushdown copy in both prepare methods. Thanks for measuring the payload; the nesting predating this PR (origin already being a copy from GraphTransaction.java:1591) was a good catch I had missed entirely.
| this.newTestTable().queryByRange(session, edgeRangeQuery(origin)); | ||
|
|
||
| Assert.assertTrue(session.scanCalled); | ||
| Assert.assertNotNull(session.lastQueryBytes); |
There was a problem hiding this comment.
🧹 This asserts only that some bytes were pushed. What is worth pinning down is which conditions survive prepareConditionQuery(), and that is unchecked: the test still passes if the code pushes the untrimmed origin, or keeps the owner-vertex condition the method drops. The size assertion on line 171 does guard the copy-not-mutate change, so only the payload content is unverified.
Requested change: decode with ConditionQuery.fromBytes(session.lastQueryBytes) (hugegraph-core backend/query/ConditionQuery.java:763-773) and assert that the user-prop condition survives and condition(HugeKeys.OWNER_VERTEX) is null. I checked the round trip works on this head.
Separately, a question rather than a change: is this shape reachable? An IdRangeQuery with an edge result type seems to come only from BinarySerializer.writeQueryEdgeRangeCondition() (line 717), reached only from the sort-keys branch that calls resetUserpropConditions() (GraphTransaction.java:1602). If so prepareConditionQuery() always returns null here in practice, and this test pins a synthetic shape.
There was a problem hiding this comment.
Done in 1072872 — the test now decodes with ConditionQuery.fromBytes and asserts the user-prop survives, OWNER_VERTEX is gone and originQuery is null.
On reachability: agreed — for EDGE sort-key traversals core resets user props before the backend sees the query (that is exactly why suppressing the pushdown is safe), so on current server paths prepareConditionQuery() should return null here in practice. The test deliberately pins the method contract rather than a live traversal shape: queryByRange also serves VERTEX result types and any future caller, and if core ever stops stripping user props, this is the behavior we want locked. Happy to add a comment in the test saying the shape is synthetic.
…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).
| // range, and the store-side row decoder cannot parse the raw | ||
| // property layout written by the server (see issue #3090). | ||
| cq = prepareConditionQuery((ConditionQuery) origin); | ||
| byte[] queryBytes = cq == null ? null : cq.bytes(); |
There was a problem hiding this comment.
🧹 With the guard in place both exits are the same session.scan(this.table(), ownerStart, ownerEnd, start, end, type, ..., position) call, differing only in the query bytes (lines 661-662 vs 664-665).
Requested change: initialise byte[] queryBytes = null; before the if, assign it inside the branch, and end the method with a single return session.scan(...). While there, the method-scoped ConditionQuery cq; at line 634 can become a block-local; that declaration predates this PR, so treat it as optional.
There was a problem hiding this comment.
Done in 0ecc10a — queryBytes is initialised before the if, assigned inside it, and the method ends with a single session.scan(...); cq is block-local now.
| // NOTE: copy before reset, see prepareConditionQuery() | ||
| ConditionQuery pushdown = conditionQuery.copy(); | ||
| pushdown.resetConditions(newConditions); | ||
| pushdown.setOriginQuery(null); |
There was a problem hiding this comment.
🧹 The copy-not-mutate change lands in both prepare methods, but the new tests cover only prepareConditionQuery(), and only through queryByRange(). prepareConditionQueryList() has two live call sites, queryByPrefixList() (line 542) and the streaming query(Session, Iterator<IdPrefixQuery>, String) (line 360), and neither is exercised: HstoreTableTest has 7 tests and none reach the prefix or list paths. Its entry guard containsLabelOrUserpropRelation() (line 590) admits label-only edge queries, so both call sites do reach line 603.
Requested change: make ScanRecordingSession.scan(String, List<HgOwnerKey>, int, long, byte[]) record instead of throw, and add a queryByPrefixList() case asserting the shared origin query keeps its OWNER_VERTEX condition after the scan.
There was a problem hiding this comment.
Done in 0ecc10a — ScanRecordingSession.scan(String, List<HgOwnerKey>, int, long, byte[]) now records (query bytes, owner keys, one iterator per key) instead of throwing, and testPrefixListQueryPushesCopyAndKeepsOrigin drives prepareConditionQueryList() through queryByPrefixList() with one origin shared by two prefix queries: the origin keeps all of its conditions including OWNER_VERTEX, the decoded pushed payload has no OWNER_VERTEX, keeps LABEL and the user-prop condition, and originQuery is null. 8/8 with mvn -pl hugegraph-server/hugegraph-hstore -am -Dtest=HstoreTableTest test on Temurin 17.
…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.
|
|
||
| @Test | ||
| public void testPrefixListQueryPushesCopyAndKeepsOrigin() { | ||
| // prepareConditionQueryList() is reached from queryByPrefixList() and |
There was a problem hiding this comment.
🧹 Worth stating what sits above these two call sites: nothing calls either of them in a live server, so prepareConditionQueryList() does not run today. As written, lines 184-186 read as a description of a live path.
queryByPrefixList (HstoreTable.java:526) has one production caller, HstoreTable.query(Session, List<IdPrefixQuery>, String) (line 331), whose only caller is HstoreStore.query(List<HugeType>, List<IdPrefixQuery>) (HstoreStore.java:506), and that overload is never called: HstoreStore.java:492 passes a QueryWrapper implements Iterator<IdPrefixQuery> and so binds to the Iterator overload at line 540. The streaming query(Session, Iterator<IdPrefixQuery>, String) (HstoreTable.java:348, called from HstoreStore.java:557) is reachable only through BackendStore.query(Iterator<Query>, Function, HugeGraph) (BackendStore.java:75-78), which carries the comment // TODO: unused now; nothing in the repository supplies its queryWriter argument.
This withdraws the reachability half of my earlier comments (3916985809, 3921393173), which called these call sites live and are where the wording here came from. The guard mismatch itself still stands, but it cannot reach the store row decoder as things are, which is worth recording in the #3090 follow-up.
Requested change, optional and not worth a repush on its own: reword lines 184-186 to say the method is called from those two sites and that neither has a live caller today.
2 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