Support default star-tree #5147

Jackie-Jiang · 2020-03-12T19:40:23Z

Support generating default star-tree config with the following rules:

All dictionary-encoded single-value dimensions (including date-time columns) with cardinality smaller or equal to the threshold (10000) will be included in the split order, sorted by their cardinality in descending order
Time column (if exists and dictionary-encoded) will be appended to the split order as the last element
Use COUNT(*) and SUM for all numeric metrics as function column pairs
Use default value (10000) for max leaf records

codecov-io · 2020-03-12T21:14:03Z

Codecov Report

Merging #5147 into master will increase coverage by 0.05%.
The diff coverage is 71.56%.

@@             Coverage Diff              @@
##             master    #5147      +/-   ##
============================================
+ Coverage     65.90%   65.95%   +0.05%     
  Complexity       12       12              
============================================
  Files          1052     1055       +3     
  Lines         54170    54150      -20     
  Branches       8078     8063      -15     
============================================
+ Hits          35702    35717      +15     
+ Misses        15819    15783      -36     
- Partials       2649     2650       +1

Impacted Files	Coverage Δ	Complexity Δ
...e/pinot/broker/api/resources/PinotBrokerDebug.java	`76.66% <ø> (ø)`	`0.00 <0.00> (ø)`
.../BrokerResourceOnlineOfflineStateModelFactory.java	`55.81% <ø> (ø)`	`0.00 <0.00> (ø)`
.../pinot/broker/broker/helix/HelixBrokerStarter.java	`71.97% <ø> (ø)`	`0.00 <0.00> (ø)`
...thandler/SingleConnectionBrokerRequestHandler.java	`92.68% <ø> (ø)`	`0.00 <0.00> (ø)`
...rg/apache/pinot/broker/routing/RoutingManager.java	`79.61% <ø> (-1.54%)`	`0.00 <0.00> (ø)`
...ting/instanceselector/InstanceSelectorFactory.java	`71.42% <ø> (ø)`	`0.00 <0.00> (ø)`
...er/routing/segmentpruner/SegmentPrunerFactory.java	`83.33% <ø> (ø)`	`0.00 <0.00> (ø)`
...outing/segmentselector/SegmentSelectorFactory.java	`60.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...oker/routing/timeboundary/TimeBoundaryManager.java	`87.50% <ø> (ø)`	`0.00 <0.00> (ø)`
...mmon/assignment/InstanceAssignmentConfigUtils.java	`67.50% <ø> (ø)`	`0.00 <0.00> (?)`
... and 169 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00fcb1d...f2c78e0. Read the comment docs.

mcvsubbu · 2020-03-12T21:43:01Z

...-core/src/main/java/org/apache/pinot/core/indexsegment/generator/SegmentGeneratorConfig.java

-      List<StarTreeV2BuilderConfig> starTreeV2BuilderConfigs = new ArrayList<>(starTreeIndexConfigs.size());
-      for (StarTreeIndexConfig starTreeIndexConfig : starTreeIndexConfigs) {
-        starTreeV2BuilderConfigs.add(StarTreeV2BuilderConfig.fromIndexConfig(starTreeIndexConfig));
+    if (indexingConfig.isEnableDefaultStarTree()) {


If both are defined, you should pay attention to the specific start tree definition requested. This will keep backward compat also.

This won't have backward compatible issue because default star-tree is not enabled by default.
But this is a great question, and we should not ignore the explicitly configured star-tree. Modify the code to generate star-tree for both configs (if both are defined, generate both default and customized).

mcvsubbu · 2020-03-12T21:47:53Z

Also, please be sure to add documentation

Jackie-Jiang · 2020-03-12T22:35:35Z

Also, please be sure to add documentation

Documentation for default star-tree config is inside StarTreeV2BuilderConfig. Will add more documentation in gitbook later

mcvsubbu · 2020-03-12T23:24:36Z

pinot-common/src/main/java/org/apache/pinot/common/config/IndexingConfig.java

+  }
+
+  public void setEnableDefaultStarTree(boolean enableDefaultStarTree) {
+    _enableDefaultStarTree = enableDefaultStarTree;


Another way to add this config may be to add a "mode" field in StarTreeIndexConfig. If mode is set to "auto" then ignore other settings. Otherwise, follow the other serttings.

I prefer the current way so that the default star-tree can be enabled by one boolean flag. Don't want to change the existing StarTreeIndexConfig because adding a mode seems more confusing to me.

@Jackie-Jiang will be good to get user feedback on this. @elonazoulay - you have used this feature - your inputs will be valuable.

kishoreg · 2020-03-16T18:16:33Z

pinot-common/src/main/java/org/apache/pinot/common/config/IndexingConfig.java

@@ -145,6 +146,14 @@ public void setOnHeapDictionaryColumns(List<String> onHeapDictionaryColumns) {
    _onHeapDictionaryColumns = onHeapDictionaryColumns;
  }

+  public boolean isEnableDefaultStarTree() {


isDefaultStarTreeEnabled

This has to match the variable name (key of the config), and I prefer the config to be "enableDefaultStarTree": true

kishoreg · 2020-03-16T18:21:48Z

pinot-common/src/main/java/org/apache/pinot/common/config/IndexingConfig.java

+  }
+
+  public void setEnableDefaultStarTree(boolean enableDefaultStarTree) {
+    _enableDefaultStarTree = enableDefaultStarTree;


@Jackie-Jiang will be good to get user feedback on this. @elonazoulay - you have used this feature - your inputs will be valuable.

kishoreg · 2020-03-16T18:58:30Z

pinot-core/src/main/java/org/apache/pinot/core/startree/v2/builder/StarTreeV2BuilderConfig.java

+      switch (fieldSpec.getFieldType()) {
+        case DIMENSION:
+        case DATE_TIME:
+          ColumnMetadata columnMetadata = segmentMetadata.getColumnMetadataFor(column);


why are we checking the cardinality threshold for date_time but not for time?

Here I assume time will be included in most queries and in the range filter or group by, so I decide to always include time column as the last dimension to split.
For DATE_TIME, I assume the query pattern should be similar to other dimensions, so use the same rule for them.
Updated the comments for this.

Another way is to just treat all of them the same, but IMO always putting time column last should suit a wider range of use cases.

kishoreg · 2020-03-16T18:59:39Z

pinot-core/src/main/java/org/apache/pinot/core/startree/v2/builder/StarTreeV2BuilderConfig.java

+          break;
+        case TIME:
+          columnMetadata = segmentMetadata.getColumnMetadataFor(column);
+          if (columnMetadata.hasDictionary()) {


we cannot generate star tree on a column that's not dictionary encoded?

Yes, the dimension must be dictionary encoded because we only store dictionary id in star-tree.

kishoreg · 2020-03-16T18:59:58Z

pinot-core/src/main/java/org/apache/pinot/core/startree/v2/builder/StarTreeV2BuilderConfig.java

+          }
+          break;
+        case METRIC:
+          if (fieldSpec.getDataType().isNumeric()) {


what about hyperloglog

For BYTES column, there is no way to tell what type of data it represents from the metadata. Similar to all other aggregation types (such as MIN, MAX, etc.), DICTINCTCOUNTHLL needs to be configured through the customized star-tree config.

Jackie-Jiang · 2020-03-30T00:55:02Z

@mcvsubbu @kishoreg Addressed the comments, please take another look

mcvsubbu · 2020-03-30T17:36:32Z

@Jackie-Jiang did you get feedback from @elonazoulay as suggested by Kishore on the table config part? Or, are we going ahead without the feedback?

Jackie-Jiang · 2020-03-30T17:42:52Z

@Jackie-Jiang did you get feedback from @elonazoulay as suggested by Kishore on the table config part? Or, are we going ahead without the feedback?

No I haven't. @elonazoulay Does the default config (described in the pr message) look good to you?

kishoreg · 2020-05-09T06:32:58Z

please rebase and push

Support generating default star-tree config with the following rules: - All dictionary-encoded single-value dimensions (including date-time columns) with cardinality smaller or equal to the threshold (10000) will be included in the split order, sorted by their cardinality in descending order - Time column (if exists and dictionary-encoded) will be appended to the split order as the last element - Use COUNT(*) and SUM for all numeric metrics as function column pairs - Use default value (10000) for max leaf records

Jackie-Jiang requested a review from kishoreg March 12, 2020 19:41

mcvsubbu reviewed Mar 12, 2020

View reviewed changes

Jackie-Jiang force-pushed the star_tree_default_config branch from 42fd0e9 to 08095da Compare March 12, 2020 22:33

Jackie-Jiang force-pushed the star_tree_default_config branch from 08095da to 03f2fae Compare March 12, 2020 22:48

mcvsubbu reviewed Mar 12, 2020

View reviewed changes

kishoreg reviewed Mar 16, 2020

View reviewed changes

Jackie-Jiang force-pushed the star_tree_default_config branch from 03f2fae to 3ae1065 Compare March 30, 2020 00:53

Jackie-Jiang force-pushed the star_tree_default_config branch from 3ae1065 to f2c78e0 Compare March 30, 2020 00:57

kishoreg approved these changes May 9, 2020

View reviewed changes

Jackie-Jiang force-pushed the star_tree_default_config branch from f2c78e0 to 274a212 Compare May 12, 2020 01:46

Jackie-Jiang merged commit 25bc1b5 into apache:master May 12, 2020

Jackie-Jiang deleted the star_tree_default_config branch May 12, 2020 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support default star-tree #5147

Support default star-tree #5147

Jackie-Jiang commented Mar 12, 2020

codecov-io commented Mar 12, 2020 •

edited

Loading

mcvsubbu Mar 12, 2020

Jackie-Jiang Mar 12, 2020

mcvsubbu commented Mar 12, 2020

Jackie-Jiang commented Mar 12, 2020

mcvsubbu Mar 12, 2020

Jackie-Jiang Mar 13, 2020

kishoreg Mar 16, 2020

kishoreg Mar 16, 2020

Jackie-Jiang Mar 30, 2020

kishoreg Mar 16, 2020

kishoreg Mar 16, 2020

Jackie-Jiang Mar 30, 2020

kishoreg Mar 16, 2020

Jackie-Jiang Mar 30, 2020

kishoreg Mar 16, 2020

Jackie-Jiang Mar 30, 2020

Jackie-Jiang commented Mar 30, 2020

mcvsubbu commented Mar 30, 2020

Jackie-Jiang commented Mar 30, 2020

kishoreg commented May 9, 2020

Support default star-tree #5147

Support default star-tree #5147

Conversation

Jackie-Jiang commented Mar 12, 2020

codecov-io commented Mar 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcvsubbu commented Mar 12, 2020

Jackie-Jiang commented Mar 12, 2020

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

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

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

Jackie-Jiang commented Mar 30, 2020

mcvsubbu commented Mar 30, 2020

Jackie-Jiang commented Mar 30, 2020

kishoreg commented May 9, 2020

codecov-io commented Mar 12, 2020 •

edited

Loading