bitflicker64 commented on code in PR #3184:
URL: https://github.com/apache/hugegraph/pull/3184#discussion_r3916985818
##########
hugegraph-server/hugegraph-hstore/src/main/java/org/apache/hugegraph/backend/store/hstore/HstoreTable.java:
##########
@@ -568,8 +568,11 @@ private ConditionQuery
prepareConditionQuery(ConditionQuery conditionQuery) {
}
}
if (newConditions.size() > 0) {
- conditionQuery.resetConditions(newConditions);
- 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();
Review 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.
##########
hugegraph-server/hugegraph-hstore/src/test/java/org/apache/hugegraph/backend/store/hstore/HstoreTableTest.java:
##########
@@ -124,6 +132,59 @@ public void
testRangeScanBudgetIncludesOneLookaheadRecord() {
Assert.assertEquals(14L, HstoreTable.rangeScanBudget(query));
}
+ @Test
+ public void testRangeQueryWithoutUserpropsDoesNotPushConditions() {
+ // Sort-key prefix/range queries keep sysprop conditions only (owner
+ // vertex, direction, label, sort values); those are enforced by the
+ // key range already and must not be pushed to the store, whose row
+ // decoder cannot parse the server's raw property layout (issue #3090)
+ ConditionQuery origin = new ConditionQuery(HugeType.EDGE);
+ origin.eq(HugeKeys.OWNER_VERTEX, IdGenerator.of("v1"));
+ origin.eq(HugeKeys.DIRECTION, Directions.OUT);
+ origin.eq(HugeKeys.LABEL, IdGenerator.of(1L));
+ origin.gte(HugeKeys.SORT_VALUES, "ETC!");
+ origin.lt(HugeKeys.SORT_VALUES, "ETC~");
+ int before = origin.conditions().size();
+
+ ScanRecordingSession session = new ScanRecordingSession();
+ this.newTestTable().queryByRange(session, edgeRangeQuery(origin));
+
+ Assert.assertTrue(session.scanCalled);
+ Assert.assertNull(session.lastQueryBytes);
+ Assert.assertEquals(before, origin.conditions().size());
+ }
+
+ @Test
+ public void testRangeQueryWithUserpropsPushesCopyAndKeepsOrigin() {
+ ConditionQuery origin = new ConditionQuery(HugeType.EDGE);
+ origin.eq(HugeKeys.OWNER_VERTEX, IdGenerator.of("v1"));
+ origin.query(Condition.eq(IdGenerator.of(7L), 100));
+ int before = origin.conditions().size();
+
+ ScanRecordingSession session = new ScanRecordingSession();
+ this.newTestTable().queryByRange(session, edgeRangeQuery(origin));
+
+ Assert.assertTrue(session.scanCalled);
+ Assert.assertNotNull(session.lastQueryBytes);
Review 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.
##########
hugegraph-server/hugegraph-hstore/src/main/java/org/apache/hugegraph/backend/store/hstore/HstoreTable.java:
##########
@@ -594,8 +597,10 @@ private ConditionQuery
prepareConditionQueryList(ConditionQuery conditionQuery)
}
}
if (newConditions.size() > 0) {
- conditionQuery.resetConditions(newConditions);
- return conditionQuery;
+ // NOTE: copy before reset, see prepareConditionQuery()
+ ConditionQuery pushdown = conditionQuery.copy();
Review Comment:
⚠️ The copy fix lands here, but the entry guard of this method is still
`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.
--
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]
