Skip to content

Commit 973edf5

Browse files
expaniexpani
committed
Fixed some boundary conditions and increased coverage
Signed-off-by: expani <[email protected]>
1 parent b85183d commit 973edf5

File tree

6 files changed

+95
-79
lines changed

6 files changed

+95
-79
lines changed

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/Dimension.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,5 +45,4 @@ public interface Dimension extends ToXContent {
4545
List<String> getSubDimensionNames();
4646

4747
DocValuesType getDocValuesType();
48-
4948
}

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/fileformats/node/FixedLengthStarTreeNode.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -328,8 +328,10 @@ private FixedLengthStarTreeNode binarySearchChild(long dimensionValue, boolean m
328328
if (lastMatchedNode instanceof FixedLengthStarTreeNode) {
329329
int lastMatchedNodeId = ((FixedLengthStarTreeNode) lastMatchedNode).nodeId();
330330
// Start the binary search from node after the last matched as low.
331-
if ((lastMatchedNodeId + 1) <= high) {
332-
low = lastMatchedNodeId + 1;
331+
if ((lastMatchedNodeId + 1) <= tempHigh) {
332+
tempLow = lastMatchedNodeId + 1;
333+
} else {
334+
return null;
333335
}
334336
}
335337

‎server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,8 @@ public Optional<Long> getMatchingOrdinal(
340340
SortedSetStarTreeValuesIterator sortedSetIterator = (SortedSetStarTreeValuesIterator) genericIterator;
341341
try {
342342
if (matchType == DimensionFilter.MatchType.EXACT) {
343-
return Optional.of(sortedSetIterator.lookupTerm((BytesRef) value));
343+
long ordinal = sortedSetIterator.lookupTerm((BytesRef) value);
344+
return ordinal >= 0 ? Optional.of(ordinal) : Optional.empty();
344345
} else {
345346
TermsEnum termsEnum = sortedSetIterator.termsEnum();
346347
TermsEnum.SeekStatus seekStatus = termsEnum.seekCeil((BytesRef) value);

‎server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, Composit
3838

3939
class SingletonFactory {
4040

41-
// TODO : Implement MatchAll and MatchNone Queries
4241
private static final Map<Class<? extends QueryBuilder>, StarTreeFilterProvider> QUERY_BUILDERS_TO_STF_PROVIDER = Map.of(
4342
MatchAllQueryBuilder.class,
4443
MATCH_ALL_PROVIDER,

‎server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java

Lines changed: 85 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
import org.apache.lucene.codecs.Codec;
1616
import org.apache.lucene.codecs.lucene101.Lucene101Codec;
1717
import org.apache.lucene.document.Document;
18+
import org.apache.lucene.document.Field;
19+
import org.apache.lucene.document.LongField;
1820
import org.apache.lucene.document.SortedNumericDocValuesField;
1921
import org.apache.lucene.index.DirectoryReader;
2022
import org.apache.lucene.index.IndexWriterConfig;
@@ -44,6 +46,7 @@
4446
import org.opensearch.index.mapper.MapperService;
4547
import org.opensearch.index.mapper.NumberFieldMapper;
4648
import org.opensearch.index.query.QueryBuilder;
49+
import org.opensearch.index.query.QueryBuilders;
4750
import org.opensearch.index.query.QueryShardContext;
4851
import org.opensearch.index.query.TermQueryBuilder;
4952
import org.opensearch.search.aggregations.AggregationBuilder;
@@ -130,11 +133,11 @@ public void testStarTreeDocValues() throws IOException {
130133
Document doc = new Document();
131134
if (random.nextBoolean()) {
132135
val = random.nextInt(10) - 5; // Random long between -5 and 4
133-
doc.add(new SortedNumericDocValuesField(SNDV, val));
136+
doc.add(new LongField(SNDV, val, Field.Store.YES));
134137
}
135138
if (random.nextBoolean()) {
136139
val = random.nextInt(20) - 10; // Random long between -10 and 9
137-
doc.add(new SortedNumericDocValuesField(DV, val));
140+
doc.add(new LongField(DV, val, Field.Store.YES));
138141
}
139142
if (random.nextBoolean()) {
140143
val = random.nextInt(50); // Random long between 0 and 49
@@ -157,6 +160,18 @@ public void testStarTreeDocValues() throws IOException {
157160
IndexSearcher indexSearcher = newSearcher(reader, false, false);
158161
CompositeIndexReader starTreeDocValuesReader = (CompositeIndexReader) reader.getDocValuesReader();
159162

163+
MapperService mapperService = mapperServiceMock();
164+
CircuitBreakerService circuitBreakerService = new NoneCircuitBreakerService();
165+
when(mapperService.fieldType(SNDV)).thenReturn(new NumberFieldMapper.NumberFieldType(SNDV, NumberFieldMapper.NumberType.LONG));
166+
when(mapperService.fieldType(DV)).thenReturn(new NumberFieldMapper.NumberFieldType(DV, NumberFieldMapper.NumberType.LONG));
167+
QueryShardContext queryShardContext = queryShardContextMock(
168+
indexSearcher,
169+
mapperService,
170+
createIndexSettings(),
171+
circuitBreakerService,
172+
new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), circuitBreakerService).withCircuitBreaking()
173+
);
174+
160175
List<CompositeIndexFieldInfo> compositeIndexFields = starTreeDocValuesReader.getCompositeIndexFields();
161176
CompositeIndexFieldInfo starTree = compositeIndexFields.get(0);
162177

@@ -173,6 +188,7 @@ public void testStarTreeDocValues() throws IOException {
173188
Query query = new MatchAllDocsQuery();
174189
// match-all query
175190
QueryBuilder queryBuilder = null; // no predicates
191+
QueryBuilder rangeQueryBuilder = null;
176192
testCase(
177193
indexSearcher,
178194
query,
@@ -222,75 +238,70 @@ public void testStarTreeDocValues() throws IOException {
222238
// Numeric-terms query
223239
for (int cases = 0; cases < 100; cases++) {
224240
String queryField;
225-
long queryValue;
241+
long queryLow, queryHigh;
226242
if (randomBoolean()) {
227243
queryField = SNDV;
228-
queryValue = random.nextInt(10);
244+
queryLow = random.nextInt(10);
229245
} else {
230246
queryField = DV;
231-
queryValue = random.nextInt(20) - 15;
247+
queryLow = random.nextInt(20) - 15;
248+
}
249+
queryHigh = random.nextInt(10);
250+
251+
query = SortedNumericDocValuesField.newSlowExactQuery(queryField, queryLow);
252+
queryBuilder = new TermQueryBuilder(queryField, queryLow);
253+
rangeQueryBuilder = QueryBuilders.rangeQuery(queryField).gte(queryLow).lte(queryHigh);
254+
255+
for (QueryBuilder qb : new QueryBuilder[] { queryBuilder, rangeQueryBuilder }) {
256+
query = qb.toQuery(queryShardContext);
257+
testCase(
258+
indexSearcher,
259+
query,
260+
qb,
261+
sumAggregationBuilder,
262+
starTree,
263+
supportedDimensions,
264+
verifyAggregation(InternalSum::getValue)
265+
);
266+
testCase(
267+
indexSearcher,
268+
query,
269+
qb,
270+
maxAggregationBuilder,
271+
starTree,
272+
supportedDimensions,
273+
verifyAggregation(InternalMax::getValue)
274+
);
275+
testCase(
276+
indexSearcher,
277+
query,
278+
qb,
279+
minAggregationBuilder,
280+
starTree,
281+
supportedDimensions,
282+
verifyAggregation(InternalMin::getValue)
283+
);
284+
testCase(
285+
indexSearcher,
286+
query,
287+
qb,
288+
valueCountAggregationBuilder,
289+
starTree,
290+
supportedDimensions,
291+
verifyAggregation(InternalValueCount::getValue)
292+
);
293+
testCase(
294+
indexSearcher,
295+
query,
296+
qb,
297+
avgAggregationBuilder,
298+
starTree,
299+
supportedDimensions,
300+
verifyAggregation(InternalAvg::getValue)
301+
);
232302
}
233-
234-
query = SortedNumericDocValuesField.newSlowExactQuery(queryField, queryValue);
235-
queryBuilder = new TermQueryBuilder(queryField, queryValue);
236-
237-
testCase(
238-
indexSearcher,
239-
query,
240-
queryBuilder,
241-
sumAggregationBuilder,
242-
starTree,
243-
supportedDimensions,
244-
verifyAggregation(InternalSum::getValue)
245-
);
246-
testCase(
247-
indexSearcher,
248-
query,
249-
queryBuilder,
250-
maxAggregationBuilder,
251-
starTree,
252-
supportedDimensions,
253-
verifyAggregation(InternalMax::getValue)
254-
);
255-
testCase(
256-
indexSearcher,
257-
query,
258-
queryBuilder,
259-
minAggregationBuilder,
260-
starTree,
261-
supportedDimensions,
262-
verifyAggregation(InternalMin::getValue)
263-
);
264-
testCase(
265-
indexSearcher,
266-
query,
267-
queryBuilder,
268-
valueCountAggregationBuilder,
269-
starTree,
270-
supportedDimensions,
271-
verifyAggregation(InternalValueCount::getValue)
272-
);
273-
testCase(
274-
indexSearcher,
275-
query,
276-
queryBuilder,
277-
avgAggregationBuilder,
278-
starTree,
279-
supportedDimensions,
280-
verifyAggregation(InternalAvg::getValue)
281-
);
282303
}
283304

284-
CircuitBreakerService circuitBreakerService = new NoneCircuitBreakerService();
285-
286-
QueryShardContext queryShardContext = queryShardContextMock(
287-
indexSearcher,
288-
mapperServiceMock(),
289-
createIndexSettings(),
290-
circuitBreakerService,
291-
new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), circuitBreakerService).withCircuitBreaking()
292-
);
293-
294305
MetricAggregatorFactory aggregatorFactory = mock(MetricAggregatorFactory.class);
295306
when(aggregatorFactory.getSubFactories()).thenReturn(AggregatorFactories.EMPTY);
296307
when(aggregatorFactory.getField()).thenReturn(FIELD_NAME);
@@ -300,7 +311,7 @@ public void testStarTreeDocValues() throws IOException {
300311
testCase(
301312
indexSearcher,
302313
query,
303-
queryBuilder,
314+
rangeQueryBuilder,
304315
sumAggregationBuilder,
305316
starTree,
306317
supportedDimensions,
@@ -315,7 +326,7 @@ public void testStarTreeDocValues() throws IOException {
315326
testCase(
316327
indexSearcher,
317328
query,
318-
queryBuilder,
329+
rangeQueryBuilder,
319330
invalidFieldSumAggBuilder,
320331
starTree,
321332
supportedDimensions,
@@ -329,7 +340,7 @@ public void testStarTreeDocValues() throws IOException {
329340
testCase(
330341
indexSearcher,
331342
query,
332-
queryBuilder,
343+
rangeQueryBuilder,
333344
sumAggregationBuilder,
334345
starTree,
335346
supportedDimensions,
@@ -343,7 +354,7 @@ public void testStarTreeDocValues() throws IOException {
343354
testCase(
344355
indexSearcher,
345356
query,
346-
queryBuilder,
357+
rangeQueryBuilder,
347358
sumAggregationBuilder,
348359
starTree,
349360
supportedDimensions,
@@ -361,7 +372,7 @@ public void testStarTreeDocValues() throws IOException {
361372
testCase(
362373
indexSearcher,
363374
query,
364-
queryBuilder,
375+
rangeQueryBuilder,
365376
sumAggregationBuilder,
366377
starTree,
367378
supportedDimensions,
@@ -375,7 +386,7 @@ public void testStarTreeDocValues() throws IOException {
375386
testCase(
376387
indexSearcher,
377388
query,
378-
queryBuilder,
389+
rangeQueryBuilder,
379390
sumAggregationBuilder,
380391
starTree,
381392
supportedDimensions,
@@ -389,6 +400,10 @@ public void testStarTreeDocValues() throws IOException {
389400
directory.close();
390401
}
391402

403+
public void testStarTreeDocValues2() throws IOException {
404+
405+
}
406+
392407
<T, R extends Number> BiConsumer<T, T> verifyAggregation(Function<T, R> valueExtractor) {
393408
return (expectedAggregation, actualAggregation) -> assertEquals(
394409
valueExtractor.apply(expectedAggregation).doubleValue(),

‎test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,6 @@
6262
import org.apache.lucene.tests.search.AssertingIndexSearcher;
6363
import org.apache.lucene.util.BytesRef;
6464
import org.apache.lucene.util.NumericUtils;
65-
import org.junit.After;
66-
import org.junit.Before;
6765
import org.opensearch.Version;
6866
import org.opensearch.cluster.metadata.IndexMetadata;
6967
import org.opensearch.common.CheckedConsumer;
@@ -153,6 +151,8 @@
153151
import org.opensearch.search.startree.StarTreeQueryContext;
154152
import org.opensearch.test.InternalAggregationTestCase;
155153
import org.opensearch.test.OpenSearchTestCase;
154+
import org.junit.After;
155+
import org.junit.Before;
156156

157157
import java.io.IOException;
158158
import java.net.InetAddress;
@@ -173,14 +173,14 @@
173173
import static java.util.Collections.emptyMap;
174174
import static java.util.Collections.singletonList;
175175
import static java.util.Collections.singletonMap;
176+
import static org.opensearch.test.InternalAggregationTestCase.DEFAULT_MAX_BUCKETS;
176177
import static org.hamcrest.Matchers.equalTo;
177178
import static org.hamcrest.Matchers.instanceOf;
178179
import static org.mockito.Mockito.any;
179180
import static org.mockito.Mockito.anyString;
180181
import static org.mockito.Mockito.doAnswer;
181182
import static org.mockito.Mockito.mock;
182183
import static org.mockito.Mockito.when;
183-
import static org.opensearch.test.InternalAggregationTestCase.DEFAULT_MAX_BUCKETS;
184184

185185
/**
186186
* Base class for testing {@link Aggregator} implementations.
@@ -439,7 +439,7 @@ protected SearchContext createSearchContextWithStarTreeContext(
439439
NumberFieldMapper.NumberFieldType mappedFieldType = mock(NumberFieldMapper.NumberFieldType.class);
440440
when(mappedFieldType.name()).thenReturn(dimension.getField());
441441
// TODO : Number type should be supplied as parameter to create function
442-
when(mappedFieldType.numberType()).thenReturn(NumberFieldMapper.NumberType.INTEGER);
442+
when(mappedFieldType.numberType()).thenReturn(NumberFieldMapper.NumberType.LONG);
443443
when(mapperService.fieldType(dimension.getField())).thenReturn(mappedFieldType);
444444
when(searchContext.getQueryShardContext().fieldMapper(dimension.getField())).thenReturn(mappedFieldType);
445445
}

0 commit comments

Comments
 (0)