Skip to content

Commit e497efd

Browse files
fix(store): stop ProjectionStage clearing dedup keys in place (#3408)
MapLimitIterator keeps each emitted RocksDBSession.BackendColumn as a key in its dedup set and passes the same object downstream. On an id-only scan with every property removed, ProjectionStage then set column.value to an empty array in place, so the stored key no longer equalled a later byte-equal row and the copy was emitted again: 200 rows for 100 keys read twice. - replace the projected column instead of clearing value on the shared object - keep the dedup key MapLimitIterator holds unchanged - cover both with ProjectionStageTest in ServerSuiteTest Fixes #3407
1 parent 608cb1d commit e497efd

3 files changed

Lines changed: 141 additions & 1 deletion

File tree

‎hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/grpc/query/stages/ProjectionStage.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.util.Set;
2323

2424
import org.apache.hugegraph.id.Id;
25+
import org.apache.hugegraph.rocksdb.access.RocksDBSession;
2526
import org.apache.hugegraph.store.node.grpc.query.QueryStage;
2627
import org.apache.hugegraph.store.node.grpc.query.QueryUtil;
2728
import org.apache.hugegraph.store.node.grpc.query.model.PipelineResult;
@@ -60,8 +61,10 @@ public PipelineResult handle(PipelineResult result) {
6061
return result;
6162
} else if (result.getResultType() == PipelineResultType.BACKEND_COLUMN &&
6263
this.removeAllProperty) {
64+
// Replace the column rather than clearing it in place: an upstream dedup set, such as
65+
// MapLimitIterator's, may still hold this object as a key.
6366
var column = result.getColumn();
64-
column.value = new byte[0];
67+
result.setColumn(RocksDBSession.BackendColumn.of(column.name, new byte[0]));
6568
}
6669
return result;
6770
}
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
18+
package org.apache.hugegraph.store.node.grpc.query;
19+
20+
import static org.junit.Assert.assertArrayEquals;
21+
import static org.junit.Assert.assertEquals;
22+
import static org.junit.Assert.assertSame;
23+
import static org.junit.Assert.assertTrue;
24+
25+
import java.nio.ByteBuffer;
26+
import java.util.ArrayList;
27+
import java.util.HashSet;
28+
import java.util.Iterator;
29+
import java.util.List;
30+
import java.util.Set;
31+
import java.util.stream.Collectors;
32+
import java.util.stream.IntStream;
33+
34+
import org.apache.hugegraph.rocksdb.access.RocksDBSession.BackendColumn;
35+
import org.apache.hugegraph.rocksdb.access.ScanIterator;
36+
import org.apache.hugegraph.store.business.itrv2.MapLimitIterator;
37+
import org.apache.hugegraph.store.node.grpc.query.model.PipelineResult;
38+
import org.apache.hugegraph.store.node.grpc.query.stages.ProjectionStage;
39+
import org.junit.Test;
40+
41+
/**
42+
* MapLimitIterator keeps every column it emits as a key of its dedup set, so a later stage must
43+
* not change that column in place. An id-only scan runs each emitted row through ProjectionStage
44+
* with every property removed before the next row is read.
45+
*/
46+
public class ProjectionStageTest {
47+
48+
private static final int KEYS = 100;
49+
50+
@Test
51+
public void testRemoveAllPropertyLeavesInputColumnUnchanged() {
52+
byte[] name = name(7);
53+
byte[] value = new byte[]{1, 2, 3};
54+
BackendColumn input = BackendColumn.of(name, value);
55+
56+
PipelineResult result = removeAllProperty().handle(new PipelineResult(input));
57+
58+
assertArrayEquals(name, result.getColumn().name);
59+
assertArrayEquals(new byte[0], result.getColumn().value);
60+
assertSame(value, input.value);
61+
assertArrayEquals(new byte[]{1, 2, 3}, input.value);
62+
}
63+
64+
@Test
65+
public void testLimitDedupStillDropsCopiesAfterProjection() {
66+
ProjectionStage projection = removeAllProperty();
67+
ScanIterator dedup = new MapLimitIterator<BackendColumn>(scanTwice());
68+
69+
Set<Integer> keys = new HashSet<>();
70+
try {
71+
while (dedup.hasNext()) {
72+
BackendColumn column = dedup.next();
73+
PipelineResult result = projection.handle(new PipelineResult(column));
74+
int key = ByteBuffer.wrap(result.getColumn().name).getInt();
75+
assertTrue("key " + key + " emitted twice", keys.add(key));
76+
}
77+
} finally {
78+
dedup.close();
79+
}
80+
81+
assertEquals(IntStream.range(0, KEYS).boxed().collect(Collectors.toSet()), keys);
82+
}
83+
84+
private static ProjectionStage removeAllProperty() {
85+
ProjectionStage stage = new ProjectionStage();
86+
stage.init(List.of(), true);
87+
return stage;
88+
}
89+
90+
/**
91+
* Each key appears twice, each time as a newly allocated column with its own arrays.
92+
*/
93+
private static ScanIterator scanTwice() {
94+
List<BackendColumn> rows = new ArrayList<>();
95+
for (int round = 0; round < 2; round++) {
96+
for (int key = 0; key < KEYS; key++) {
97+
rows.add(BackendColumn.of(name(key), name(key)));
98+
}
99+
}
100+
return new ListScanIterator(rows);
101+
}
102+
103+
private static byte[] name(int key) {
104+
return ByteBuffer.allocate(Integer.BYTES).putInt(key).array();
105+
}
106+
107+
private static class ListScanIterator implements ScanIterator {
108+
109+
private final Iterator<BackendColumn> rows;
110+
111+
private ListScanIterator(List<BackendColumn> rows) {
112+
this.rows = rows.iterator();
113+
}
114+
115+
@Override
116+
public boolean hasNext() {
117+
return this.rows.hasNext();
118+
}
119+
120+
@Override
121+
public boolean isValid() {
122+
return this.rows.hasNext();
123+
}
124+
125+
@Override
126+
@SuppressWarnings("unchecked")
127+
public <T> T next() {
128+
return (T) this.rows.next();
129+
}
130+
131+
@Override
132+
public void close() {
133+
}
134+
}
135+
}

‎hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/service/ServerSuiteTest.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.apache.hugegraph.store.node.grpc.query.FilterIteratorContractTest;
2525
import org.apache.hugegraph.store.node.grpc.query.IntersectionSpillIteratorTest;
2626
import org.apache.hugegraph.store.node.grpc.query.OrderByStageTest;
27+
import org.apache.hugegraph.store.node.grpc.query.ProjectionStageTest;
2728
import org.apache.hugegraph.store.node.grpc.query.UnaryQueryLifecycleTest;
2829
import org.apache.hugegraph.store.node.grpc.BatchScanHalfCloseTest;
2930
import org.junit.runner.RunWith;
@@ -45,6 +46,7 @@
4546
FilterIteratorContractTest.class,
4647
IntersectionSpillIteratorTest.class,
4748
OrderByStageTest.class,
49+
ProjectionStageTest.class,
4850
UnaryQueryLifecycleTest.class,
4951
BatchScanHalfCloseTest.class,
5052
ScanLifecycleTest.class,

0 commit comments

Comments
 (0)