Skip to content

fix(hstore): don't push sysprop-only range queries down to the store #3184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SebastianGruza wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(hstore): don't push sysprop-only range queries down to the store #3184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
847fab9
fix(hstore): don't push sysprop-only range queries down to the store
SebastianGruza Sep 2, 2026
1072872
fix(hstore): clear pushdown back reference; assert pushed payload con…
SebastianGruza Sep 2, 2026
0ecc10a
fix(hstore): single scan exit in queryByRange; cover prepareCondition…
SebastianGruza Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions ...hugegraph-hstore/src/main/java/org/apache/hugegraph/backend/store/hstore/HstoreTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,8 +568,13 @@ 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; drop the
// back reference so the serialized payload stays flat
ConditionQuery pushdown = conditionQuery.copy();

@bitflicker64 bitflicker64 Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

@SebastianGruza SebastianGruza Sep 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pushdown.resetConditions(newConditions);
pushdown.setOriginQuery(null);
return pushdown;
} else {
return null;
}
Expand All @@ -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();

@bitflicker64 bitflicker64 Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

@SebastianGruza SebastianGruza Sep 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pushdown.resetConditions(newConditions);
pushdown.setOriginQuery(null);

@bitflicker64 bitflicker64 Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

@SebastianGruza SebastianGruza Sep 3, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return pushdown;
} else {
return null;
}
Expand Down Expand Up @@ -623,7 +631,6 @@ protected BackendColumnIterator queryByRange(Session session,
type |= query.inclusiveEnd() ?
Session.SCAN_LTE_END : Session.SCAN_LT_END;
}
ConditionQuery cq;
Query origin = query.originQuery();
byte[] position = null;
byte[] ownerStart = this.ownerByQueryDelegate.apply(query.resultType(),
Expand All @@ -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);
queryBytes = cq == null ? null : cq.bytes();
}
return session.scan(this.table(), ownerStart, ownerEnd, start, end,
type, queryBytes, position);
}

static boolean shouldUseOrderedRangeScan(IdRangeQuery query) {
Expand Down
Loading