Jackie-Jiang commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1433107168
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/BaseSingleTreeBuilder.java:
##########
@@ -133,7 +133,9 @@ static class Record {
"Dimension: " + dimension + " does not have dictionary");
}
- TreeMap<AggregationFunctionColumnPair, AggregationSpec> aggregationSpecs =
builderConfig.getAggregationSpecs();
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec> aggregationSpecs =
+
StarTreeBuilderUtils.deduplicateAggregationSpecs(builderConfig.getAggregationSpecs());
Review Comment:
Similarly can we directly resolve the `AggregationFunctionType` within the
builder config?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLPlusValueAggregator.java:
##########
@@ -31,6 +31,7 @@
public class DistinctCountHLLPlusValueAggregator implements
ValueAggregator<Object, HyperLogLogPlus> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
+ public static final AggregationFunctionType AGGREGATION_FUNCTION_TYPE =
AggregationFunctionType.DISTINCTCOUNTHLL;
Review Comment:
This is incorrect. It should be `DISTINCTCOUNTHLLPLUS`, which explains why
the test failed
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/StarTreeIndexReader.java:
##########
@@ -121,7 +124,9 @@ private void mapBufferEntries(int starTreeId,
_dataBuffer, ByteOrder.BIG_ENDIAN));
}
// Load metric (function-column pair) forward indexes
- for (AggregationFunctionColumnPair functionColumnPair :
starTreeMetadata.getFunctionColumnPairs()) {
+ TreeMap<AggregationFunctionColumnPair, AggregationSpec>
deduplicatedAggregationSpecs =
Review Comment:
Can we do this at `StarTreeV2Metadata` loading time (in the constructor),
essentially keeping the stored `AggregationFunctionType` within
`AggregationFunctionColumnPair`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationSpec.java:
##########
@@ -50,4 +52,10 @@ public boolean equals(Object o) {
public int hashCode() {
return _compressionType.hashCode();
}
+
+ @Override
+ public String toString() {
+ return new ToStringBuilder(this,
ToStringStyle.SHORT_PREFIX_STYLE).append("compressionType", _compressionType)
+ .toString();
Review Comment:
(minor) Is this for debugging purpose? We usually use the IDE auto-generated
one
```suggestion
return "AggregationSpec{" + "_compressionType=" + _compressionType + '}';
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
