optimize create inverted indexes #2111

binlijin · 2015-12-17T09:48:51Z

In index persist or merge when "Create Inverted Indexes" phase, it iterate dim's every value, then get the value's dictionary id in each index to get the bitmap.
We can direct iterate value's dictionary id, and get the corresponding dictionary id in each index from dimConversion to get the bitmap.
This can improve performance much when dim's cardinality high.

Current i do not see any improvement when the data is small.
But we find when large data do hadoop batch ingest and with some high cardinality dimensions the create inverted indexes in Index merger takes the most time.
I will do the performance later with large data.

binlijin · 2015-12-18T09:40:35Z

Performance number1 :
Before:
2015-12-18 08:55:39,529 INFO [main] segment.IndexMerger (Logger.java:info(70)) - outDir[/tmp/base472859607006656847flush/merged/v8-tmp] completed walk through of 11,192,533 rows in 295,312 millis.

2015-12-18 08:58:31,493 INFO [main] segment.IndexMerger (Logger.java:info(70)) - Starting dimension[nid] with cardinality[10,493,398]
2015-12-18 08:59:57,578 INFO [main] segment.IndexMerger (Logger.java:info(70)) - Completed dimension[nid] in 86,085 millis.

2015-12-18 09:02:06,165 INFO [main] segment.IndexMerger (Logger.java:info(70)) - outDir[/tmp/base472859607006656847flush/merged/v8-tmp] completed inverted.drd in 386,635 millis.

After:
2015-12-18 08:40:15,936 INFO [main] segment.IndexMerger (Logger.java:info(70)) - outDir[/tmp/base4666050658270672045flush/merged/v8-tmp] completed walk through of 11,192,533 rows in 292,092 millis.

2015-12-18 08:43:03,655 INFO [main] segment.IndexMerger (Logger.java:info(70)) - Starting dimension[nid] with cardinality[10,493,398]
2015-12-18 08:43:22,763 INFO [main] segment.IndexMerger (Logger.java:info(70)) - Completed dimension[nid] in 19,108 millis.

2015-12-18 08:45:03,878 INFO [main] segment.IndexMerger (Logger.java:info(70)) - outDir[/tmp/base4666050658270672045flush/merged/v8-tmp] completed inverted.drd in 287,941 millis.

binlijin · 2015-12-18T09:47:26Z

Performance number2 :
Before:
2015-12-18 09:44:16,345 INFO [main] segment.IndexMerger (Logger.java:info(70)) - outDir[/tmp/base6193429426037721634flush/merged/v8-tmp] completed walk through of 4,477,564 rows in 112,079 millis.

2015-12-18 09:45:12,948 INFO [main] segment.IndexMerger (Logger.java:info(70)) - Starting dimension[nid] with cardinality[4,362,606]
2015-12-18 09:45:32,210 INFO [main] segment.IndexMerger (Logger.java:info(70)) - Completed dimension[nid] in 19,262 millis.

2015-12-18 09:46:15,038 INFO [main] segment.IndexMerger (Logger.java:info(70)) - outDir[/tmp/base6193429426037721634flush/merged/v8-tmp] completed inverted.drd in 118,692 millis.

After:
2015-12-18 09:27:56,696 INFO [main] segment.IndexMerger (Logger.java:info(70)) - outDir[/tmp/base5295145984422027811flush/merged/v8-tmp] completed walk through of 4,477,564 rows in 119,256 millis.

2015-12-18 09:28:52,253 INFO [main] segment.IndexMerger (Logger.java:info(70)) - Starting dimension[nid] with cardinality[4,362,606]
2015-12-18 09:28:58,954 INFO [main] segment.IndexMerger (Logger.java:info(70)) - Completed dimension[nid] in 6,701 millis.

2015-12-18 09:29:33,492 INFO [main] segment.IndexMerger (Logger.java:info(70)) - outDir[/tmp/base5295145984422027811flush/merged/v8-tmp] completed inverted.drd in 96,796 millis.

fjy · 2015-12-18T19:46:19Z

@binlijin just looking at your merging times, have you thought about sharding your data more?

fjy · 2015-12-18T19:46:25Z

In any case, this is cool

fjy · 2015-12-18T19:47:04Z

processing/src/main/java/io/druid/segment/IndexMerger.java

+      DictIdSeeker[] dictIdConverter = new DictIdSeeker[indexes.size()];
+      for (int j = 0; j < indexes.size(); j++) {
+        IntBuffer dimConversion = dimConversions.get(j).get(dimension);
+        if(dimConversion != null) {


minor formatting, need a space here

Actually there's a bunch of formatting stuff in this PR. Please make sure to use the style guide.

I use eclipse and find the eclipse_formatting.xml is not useful, and i will try use IntelliJ.

binlijin · 2015-12-21T05:18:49Z

@fjy, we have a big datasource and every day we need to build 30 billion records, so may be we need more big segment.

fjy · 2015-12-21T05:23:10Z

@binlijin You can create multiple segments for the same time interval with different shard numbers. I think you should try to keep segments around 5M rows. This is what we did for 100+ billion records per day.

binlijin · 2015-12-21T05:32:05Z

@fjy, The big datasource we need to keep 15 day's data, and will do query on per day's data,so what if we have too much segments, do druid can handle?

binlijin · 2015-12-29T06:10:52Z

@fjy, what is the problem and why the travis fail?

fjy · 2015-12-29T06:11:44Z

@binlijin there's a couple of non-deterministic unit tests

If you pull the latest master and merge in #2165, things should pass

binlijin · 2015-12-29T06:15:42Z

@fjy, thanks..

fjy · 2015-12-29T19:16:24Z

👍 this looks good to me now, but I think someone else who knows this code should do a review as well

binlijin · 2016-01-07T03:01:05Z

Related to #2138

binlijin · 2016-01-07T13:35:21Z

@xvrl can you take a look?

xvrl · 2016-01-07T17:50:07Z

processing/src/main/java/io/druid/segment/QueryableIndexIndexableAdapter.java

    }

-    final Indexed<String> dimSet = getDimValueLookup(dimension);
-
    // BitmapIndexSeeker is the main performance boost comes from.
    // In the previous version of index merge, during the creation of invert index, we do something like


can we update the comments to explain how BitmapIndexHolder works?

binlijin · 2016-01-15T01:15:44Z

ping @xvrl

xvrl · 2016-01-15T07:31:45Z

processing/src/main/java/io/druid/segment/incremental/IncrementalIndexAdapter.java

-      return new EmptyBitmapIndexSeeker();
+    if (dictId >= 0) {
+      final Indexed<String> dimValues = getDimValueLookup(dimension);
+      String value = Strings.nullToEmpty(dimValues.get(dictId));


is there a reason to call nullToEmpty here? this seems it might be an artifact of wrapping DimDim with NullValueConverterDimDim, however getBitmapIndex relies on the actual values stored in DimDim, not the values returned by the wrapper, it that correct?

Yes, you are right, NullValueConverterDimDim will convert empty to null, we need convert it back to the actual values, because getBitmapIndex relies on the actual values stored in DimDim.

I think this warrants a comment, given that it took me a while to track down the reason for this.

Yes, done, already add a comment for this.

binlijin · 2016-01-18T02:37:09Z

@xvrl rebase

binlijin · 2016-01-20T09:14:18Z

rebase

fjy · 2016-01-21T00:57:39Z

@himanshug can you take a look to help finish this off?

himanshug · 2016-01-21T05:16:28Z

processing/src/main/java/io/druid/segment/QueryableIndexIndexableAdapter.java

+    if (dictId >= 0) {
+      return new BitmapCompressedIndexedInts(bitmaps.getBitmap(dictId));
+    } else {
+      return new EmptyIndexedInts();


I believe EmptyIndexedInts should be a singleton, it already has a static final instance, can you use that? also make the no arg constructor in that class be private.

himanshug · 2016-01-21T05:19:47Z

@binlijin can you update the PR description with a summary of why this change improves performance, it will be helpful to anyone looking at PR.

himanshug · 2016-01-21T05:22:12Z

processing/src/main/java/io/druid/segment/IndexMerger.java

+  public static class DictIdSeeker
+  {
+    static final int NOT_EXIST = -1;
+    static final int NOT_INIT = -1;


can u make both static variables private as well?

ok, i see that they are used in other places.

binlijin · 2016-01-21T06:12:27Z

@himanshug do update the PR description.

binlijin · 2016-01-21T06:24:50Z

rebase

himanshug · 2016-01-21T21:07:05Z

processing/src/test/java/io/druid/segment/IndexMergerTest.java

+    Assert.assertEquals(1, dictIdSeeker.seek(2));
+    try {
+      dictIdSeeker.seek(1);
+    }


you should add an Assert.fail(..) here or else the verification doesn't happen for the case when exception is not thrown.

Good catch, i will fix it.

himanshug · 2016-01-21T21:07:32Z

👍 after #2111 (comment) is resolved.

fjy · 2016-01-21T23:09:35Z

I'm still 👍

@binlijin feel free to merge this after you address @himanshug's comment

binlijin · 2016-01-22T02:40:46Z

rebase and fix test

optimize create inverted indexes

fjy reviewed Dec 18, 2015
View reviewed changes

binlijin closed this Dec 22, 2015

binlijin reopened this Dec 22, 2015

binlijin closed this Dec 28, 2015

binlijin reopened this Dec 28, 2015

binlijin closed this Dec 28, 2015

binlijin reopened this Dec 28, 2015

binlijin closed this Dec 29, 2015

binlijin reopened this Dec 29, 2015

binlijin closed this Dec 30, 2015

binlijin reopened this Dec 30, 2015

xvrl reviewed Jan 7, 2016
View reviewed changes

xvrl reviewed Jan 15, 2016
View reviewed changes

binlijin closed this Jan 18, 2016

binlijin reopened this Jan 18, 2016

binlijin closed this Jan 20, 2016

binlijin reopened this Jan 20, 2016

fjy added this to the 0.9.0 milestone Jan 20, 2016

himanshug reviewed Jan 21, 2016
View reviewed changes

binlijin closed this Jan 21, 2016

binlijin reopened this Jan 21, 2016

himanshug reviewed Jan 21, 2016
View reviewed changes

binlijin closed this Jan 22, 2016

optimize create inverted indexes

55f7dd4

binlijin reopened this Jan 22, 2016

binlijin added a commit that referenced this pull request Jan 22, 2016

Merge pull request #2111 from binlijin/optimize-create-inverted-indexes

1d1f4d9

optimize create inverted indexes

binlijin merged commit 1d1f4d9 into apache:master Jan 22, 2016

fjy mentioned this pull request Feb 5, 2016

druid-0.9.0 release notes #2404

Closed

binlijin deleted the optimize-create-inverted-indexes branch February 18, 2016 07:24

optimize create inverted indexes #2111

optimize create inverted indexes #2111

Conversation

binlijin commented Dec 17, 2015

binlijin commented Dec 18, 2015

binlijin commented Dec 18, 2015

fjy commented Dec 18, 2015

fjy commented Dec 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binlijin commented Dec 21, 2015

fjy commented Dec 21, 2015

binlijin commented Dec 21, 2015

binlijin commented Dec 29, 2015

fjy commented Dec 29, 2015

binlijin commented Dec 29, 2015

fjy commented Dec 29, 2015

binlijin commented Jan 7, 2016

binlijin commented Jan 7, 2016

Choose a reason for hiding this comment

binlijin commented Jan 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binlijin commented Jan 18, 2016

binlijin commented Jan 20, 2016

fjy commented Jan 21, 2016

Choose a reason for hiding this comment

himanshug commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binlijin commented Jan 21, 2016

binlijin commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshug commented Jan 21, 2016

fjy commented Jan 21, 2016

binlijin commented Jan 22, 2016