Skip to content

Commit

Permalink
fix off by one error in FrontCodedIndexedWriter and FrontCodedIntArra…
Browse files Browse the repository at this point in the history 
…yIndexedWriter getCardinality method (#14047)

* fix off by one error in FrontCodedIndexedWriter and FrontCodedIntArrayIndexedWriter getCardinality method
  • Loading branch information
@clintropolis
clintropolis authored Apr 7, 2023
1 parent f47b05a commit f41468f
Show file tree
Hide file tree
Showing 8 changed files with 105 additions and 26 deletions.
2 changes: 1 addition & 1 deletion processing/src/main/java/org/apache/druid/segment/data/FrontCodedIndexedWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ public byte[] get(int index) throws IOException
@Override
public int getCardinality()
{
return numWritten;
return numWritten + (hasNulls ? 1 : 0);
}

private long getBucketOffset(int index) throws IOException
Expand Down
2 changes: 1 addition & 1 deletion processing/src/main/java/org/apache/druid/segment/data/FrontCodedIntArrayIndexedWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ public int[] get(int index) throws IOException
@Override
public int getCardinality()
{
return numWritten;
return numWritten + (hasNulls ? 1 : 0);
}

private long getBucketOffset(int index) throws IOException
Expand Down
103 changes: 86 additions & 17 deletions processing/src/test/java/org/apache/druid/query/NestedDataTestUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@
import org.apache.druid.segment.AutoTypeColumnSchema;
import org.apache.druid.segment.IncrementalIndexSegment;
import org.apache.druid.segment.IndexBuilder;
import org.apache.druid.segment.IndexSpec;
import org.apache.druid.segment.QueryableIndexSegment;
import org.apache.druid.segment.Segment;
import org.apache.druid.segment.TestHelper;
import org.apache.druid.segment.column.StringEncodingStrategy;
import org.apache.druid.segment.incremental.IncrementalIndexSchema;
import org.apache.druid.segment.transform.ExpressionTransform;
import org.apache.druid.segment.transform.TransformSpec;
Expand Down Expand Up @@ -180,7 +182,8 @@ public static List<Segment> createSimpleNestedTestDataTsvSegments(
SIMPLE_DATA_TSV_TRANSFORM,
COUNT,
granularity,
rollup
rollup,
new IndexSpec()
);
}

Expand All @@ -205,7 +208,8 @@ public static List<Segment> createSimpleNestedTestDataSegments(
closer,
SIMPLE_DATA_FILE,
Granularities.NONE,
true
true,
new IndexSpec()
);
}

Expand Down Expand Up @@ -246,7 +250,8 @@ public static List<Segment> createSegmentsForJsonInput(
Closer closer,
String inputFile,
Granularity granularity,
boolean rollup
boolean rollup,
IndexSpec indexSpec
) throws Exception
{
return createSegments(
Expand All @@ -259,7 +264,8 @@ public static List<Segment> createSegmentsForJsonInput(
TransformSpec.NONE,
COUNT,
granularity,
rollup
rollup,
indexSpec
);
}

Expand Down Expand Up @@ -288,14 +294,16 @@ public static List<Segment> createSegmentsWithConcatenatedJsonInput(
TransformSpec.NONE,
COUNT,
granularity,
rollup
rollup,
new IndexSpec()
);
}

public static List<Segment> createSegmentsForJsonInput(
TemporaryFolder tempFolder,
Closer closer,
String inputFile
String inputFile,
IndexSpec indexSpec
)
throws Exception
{
Expand All @@ -304,7 +312,8 @@ public static List<Segment> createSegmentsForJsonInput(
closer,
inputFile,
Granularities.NONE,
true
true,
indexSpec
);
}

Expand Down Expand Up @@ -355,7 +364,8 @@ public static List<Segment> createSegments(
TransformSpec transformSpec,
AggregatorFactory[] aggregators,
Granularity queryGranularity,
boolean rollup
boolean rollup,
IndexSpec indexSpec
) throws Exception
{
return createSegments(
Expand All @@ -368,7 +378,8 @@ public static List<Segment> createSegments(
transformSpec,
aggregators,
queryGranularity,
rollup
rollup,
indexSpec
);
}

Expand All @@ -382,7 +393,8 @@ public static List<Segment> createSegments(
TransformSpec transformSpec,
AggregatorFactory[] aggregators,
Granularity queryGranularity,
boolean rollup
boolean rollup,
IndexSpec indexSpec
) throws Exception
{
final List<Segment> segments = Lists.newArrayListWithCapacity(inputs.size());
Expand All @@ -400,6 +412,7 @@ public static List<Segment> createSegments(
.withMinTimestamp(0)
.build()
)
.indexSpec(indexSpec)
.inputSource(inputSource)
.inputFormat(inputFormat)
.transform(transformSpec)
Expand Down Expand Up @@ -464,7 +477,8 @@ public List<Segment> apply(TemporaryFolder tempFolder, Closer closer)
NestedDataTestUtils.createSegmentsForJsonInput(
tempFolder,
closer,
jsonInputFile
jsonInputFile,
new IndexSpec()
)
)
.add(NestedDataTestUtils.createIncrementalIndexForJsonInput(tempFolder, jsonInputFile))
Expand Down Expand Up @@ -514,14 +528,17 @@ public List<Segment> apply(TemporaryFolder tempFolder, Closer closer)
NestedDataTestUtils.createSegmentsForJsonInput(
tempFolder,
closer,
jsonInputFile
jsonInputFile,
new IndexSpec()
)
)
.addAll(NestedDataTestUtils.createSegmentsForJsonInput(
tempFolder,
closer,
jsonInputFile
)
.addAll(
NestedDataTestUtils.createSegmentsForJsonInput(
tempFolder,
closer,
jsonInputFile,
new IndexSpec()
)
)
.build();
}
Expand All @@ -536,6 +553,58 @@ public String toString()
return "segments";
}
});
segmentsGenerators.add(new BiFunction<TemporaryFolder, Closer, List<Segment>>()
{
@Override
public List<Segment> apply(TemporaryFolder tempFolder, Closer closer)
{
try {
return ImmutableList.<Segment>builder()
.addAll(
NestedDataTestUtils.createSegmentsForJsonInput(
tempFolder,
closer,
jsonInputFile,
new IndexSpec(
null,
null,
new StringEncodingStrategy.FrontCoded(4, (byte) 0x01),
null,
null,
null,
null
)
)
)
.addAll(
NestedDataTestUtils.createSegmentsForJsonInput(
tempFolder,
closer,
jsonInputFile,
new IndexSpec(
null,
null,
new StringEncodingStrategy.FrontCoded(4, (byte) 0x00),
null,
null,
null,
null
)
)
)
.build();
}
catch (Exception e) {
throw new RuntimeException(e);
}
}

@Override
public String toString()
{
return "segments-frontcoded";
}
});
return segmentsGenerators;
}
}
2 changes: 1 addition & 1 deletion processing/src/test/java/org/apache/druid/query/groupby/NestedDataGroupByQueryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ private void runResults(
return;
}
}
if (!"segments".equals(segmentsName)) {
if (!"segments".equals(segmentsName) && !"segments-frontcoded".equals(segmentsName)) {
if (GroupByStrategySelector.STRATEGY_V1.equals(config.getDefaultStrategy())) {
Throwable t = Assert.assertThrows(RuntimeException.class, runner::get);
Assert.assertEquals(
Expand Down
13 changes: 9 additions & 4 deletions processing/src/test/java/org/apache/druid/query/scan/NestedDataScanQueryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.apache.druid.query.filter.SelectorDimFilter;
import org.apache.druid.query.ordering.StringComparators;
import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
import org.apache.druid.segment.IndexSpec;
import org.apache.druid.segment.Segment;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.transform.TransformSpec;
Expand Down Expand Up @@ -135,7 +136,8 @@ public void testIngestAndScanSegmentsRollup() throws Exception
TransformSpec.NONE,
NestedDataTestUtils.COUNT,
Granularities.YEAR,
true
true,
new IndexSpec()
)
).build();

Expand Down Expand Up @@ -308,7 +310,8 @@ public void testIngestWithMergesAndScanSegments() throws Exception
closer,
NestedDataTestUtils.SIMPLE_DATA_FILE,
Granularities.HOUR,
true
true,
new IndexSpec()
);
final Sequence<ScanResultValue> seq = helper.runQueryOnSegmentsObjs(segs, scanQuery);

Expand Down Expand Up @@ -491,7 +494,8 @@ public void testIngestAndScanSegmentsRealtimeSchemaDiscovery() throws Exception
TransformSpec.NONE,
NestedDataTestUtils.COUNT,
Granularities.DAY,
true
true,
new IndexSpec()
);


Expand Down Expand Up @@ -550,7 +554,8 @@ public void testIngestAndScanSegmentsRealtimeSchemaDiscoveryArrayTypes() throws
TransformSpec.NONE,
aggs,
Granularities.NONE,
true
true,
new IndexSpec()
);


Expand Down
1 change: 1 addition & 0 deletions processing/src/test/java/org/apache/druid/segment/data/FrontCodedIndexedTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@ private static long persistToBuffer(
}
index++;
}
Assert.assertEquals(index, writer.getCardinality());

// check 'get' again so that we aren't always reading from current page
index = 0;
Expand Down
1 change: 1 addition & 0 deletions processing/src/test/java/org/apache/druid/segment/data/FrontCodedIntArrayIndexedTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,7 @@ private static long persistToBuffer(ByteBuffer buffer, Iterable<int[]> sortedIte
assertSame(index, next, writer.get(index));
index++;
}
Assert.assertEquals(index, writer.getCardinality());

// check 'get' again so that we aren't always reading from current page
index = 0;
Expand Down
7 changes: 5 additions & 2 deletions processing/src/test/java/org/apache/druid/segment/nested/NestedFieldColumnSelectorsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.apache.druid.segment.ColumnValueSelector;
import org.apache.druid.segment.Cursor;
import org.apache.druid.segment.DoubleColumnSelector;
import org.apache.druid.segment.IndexSpec;
import org.apache.druid.segment.LongColumnSelector;
import org.apache.druid.segment.Segment;
import org.apache.druid.segment.StorageAdapter;
Expand Down Expand Up @@ -336,7 +337,8 @@ private ColumnSelectorFactory getNumericColumnSelectorFactory(VirtualColumns vir
TransformSpec.NONE,
NestedDataTestUtils.COUNT,
Granularities.NONE,
true
true,
new IndexSpec()
);
Assert.assertEquals(1, segments.size());
StorageAdapter storageAdapter = segments.get(0).asStorageAdapter();
Expand Down Expand Up @@ -366,7 +368,8 @@ private VectorColumnSelectorFactory getVectorColumnSelectorFactory(VirtualColumn
TransformSpec.NONE,
NestedDataTestUtils.COUNT,
Granularities.NONE,
true
true,
new IndexSpec()
);
Assert.assertEquals(1, segments.size());
StorageAdapter storageAdapter = segments.get(0).asStorageAdapter();
Expand Down

0 comments on commit f41468f

Please sign in to comment.