SebastianGruza commented on code in PR #3184:
URL: https://github.com/apache/hugegraph/pull/3184#discussion_r3917443081
##########
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:
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.
##########
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:
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.
##########
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:
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.
--
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]
