Skip to content

segment metadata fallback analysis if no bitmaps #7116

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

Merged
fjy merged 6 commits into from
Feb 26, 2019
Merged

segment metadata fallback analysis if no bitmaps #7116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ddcbe16
segment metadata fallback analysis if no bitmaps
clintropolis Feb 21, 2019
098d2eb
remove accidental line
clintropolis Feb 21, 2019
d29d7a2
remove nonsense size estimation
clintropolis Feb 21, 2019
02fdf68
less ternary
clintropolis Feb 21, 2019
8e1c37a
fix it
clintropolis Feb 23, 2019
59b10db
do the thing
clintropolis Feb 25, 2019
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
47 changes: 28 additions & 19 deletions processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.apache.druid.segment.column.ColumnCapabilitiesImpl;
import org.apache.druid.segment.column.ColumnHolder;
import org.apache.druid.segment.column.ComplexColumn;
import org.apache.druid.segment.column.DictionaryEncodedColumn;
import org.apache.druid.segment.column.ValueType;
import org.apache.druid.segment.data.IndexedInts;
import org.apache.druid.segment.serde.ComplexMetricSerde;
Expand Down Expand Up @@ -194,30 +195,38 @@ private ColumnAnalysis analyzeStringColumn(
final ColumnHolder columnHolder
)
{
long size = 0;

Comparable min = null;
Comparable max = null;

if (!capabilities.hasBitmapIndexes()) {
return ColumnAnalysis.error("string_no_bitmap");
}

final BitmapIndex bitmapIndex = columnHolder.getBitmapIndex();
final int cardinality = bitmapIndex.getCardinality();

if (analyzingSize()) {
for (int i = 0; i < cardinality; ++i) {
String value = bitmapIndex.getValue(i);
if (value != null) {
size += StringUtils.estimatedBinaryLengthAsUTF8(value) * bitmapIndex.getBitmap(bitmapIndex.getIndex(value)).size();
long size = 0;
final int cardinality;
if (capabilities.hasBitmapIndexes()) {
final BitmapIndex bitmapIndex = columnHolder.getBitmapIndex();
cardinality = bitmapIndex.getCardinality();

if (analyzingSize()) {
for (int i = 0; i < cardinality; ++i) {
String value = bitmapIndex.getValue(i);
if (value != null) {
size += StringUtils.estimatedBinaryLengthAsUTF8(value) * bitmapIndex.getBitmap(bitmapIndex.getIndex(value))
.size();
}
}
}
}

if (analyzingMinMax() && cardinality > 0) {
min = NullHandling.nullToEmptyIfNeeded(bitmapIndex.getValue(0));
max = NullHandling.nullToEmptyIfNeeded(bitmapIndex.getValue(cardinality - 1));
if (analyzingMinMax() && cardinality > 0) {
min = NullHandling.nullToEmptyIfNeeded(bitmapIndex.getValue(0));
max = NullHandling.nullToEmptyIfNeeded(bitmapIndex.getValue(cardinality - 1));
}
} else if (capabilities.isDictionaryEncoded()) {
// fallback if no bitmap index
Copy link
Copy Markdown
Contributor

@gianm gianm Feb 21, 2019

Choose a reason for hiding this comment

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

I dunno, maybe we should just remove the "size" analysisType. It is nothing but trouble.

Copy link
Copy Markdown
Member Author

@clintropolis clintropolis Feb 21, 2019

Choose a reason for hiding this comment

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

Agree. I think it would be interesting to have size be representative of "estimated size in memory" for incremental index, or size in bytes of column in the segment file that is mapped, but what it's computing here seems like basically nonsense for all column types.

Copy link
Copy Markdown
Contributor

@egor-ryashin egor-ryashin Feb 21, 2019

Choose a reason for hiding this comment

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

or size in bytes of column in the segment file that is mapped

dictionary size + bitmap size I vote for that too.

Copy link
Copy Markdown
Member Author

@clintropolis clintropolis Feb 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Raised proposal #7124, which I will address in a follow up PR to not block this fix.

Copy link
Copy Markdown
Contributor

@gianm gianm Feb 25, 2019

Choose a reason for hiding this comment

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

Until #7124 is sorted, what you're doing here (setting size to 0 when there's no bitmap index) sounds good to me.

DictionaryEncodedColumn<String> theColumn = (DictionaryEncodedColumn<String>) columnHolder.getColumn();
cardinality = theColumn.getCardinality();
if (analyzingMinMax() && cardinality > 0) {
min = NullHandling.nullToEmptyIfNeeded(theColumn.lookupName(0));
max = NullHandling.nullToEmptyIfNeeded(theColumn.lookupName(cardinality - 1));
}
} else {
cardinality = 0;
}

return new ColumnAnalysis(
Expand Down
86 changes: 65 additions & 21 deletions processing/src/test/java/org/apache/druid/query/metadata/SegmentMetadataQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,16 @@ public class SegmentMetadataQueryTest
public static QueryRunner makeMMappedQueryRunner(
SegmentId segmentId,
boolean rollup,
boolean bitmaps,
QueryRunnerFactory factory
)
{
QueryableIndex index = rollup ? TestIndex.getMMappedTestIndex() : TestIndex.getNoRollupMMappedTestIndex();
QueryableIndex index;
if (bitmaps) {
index = rollup ? TestIndex.getMMappedTestIndex() : TestIndex.getNoRollupMMappedTestIndex();
} else {
index = TestIndex.getNoBitmapMMappedTestIndex();
}
return QueryRunnerTestHelper.makeQueryRunner(
factory,
segmentId,
Expand All @@ -99,10 +105,16 @@ public static QueryRunner makeMMappedQueryRunner(
public static QueryRunner makeIncrementalIndexQueryRunner(
SegmentId segmentId,
boolean rollup,
boolean bitmaps,
QueryRunnerFactory factory
)
{
IncrementalIndex index = rollup ? TestIndex.getIncrementalTestIndex() : TestIndex.getNoRollupIncrementalTestIndex();
IncrementalIndex index;
if (bitmaps) {
index = rollup ? TestIndex.getIncrementalTestIndex() : TestIndex.getNoRollupIncrementalTestIndex();
} else {
index = TestIndex.getNoBitmapIncrementalTestIndex();
}
return QueryRunnerTestHelper.makeQueryRunner(
factory,
segmentId,
Expand All @@ -121,17 +133,19 @@ public static QueryRunner makeIncrementalIndexQueryRunner(
private final SegmentMetadataQuery testQuery;
private final SegmentAnalysis expectedSegmentAnalysis1;
private final SegmentAnalysis expectedSegmentAnalysis2;
private final boolean bitmaps;

@Parameterized.Parameters(name = "mmap1 = {0}, mmap2 = {1}, rollup1 = {2}, rollup2 = {3}, differentIds = {4}")
@Parameterized.Parameters(name = "mmap1 = {0}, mmap2 = {1}, rollup1 = {2}, rollup2 = {3}, differentIds = {4}, bitmaps={5}")
public static Collection<Object[]> constructorFeeder()
{
return ImmutableList.of(
new Object[]{true, true, true, true, false},
new Object[]{true, false, true, false, false},
new Object[]{false, true, true, false, false},
new Object[]{false, false, false, false, false},
new Object[]{false, false, true, true, false},
new Object[]{false, false, false, true, true}
new Object[]{true, true, true, true, false, true},
new Object[]{true, false, true, false, false, true},
new Object[]{false, true, true, false, false, true},
new Object[]{false, false, false, false, false, true},
new Object[]{false, false, true, true, false, true},
new Object[]{false, false, false, true, true, true},
new Object[]{true, true, false, false, false, false}
);
}

Expand All @@ -140,22 +154,24 @@ public SegmentMetadataQueryTest(
boolean mmap2,
boolean rollup1,
boolean rollup2,
boolean differentIds
boolean differentIds,
boolean bitmaps
)
{
final SegmentId id1 = SegmentId.dummy(differentIds ? "testSegment1" : "testSegment");
final SegmentId id2 = SegmentId.dummy(differentIds ? "testSegment2" : "testSegment");
this.runner1 = mmap1
? makeMMappedQueryRunner(id1, rollup1, FACTORY)
: makeIncrementalIndexQueryRunner(id1, rollup1, FACTORY);
? makeMMappedQueryRunner(id1, rollup1, bitmaps, FACTORY)
: makeIncrementalIndexQueryRunner(id1, rollup1, bitmaps, FACTORY);
this.runner2 = mmap2
? makeMMappedQueryRunner(id2, rollup2, FACTORY)
: makeIncrementalIndexQueryRunner(id2, rollup2, FACTORY);
? makeMMappedQueryRunner(id2, rollup2, bitmaps, FACTORY)
: makeIncrementalIndexQueryRunner(id2, rollup2, bitmaps, FACTORY);
this.mmap1 = mmap1;
this.mmap2 = mmap2;
this.rollup1 = rollup1;
this.rollup2 = rollup2;
this.differentIds = differentIds;
this.bitmaps = bitmaps;
testQuery = Druids.newSegmentMetadataQueryBuilder()
.dataSource("testing")
.intervals("2013/2014")
Expand All @@ -169,6 +185,16 @@ public SegmentMetadataQueryTest(
.merge(true)
.build();

int preferedSize1 = 0;
int placementSize2 = 0;
int overallSize1 = 119691;
int overallSize2 = 119691;
if (bitmaps) {
preferedSize1 = mmap1 ? 10881 : 10764;
placementSize2 = mmap2 ? 10881 : 0;
overallSize1 = mmap1 ? 167493 : 168188;
overallSize2 = mmap2 ? 167493 : 168188;
}
expectedSegmentAnalysis1 = new SegmentAnalysis(
id1.toString(),
ImmutableList.of(Intervals.of("2011-01-12T00:00:00.000Z/2011-04-15T00:00:00.001Z")),
Expand All @@ -187,7 +213,7 @@ public SegmentMetadataQueryTest(
new ColumnAnalysis(
ValueType.STRING.toString(),
false,
mmap1 ? 10881 : 10764,
preferedSize1,
1,
"preferred",
"preferred",
Expand All @@ -203,7 +229,7 @@ public SegmentMetadataQueryTest(
null,
null
)
), mmap1 ? 167493 : 168188,
), overallSize1,
1209,
null,
null,
Expand All @@ -228,7 +254,7 @@ public SegmentMetadataQueryTest(
new ColumnAnalysis(
ValueType.STRING.toString(),
false,
mmap2 ? 10881 : 0,
placementSize2,
1,
null,
null,
Expand All @@ -245,7 +271,7 @@ public SegmentMetadataQueryTest(
null
)
// null_column will be included only for incremental index, which makes a little bigger result than expected
), mmap2 ? 167493 : 168188,
), overallSize2,
1209,
null,
null,
Expand Down Expand Up @@ -470,10 +496,16 @@ public void testSegmentMetadataQueryWithComplexColumnMerge()
@Test
public void testSegmentMetadataQueryWithDefaultAnalysisMerge()
{
int size1 = 0;
int size2 = 0;
if (bitmaps) {
size1 = mmap1 ? 10881 : 10764;
size2 = mmap2 ? 10881 : 10764;
}
ColumnAnalysis analysis = new ColumnAnalysis(
ValueType.STRING.toString(),
false,
(mmap1 ? 10881 : 10764) + (mmap2 ? 10881 : 10764),
size1 + size2,
1,
"preferred",
"preferred",
Expand All @@ -485,10 +517,16 @@ public void testSegmentMetadataQueryWithDefaultAnalysisMerge()
@Test
public void testSegmentMetadataQueryWithDefaultAnalysisMerge2()
{
int size1 = 0;
int size2 = 0;
if (bitmaps) {
size1 = mmap1 ? 6882 : 6808;
size2 = mmap2 ? 6882 : 6808;
}
ColumnAnalysis analysis = new ColumnAnalysis(
ValueType.STRING.toString(),
false,
(mmap1 ? 6882 : 6808) + (mmap2 ? 6882 : 6808),
size1 + size2,
3,
"spot",
"upfront",
Expand All @@ -500,10 +538,16 @@ public void testSegmentMetadataQueryWithDefaultAnalysisMerge2()
@Test
public void testSegmentMetadataQueryWithDefaultAnalysisMerge3()
{
int size1 = 0;
int size2 = 0;
if (bitmaps) {
size1 = mmap1 ? 9765 : 9660;
size2 = mmap2 ? 9765 : 9660;
}
ColumnAnalysis analysis = new ColumnAnalysis(
ValueType.STRING.toString(),
false,
(mmap1 ? 9765 : 9660) + (mmap2 ? 9765 : 9660),
size1 + size2,
9,
"automotive",
"travel",
Expand Down
Loading