extension for exactly distinct count for single long type dimension:accurate-cardinality by elloooooo · Pull Request #6768 · apache/druid

elloooooo · 2018-12-21T08:40:28Z

Now, Druid offers the ability by nested group by query.
Its logic can be described as follows:

For a sql like:

select count(distinct pid) from DATASOURCE where col="val"

the exactly query will be like:

select count(*) from (
  select pid from DATASOURCE_segments_in_historical1 where col="val" group by pid
  UNION ALL
  select pid from DATASOURCE_segments_in_historical2 where col="val" group by pid
  UNION ALL
  select pid from DATASOURCE_segments_in_historical3 where col="val" group by pid
  ...
) group by pid

For high cardinality case, the size of result transfered from historical node to broker node can be really large and leads to poor performance.
So this extension try to use bitmap(64bit RoaringBitmap) as the container for the result data from the historical.
The performance can be 10 times better the nested group by method.

Welcome suggestions to this MR.

dylwylie · 2018-12-21T14:30:50Z

+    implements AccurateCardinalityAggregatorColumnSelectorStrategy<DimensionSelector>
+{
+  @Override
+  public long getUniversalUniqueCode(DimensionSelector dimSelector)


Is this truly universal? I'm not familiar with the classes we're interacting with but I'm curious if distinct values from different segments could return colliding ints here.

org.apache.druid.query.aggregation.cardinality.types.StringCardinalityAggregatorColumnSelectorStrategy#hashValues looks up the actual value of the dimension before adding it to the cardinality aggregator.

Thank you for your reply. Because of lack of globle dict, cardinality of string type dimension can not be computed correctly as you said. So this extension can only support long type dimension.

Maybe this code should be removed if it is invalid?

clintropolis

Very sorry for the delayed review, finally got around to making an initial pass. I think overall concept makes sense, will have another look very soon.

clintropolis · 2019-02-06T01:42:27Z

    <inspection_tool class="DuplicateBooleanBranch" enabled="true" level="ERROR" enabled_by_default="true" />
    <inspection_tool class="DuplicateCondition" enabled="true" level="ERROR" enabled_by_default="true">
-      <option name="ignoreSideEffectConditions" value="true" />
+      <option name="ignoreMethodCalls" value="false" />


These changes seem unrelated to this PR?

These changes seem unrelated to this PR?

Yes, I'll remove the change.

clintropolis · 2019-02-06T01:42:47Z

 <component name="InspectionProjectProfileManager">
  <settings>
    <option name="PROJECT_PROFILE" value="Druid" />
+    <option name="USE_PROJECT_PROFILE" value="true" />


Ditto unrelated change

I'll remove it.

clintropolis · 2019-02-06T02:33:28Z

+
+public class AccurateCardinalityAggregatorFactory extends AggregatorFactory
+{
+  private static final AccurateCardinalityAggregatorColumnSelectorStrategyFactory STRATEGY_FACTORY =


I think we are trying to get away from using the ColumnSelectorStrategyFactory pattern when possible in aggregators, see #6909 for details and #6397 (comment) for potential ideas of how to refactor.

clintropolis · 2019-02-06T02:36:00Z

+
+import java.nio.ByteBuffer;
+
+public interface BitmapFactory


Suggest using a different name to avoid confusion with org.apache.druid.collections.bitmap.BitmapFactory, maybe LongBitmapFactory to be similar to other Druid class naming conventions.

clintropolis · 2019-02-06T02:39:22Z

+ * This class is meant to represent a simple wrapper around an immutable bitmap
+ * class.
+ */
+public interface ImmutableBitmap


Likewise, suggest using LongImmutableBitmap or something like here to distinguish from processing library types.

Actually, I've pulled a copy of this branch and.had a closer look and some time to think about it a bit. The only reason for all of this abstraction is if this extension needed to support using multiple bitmap implementations for computing the distinct count, but it only seems to be using roaring. I think it might be better to drop all of this extra structure and just using roaring bitmap directly, either into a concrete collector, also without all of the interface infrastructure, or just pull it all the way up into the aggregators themselves. I think this would significantly simplify the implementation as well.

Is there any reason to support multiple bitmap implementations that I am missing?

clintropolis · 2019-02-06T02:42:03Z

+
+import java.nio.ByteBuffer;
+
+public class RoaringBitmapFactory implements BitmapFactory


Same comment about renaming to distinguish between processing module.

clintropolis · 2019-02-06T02:43:40Z

+
+import java.nio.ByteBuffer;
+
+public interface Collector extends Comparable<Collector>


Collector is a bit generic of a name, maybe LongBitmapCollector?

clintropolis · 2019-02-06T02:46:44Z

+    implements AccurateCardinalityAggregatorColumnSelectorStrategy<DimensionSelector>
+{
+  @Override
+  public long getUniversalUniqueCode(DimensionSelector dimSelector)


Maybe this code should be removed if it is invalid?

clintropolis · 2019-02-06T02:50:23Z

+  @Override
+  public int getMaxIntermediateSize()
+  {
+    return 1024;


Where did this number come from?

clintropolis · 2019-02-06T02:53:55Z

+  @Override
+  public int getMaxIntermediateSize()
+  {
+    return 1024;


Where did this number come from?

clintropolis · 2019-02-06T06:34:15Z

  public static final byte LONG_LAST_CACHE_TYPE_ID = 0x18;
  public static final byte TIMESTAMP_CACHE_TYPE_ID = 0x19;
  public static final byte VARIANCE_CACHE_TYPE_ID = 0x1A;
+  public static final byte ACCURATE_CARDINALITY_CACHE_TYPE_ID = 0x1B;


These cache ids are already in use, they all need to be distinct. Hopefully soon we can create a mechanism to make this a less manual process with #6823, but for now we need to keep this correct by hand. 0x35 is the current highest, but there is another aggregator PR that is using up to 0x37 so you should probably just start with 0x38.

clintropolis · 2019-02-06T06:41:31Z

+ * This class is meant to represent a simple wrapper around an immutable bitmap
+ * class.
+ */
+public interface ImmutableBitmap


Actually, I've pulled a copy of this branch and.had a closer look and some time to think about it a bit. The only reason for all of this abstraction is if this extension needed to support using multiple bitmap implementations for computing the distinct count, but it only seems to be using roaring. I think it might be better to drop all of this extra structure and just using roaring bitmap directly, either into a concrete collector, also without all of the interface infrastructure, or just pull it all the way up into the aggregators themselves. I think this would significantly simplify the implementation as well.

Is there any reason to support multiple bitmap implementations that I am missing?

stale · 2019-04-08T09:45:48Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale · 2019-04-15T11:07:31Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

elloooooo · 2019-07-22T09:44:39Z

reactive

elloooooo · 2019-07-29T07:41:29Z

Can I revive the PR?

liuboyu added 2 commits December 21, 2018 16:34

add extension accurate-cardinality

6467168

fix License check failure

c5d332a

dylwylie reviewed Dec 21, 2018

View reviewed changes

gianm assigned clintropolis Jan 15, 2019

clintropolis requested changes Feb 6, 2019

View reviewed changes

stale Bot added the stale label Apr 8, 2019

stale Bot closed this Apr 15, 2019

elloooooo mentioned this pull request Jul 29, 2019

extension for exactly distinct count for single long type dimension:accurate-cardinality #8185

Closed

4 tasks


		import java.nio.ByteBuffer;

		public class RoaringBitmapFactory implements BitmapFactory


		import java.nio.ByteBuffer;

		public interface Collector extends Comparable<Collector>

Conversation

elloooooo commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylwylie Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale Bot commented Apr 8, 2019

Uh oh!

stale Bot commented Apr 15, 2019

Uh oh!

elloooooo commented Jul 22, 2019

Uh oh!

elloooooo commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elloooooo commented Dec 21, 2018 •

edited

Loading

dylwylie Dec 21, 2018 •

edited

Loading

clintropolis Feb 6, 2019 •

edited

Loading