bitflicker64 commented on code in PR #3184:
URL: https://github.com/apache/hugegraph/pull/3184#discussion_r3921393173
##########
hugegraph-server/hugegraph-hstore/src/main/java/org/apache/hugegraph/backend/store/hstore/HstoreTable.java:
##########
@@ -594,8 +599,11 @@ private ConditionQuery
prepareConditionQueryList(ConditionQuery conditionQuery)
}
}
if (newConditions.size() > 0) {
- conditionQuery.resetConditions(newConditions);
- return conditionQuery;
+ // NOTE: copy before reset, see prepareConditionQuery()
+ ConditionQuery pushdown = conditionQuery.copy();
+ pushdown.resetConditions(newConditions);
+ pushdown.setOriginQuery(null);
Review 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.
##########
hugegraph-server/hugegraph-hstore/src/main/java/org/apache/hugegraph/backend/store/hstore/HstoreTable.java:
##########
@@ -642,16 +650,16 @@ protected BackendColumnIterator queryByRange(Session
session,
}
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());
+ // 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).
+ cq = prepareConditionQuery((ConditionQuery) origin);
+ byte[] queryBytes = cq == null ? null : cq.bytes();
Review 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.
--
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]
