bitflicker64 commented on code in PR #3184:
URL: https://github.com/apache/hugegraph/pull/3184#discussion_r4023288079
##########
hugegraph-server/hugegraph-hstore/src/main/java/org/apache/hugegraph/backend/store/hstore/HstoreTable.java:
##########
@@ -640,21 +647,20 @@ protected BackendColumnIterator queryByRange(Session
session,
if (query.paging() && !query.page().isEmpty()) {
position = PageState.fromString(query.page()).position();
}
+ byte[] queryBytes = null;
if (origin instanceof ConditionQuery &&
(query.resultType().isEdge() || query.resultType().isVertex())) {
- cq = (ConditionQuery) query.originQuery();
-
- // LOG.debug("query {} with ownerKeyFrom: {}, ownerKeyTo: {}, " +
- // "keyFrom: {}, keyTo: {}, " +
- // "scanType: {}, conditionQuery: {}",
- // this.table(), bytes2String(ownerStart),
- // bytes2String(ownerEnd), bytes2String(start),
- // bytes2String(end), type, cq.bytes());
- return session.scan(this.table(), ownerStart,
- ownerEnd, start, end, type, cq.bytes(),
position);
- }
- return session.scan(this.table(), ownerStart,
- ownerEnd, start, end, type, null, position);
+ // Same guard as queryByPrefix(): only push the query down to the
+ // store when user-prop conditions remain. A sort-key prefix/range
+ // query keeps sysprop conditions only (owner vertex, direction,
+ // label, sort values), which are already enforced by the key
+ // range, and the store-side row decoder cannot parse the raw
+ // property layout written by the server (see issue #3090).
+ ConditionQuery cq = prepareConditionQuery((ConditionQuery) origin);
Review Comment:
⚠️ The guard lands here, but the same unguarded pushdown survives in
`queryAll()` (lines 448-469), which pushes `((ConditionQuery) query).bytes()`
on both exits with no guard at all. Unlike the `prepareConditionQueryList()`
path discussed earlier on this PR, that one has a live caller: `queryBy()`
(line 405) -> `queryByCond()` (line 707) -> `queryAll()`.
Reachability for a sysprop-only condition query such as
`g.V().hasLabel('person')` or `g.E().hasLabel('flow')`:
`BinarySerializer.writeQueryEdgePrefixCondition()`
(`BinarySerializer.java:721-763`) returns `null` when `OWNER_VERTEX` is absent
(the loop breaks on the first key, `count == 0`), and `writeQueryCondition()`
(line 765-769) returns the query unchanged for non-index types, so a plain
`ConditionQuery` arrives at `queryBy()` with conditions but no id, prefix or
range and falls through to `queryAll()`. `HstoreSessionsImpl` then hands those
bytes straight to the store (`scan(String, byte[])` at line 582-588, paging
overload at 710-729), i.e. the same `FilterIterator` ->
`parseEdge`/`parseVertex` decoder this PR is avoiding. The shard scan
`queryByRange(Session, Shard, ConditionQuery)` (line 710-728) calls
`query.bytes()` unconditionally too.
Worth noting that `prepareConditionQuery()` cannot simply be copied to
`queryAll()`: nothing else enforces the label there, since
`GraphTransaction.rightResultFromIndexQuery()` returns `true` early for a
label-only edge `ConditionQuery` (`GraphTransaction.java:1914-1918`), so
dropping the pushdown would return unfiltered rows.
Requested change: record `queryAll()` and the shard overload explicitly in
the #3090 follow-up, and add a line to the comment above saying the guard
applies to the range path because the key range already covers its sysprops,
while the full-scan path still needs the pushdown to filter.
Confidence: the reachability trace is confirmed statically on this head. I
could not reproduce the decode failure on a live PD + HStore cluster, so the
size of the residual gap is taken from the root cause in the PR description
rather than measured.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
