Ingestion Aggregation Feature #8611
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #8611 +/- ##
=============================================
- Coverage 69.57% 14.24% -55.34%
+ Complexity 4572 168 -4404
=============================================
Files 1729 1684 -45
Lines 90179 88351 -1828
Branches 13413 13223 -190
=============================================
- Hits 62744 12585 -50159
- Misses 23083 74829 +51746
+ Partials 4352 937 -3415
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
noon-stripe
changed the title
Jackie-Jiang
reviewed
May 5, 2022
Contributor
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
The logic is in general good. We can make it more robust
pinot-common/src/main/java/org/apache/pinot/common/utils/grpc/GrpcRequestBuilder.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/AggregationConfig.java
Outdated
Show resolved
Hide resolved
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java
Outdated
Show resolved
Hide resolved
...st/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImplTestUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/ingestion/IngestionConfig.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
noon-stripe
force-pushed
the
pre-aggregation
branch
from
ce69c7b to
0d24624
Compare
noon-stripe
commented
May 11, 2022
Contributor
Author
noon-stripe
left a comment
noon-stripe
left a comment
There was a problem hiding this comment.
Addressed comments
pinot-common/src/main/java/org/apache/pinot/common/utils/grpc/GrpcRequestBuilder.java
Outdated
Show resolved
Hide resolved
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
...st/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImplTestUtils.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/AggregationConfig.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/ingestion/IngestionConfig.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/ingestion/IngestionConfig.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
noon-stripe
force-pushed
the
pre-aggregation
branch
from
0d24624 to
34431d4
Compare
Contributor
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Seems we are just maintaining the map between column to its source column and the value aggregator in the IngestionAggregator. Suggest removing the IngestionAggregator and add the source column and value aggregator into the IndexContainer. This way we can avoid a lot of unnecessary map lookups
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/IngestionUtils.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
noon-stripe
force-pushed
the
pre-aggregation
branch
from
25390fa to
eefc4ef
Compare
Contributor
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Mostly good. Well done!
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Move this check after the null value check, or it will throw NPE
Contributor
Author
There was a problem hiding this comment.
Which check? I believe these are in the correct order, right?
FieldSpec fieldSpec = schema.getFieldSpecFor(columnName);
Preconditions.checkState(fieldSpec != null, "The destination column '" + columnName
+ "' of the aggregation function must be present in the schema");
Preconditions.checkState(fieldSpec.getFieldType() == FieldSpec.FieldType.METRIC,
"The destination column '" + columnName + "' of the aggregation function must be a metric column");
Contributor
There was a problem hiding this comment.
Move this block before the field spec check:
String aggregationFunction = aggregationConfig.getAggregationFunction();
if (columnName == null || aggregationFunction == null) {
throw new IllegalStateException(
"columnName/aggregationFunction cannot be null in AggregationConfig " + aggregationConfig);
}
Without checking the columnName is not null, schema.getFieldSpecFor(columnName) could throw NPE
Contributor
There was a problem hiding this comment.
Should we consider using SUM on metric itself as the default if not configured?
Contributor
Author
There was a problem hiding this comment.
That's a good suggestion, but I actually prefer to have the aggregation config be a requirement. Otherwise, you're altering the data without informing the user.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
noon-stripe
force-pushed
the
pre-aggregation
branch
from
b1dd998 to
0d7bfd1
Compare
Jackie-Jiang
approved these changes
May 17, 2022
Contributor
There was a problem hiding this comment.
Move this block before the field spec check:
String aggregationFunction = aggregationConfig.getAggregationFunction();
if (columnName == null || aggregationFunction == null) {
throw new IllegalStateException(
"columnName/aggregationFunction cannot be null in AggregationConfig " + aggregationConfig);
}
Without checking the columnName is not null, schema.getFieldSpecFor(columnName) could throw NPE
Jackie-Jiang
added
feature
release-notes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds Ingestion Aggregation. The design doc can be found #8360.
This feature aggregates values at ingestion time, which reduces the number of rows stores (thus speeding up queries), using the same strategy as the 'aggregateMetrics' feature. This expands the feature to include, COUNT, MIN, and MAX, with the ability to expand further.
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
Yes (Please label as backward-incompat, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
Yes (Please label this as backward-incompat, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed
Yes (Please label this PR as release-notes and complete the section on Release Notes)
Release Notes
Ingestion Pre-Aggregation is now supported for MIN, MAX, and COUNT, in addition to SUM. To enable the feature, add an aggregationConfig to the ingestionConfigs of a realtime table config. The format of the config is
"aggregationConfigs": [
{
"columnName": "destColumn",
"aggregationFunction": "MIN(srcColumn)"
}
],
The destColumn must be in the schema and the srcColumn must not be in the schema. Additionally, all destColumns must be noDictionaryColumns.
Documentation
After this PR I will add the documentation.