imbajin commented on code in PR #3140:
URL: https://github.com/apache/hugegraph/pull/3140#discussion_r3731208410
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/page/QueryList.java:
##########
@@ -258,7 +258,7 @@ private QueryResults<R> each(IdHolder holder) {
return null;
}
- return this.queryByIndexIds(ids);
+ return this.queryByIndexIds(ids, holder.keepOrder());
Review Comment:
⚠️ important — `keepOrder` is propagated to each batch `IdQuery`, but
`QueryResults.keepInputOrderIfNeeded()` snapshots `queryIds()` before the lazy
`FlatMapperIterator` has materialized later batches. When the index result
exceeds `QUERY_BATCH`, `ids` contains only the first batch; after the map is
filled, `map.size() > ids.size()` falls back to `map.keySet()`, which is
backend fetch order. The new ordering path can therefore return multi-batch
range-index results out of input order. Please accumulate all batch IDs before
ordering and add a regression test with more than `QUERY_BATCH` IDs.
##########
hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/NodeTxSessionProxy.java:
##########
@@ -748,6 +807,44 @@ private List<NodeTkv> toNodeTkvList(String table,
HgOwnerKey startKey, HgOwnerKe
return nodeTkvs;
}
+ private List<NodeTkv> toOrderedRangeNodeTkvList(String table,
+ HgOwnerKey startKey,
+ HgOwnerKey endKey) {
+ byte[] allOwner = HgStoreClientConst.ALL_PARTITION_OWNER;
+ Collection<HgNodePartition> partitions =
+ this.doPartition(table, allOwner, allOwner);
+ List<NodeTkv> nodeTkvs = new ArrayList<>(partitions.size());
+ for (HgNodePartition partition : partitions) {
+ nodeTkvs.add(new NodeTkv(partition, table, startKey, endKey));
+ }
+ return nodeTkvs;
+ }
+
+ private Builder orderedRangeScanBuilder(NodeTkv nodeTkv, long limit,
+ int scanType, byte[] query) {
+ long scanLimit = limit <= HgStoreClientConst.NO_LIMIT ?
+ Integer.MAX_VALUE : limit;
+ return ScanStreamReq.newBuilder()
+ .setHeader(Header.newBuilder()
+ .setGraph(this.graphName)
+ .build())
+ .setMethod(ScanMethod.RANGE)
+ .setTable(nodeTkv.getTable())
+ .setStart(toByteString(nodeTkv.getKey().getKey()))
+ .setEnd(toByteString(nodeTkv.getEndKey().getKey()))
+ .setLimit(scanLimit)
+ .setCode(nodeTkv.getKey().getKeyCode())
+ .setScanType(scanType)
+ .setPageSize(ORDERED_SCAN_PAGE_SIZE)
+ .setOrderType(ScanOrderType.ORDER_BY_KEY)
Review Comment:
⚠️ important — This sends `ORDER_BY_KEY` as an optional protobuf field, but
an older Store silently ignores unknown field 15 and processes the request via
the legacy all-partition scan, which concatenates partitions rather than
sorting by key. `OrderedKvIterator` then heap-merges an unsorted source and can
return incorrect global order/limit/page results during rolling upgrades.
Please negotiate support or fail closed with an operation old nodes reject, and
add a mixed-version test.
--
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]
