Add compression configuration for aggregationConfigs to StartreeIndexConfigs #11744

t0mpere · 2023-10-05T14:54:46Z

Adding aggregationConfigs to enable configuring compression.

"starTreeIndexConfigs": [
        {
          "dimensionsSplitOrder": [
            "a",
            "b",
            "c"
          ],
          "skipStarNodeCreationForDimensions": [],
          "functionColumnPairs": [],
          "aggregationConfigs": [
            {
              "columnName": "column1",
              "aggregationFunction": "SUM",
              "compressionCodec": "SNAPPY"
            },
            {
              "columnName": "column2",
              "aggregationFunction": "distinctcounthll",
              "compressionCodec": "LZ4"
            }
          ],
          "maxLeafRecords": 10000
        }
      ]

See: #9694

This change was tested manually with multiple segment refresh on DOUBLE and BYTES metrics columns.

codecov-commenter · 2023-10-07T08:40:09Z

Codecov Report

Merging #11744 (200fbcf) into master (e0a1f62) will increase coverage by 0.05%.
Report is 49 commits behind head on master.
The diff coverage is 62.62%.

@@             Coverage Diff              @@
##             master   #11744      +/-   ##
============================================
+ Coverage     63.13%   63.18%   +0.05%     
- Complexity     1118     1141      +23     
============================================
  Files          2342     2344       +2     
  Lines        125883   126415     +532     
  Branches      19357    19450      +93     
============================================
+ Hits          79477    79878     +401     
- Misses        40749    40859     +110     
- Partials       5657     5678      +21

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (?)`
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`63.16% <62.62%> (+0.10%)`	⬆️
java-17	`63.05% <62.62%> (+0.12%)`	⬆️
java-20	`63.05% <62.62%> (+0.06%)`	⬆️
temurin	`63.18% <62.62%> (+0.05%)`	⬆️
unittests	`63.18% <62.62%> (+0.05%)`	⬆️
unittests1	`67.36% <62.62%> (+0.07%)`	⬆️
unittests2	`14.39% <0.00%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...al/startree/v2/builder/StarTreeIndexSeparator.java	`85.41% <100.00%> (-1.13%)`	⬇️
...egment/spi/index/startree/StarTreeV2Constants.java	`50.00% <ø> (ø)`
...segment/spi/index/startree/StarTreeV2Metadata.java	`95.00% <100.00%> (+1.25%)`	⬆️
...cal/startree/v2/builder/BaseSingleTreeBuilder.java	`90.47% <91.66%> (+0.02%)`	⬆️
...ocal/startree/v2/builder/MultipleTreesBuilder.java	`54.25% <0.00%> (-1.79%)`	⬇️
...he/pinot/segment/local/utils/TableConfigUtils.java	`71.08% <75.00%> (-2.34%)`	⬇️
...he/pinot/spi/config/table/StarTreeIndexConfig.java	`80.00% <57.14%> (-20.00%)`	⬇️
...l/startree/v2/builder/StarTreeV2BuilderConfig.java	`76.25% <71.42%> (+3.01%)`	⬆️
...ot/spi/config/table/StarTreeAggregationConfig.java	`0.00% <0.00%> (ø)`
.../index/startree/AggregationFunctionColumnPair.java	`66.66% <55.88%> (-14.82%)`	⬇️

... and 146 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/StarTreeIndexConfig.java

…ctionColumnPair

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/StarTreeIndexConfig.java

Jackie-Jiang

Can you also add some tests to ensure the behavior is correct? We may consider using different codec for each aggregation in BaseStarTreeV2Test or StarTreeClusterIntegrationTest

Jackie-Jiang · 2023-10-16T20:45:09Z

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/StarTreeIndexConfig.java

    _dimensionsSplitOrder = dimensionsSplitOrder;
    _skipStarNodeCreationForDimensions = skipStarNodeCreationForDimensions;
-    _functionColumnPairs = functionColumnPairs;
+    _functionColumnPairs = functionColumnPairs != null ? functionColumnPairs : new ArrayList<>();


Suggest treating empty as null instead of treating null as empty. In most of the cases, only one will be configured. We may also add this logic to _skipStarNodeCreationForDimensions

Suggested change

_functionColumnPairs = functionColumnPairs != null ? functionColumnPairs : new ArrayList<>();

_skipStarNodeCreationForDimensions = CollectionUtils.isNotEmpty(skipStarNodeCreationForDimensions) ? skipStarNodeCreationForDimensions : null;

_functionColumnPairs = CollectionUtils.isNotEmpty(functionColumnPairs) ? functionColumnPairs : null;

_aggregationConfigs = CollectionUtils.isNotEmpty(aggregationConfigs) ? aggregationConfigs : null;

Preconditions.checkArgument(_functionColumnPairs != null || _aggregationConfigs != null, ...)

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/StarTreeAggregationConfig.java

...egment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/StarTreeV2Metadata.java

...gment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/StarTreeV2Constants.java

Jackie-Jiang · 2023-10-16T21:08:24Z

...src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumnPair.java

+
+  // Adds current object to Configuration by reference
+  public void addToConfiguration(Configuration configuration) {
+    String configPrefix = StarTreeV2Constants.MetadataKey.AGGREGATION_CONFIG + "." + this.toColumnName();


Suggest not using column name as the prefix because in the future we might want to deprecate it. Using index might be good: aggregate.0.function, aggregate.0.column, aggregate.0.codec

Jackie-Jiang · 2023-10-16T21:13:29Z

...egment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/StarTreeV2Metadata.java

+      Configuration functionColPairsConfig =
+          metadataProperties.subset(MetadataKey.AGGREGATION_CONFIG + "." + functionColumnPair);
+      if (!functionColPairsConfig.isEmpty()) {
+        _functionColumnPairs.add(AggregationFunctionColumnPair.fromConfiguration(functionColPairsConfig));


(CRITICAL) This will cause the same aggregation being added twice if the codec is different

I think I can modify the equals to account for this, ignoring the compression type.

I've changed the data structure to a treeMap with functionColumnPair as a string key to avoid duplicates.

t0mpere · 2023-10-17T09:07:39Z

Thanks for the review. Adressing everything :)

Jackie-Jiang · 2023-10-18T21:36:26Z

Have you pushed the changes?

t0mpere · 2023-10-19T06:06:40Z

Not yet, I'll try to do it by eod

Jackie-Jiang · 2023-10-25T09:00:25Z

I applied a commit to introduce the AggregationSpec and revert the changes in AggregationFunctionColumnPair because that is used to match the functions at query time. Changed the TreeSet<AggregationFunctionColumnPair> to TreeMap<AggregationFunctionColumnPair, AggregationSpec> so that we can use keySet() to achieve the same behavior.
Other changes:

Removed the duplicate config check because I feel user might config both fields, and we prioritize on aggregationConfigs over functionColumnPairs
Simplified the test changes to use a random compression codec in BaseStarTreeV2Test.

Please take a look and let me know if you have concern about the change

t0mpere · 2023-10-26T22:34:59Z

Looks good from my side, and much cleaner. Thanks for helping. :)

Jackie-Jiang · 2023-10-30T23:35:21Z

@t0mpere Thanks for the contribution! Could you please also help contribute the documentation for the new config? It can be added to this repo: git@github.com:pinot-contrib/pinot-docs.git

t0mpere · 2023-10-31T03:24:09Z

Sure, I'm OOO at the moment I will take care in 2 weeks as soon as I will get back to my laptop.

t0mpere · 2023-11-13T17:55:27Z

For reference,
Documentation pr: pinot-contrib/pinot-docs#258

t0mpere added 2 commits October 4, 2023 19:44

add functionColumnPairConfig

f5c5f8f

Fix style

187991e

t0mpere marked this pull request as draft October 5, 2023 16:15

t0mpere added 2 commits October 5, 2023 18:35

Added metadata update and segment refresh logic

c3ddd33

Config logic for StarTreeV2BuilderConfig and StarTreeV2Metadata

2e9d36b

t0mpere marked this pull request as ready for review October 6, 2023 17:34

Jackie-Jiang added feature Configuration Config changes (addition/deletion/change in behavior) labels Oct 9, 2023

Jackie-Jiang reviewed Oct 9, 2023

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/StarTreeIndexConfig.java Outdated Show resolved Hide resolved

Added FunctionColumnPairConfig and compression type to AggregationFun…

78502c9

…ctionColumnPair

t0mpere requested a review from Jackie-Jiang October 11, 2023 09:26

Jackie-Jiang reviewed Oct 12, 2023

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/StarTreeIndexConfig.java Outdated Show resolved Hide resolved

aggregationConfig unified with fieldColumnPair

200fbcf

t0mpere requested a review from Jackie-Jiang October 14, 2023 13:58

Jackie-Jiang reviewed Oct 16, 2023

View reviewed changes

t0mpere and others added 3 commits October 20, 2023 12:43

adding Tests and addressing comments

a9d4dab

Introduce AggregationSpec and clean up tests

18e56b0

Minor fixes

e52f979

Jackie-Jiang added the release-notes Referenced by PRs that need attention when compiling the next release notes label Oct 25, 2023

Jackie-Jiang approved these changes Oct 30, 2023

View reviewed changes

Jackie-Jiang merged commit 9da03b4 into apache:master Oct 30, 2023
19 checks passed

t0mpere changed the title ~~Add compression configuration for functionColumnPairs to StartreeIndexConfigs~~ Add compression configuration for aggregationConfigs to StartreeIndexConfigs Nov 13, 2023

t0mpere mentioned this pull request Nov 13, 2023

Adding aggregationConfigs example to starTreeIndexConfigs pinot-contrib/pinot-docs#258

Merged

Jackie-Jiang added the documentation label Nov 14, 2023

Jackie-Jiang mentioned this pull request Nov 15, 2023

Unable to create Star-Tree index for DISTINCTCOUNTBITMAP #10691

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compression configuration for aggregationConfigs to StartreeIndexConfigs #11744

Add compression configuration for aggregationConfigs to StartreeIndexConfigs #11744

t0mpere commented Oct 5, 2023 •

edited

Loading

codecov-commenter commented Oct 7, 2023 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang Oct 16, 2023

Jackie-Jiang Oct 16, 2023

Jackie-Jiang Oct 16, 2023

t0mpere Oct 19, 2023 •

edited

Loading

t0mpere Oct 20, 2023

t0mpere commented Oct 17, 2023

Jackie-Jiang commented Oct 18, 2023

t0mpere commented Oct 19, 2023

Jackie-Jiang commented Oct 25, 2023

t0mpere commented Oct 26, 2023

Jackie-Jiang commented Oct 30, 2023

t0mpere commented Oct 31, 2023

t0mpere commented Nov 13, 2023

-    _functionColumnPairs = functionColumnPairs != null ? functionColumnPairs : new ArrayList<>();
+    _skipStarNodeCreationForDimensions = CollectionUtils.isNotEmpty(skipStarNodeCreationForDimensions) ? skipStarNodeCreationForDimensions : null;
+    _functionColumnPairs = CollectionUtils.isNotEmpty(functionColumnPairs) ? functionColumnPairs : null;
+    _aggregationConfigs = CollectionUtils.isNotEmpty(aggregationConfigs) ? aggregationConfigs : null;
+    Preconditions.checkArgument(_functionColumnPairs != null || _aggregationConfigs != null, ...)

Add compression configuration for aggregationConfigs to StartreeIndexConfigs #11744

Add compression configuration for aggregationConfigs to StartreeIndexConfigs #11744

Conversation

t0mpere commented Oct 5, 2023 • edited Loading

codecov-commenter commented Oct 7, 2023 • edited Loading

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang Oct 16, 2023

Choose a reason for hiding this comment

Jackie-Jiang Oct 16, 2023

Choose a reason for hiding this comment

Jackie-Jiang Oct 16, 2023

Choose a reason for hiding this comment

t0mpere Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

t0mpere Oct 20, 2023

Choose a reason for hiding this comment

t0mpere commented Oct 17, 2023

Jackie-Jiang commented Oct 18, 2023

t0mpere commented Oct 19, 2023

Jackie-Jiang commented Oct 25, 2023

t0mpere commented Oct 26, 2023

Jackie-Jiang commented Oct 30, 2023

t0mpere commented Oct 31, 2023

t0mpere commented Nov 13, 2023

t0mpere commented Oct 5, 2023 •

edited

Loading

codecov-commenter commented Oct 7, 2023 •

edited

Loading

t0mpere Oct 19, 2023 •

edited

Loading