Jackie-Jiang commented on code in PR #10926:
URL: https://github.com/apache/pinot/pull/10926#discussion_r1244507824
##########
pinot-core/src/test/java/org/apache/pinot/core/common/ObjectSerDeUtilsTest.java:
##########
@@ -374,4 +378,38 @@ public void testDouble2LongMap() {
assertEquals(actual, expected, ERROR_MESSAGE);
}
}
+
+ @Test
Review Comment:
(minor) Let's move the changes in this test into `BigDecimalUtilsTest`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLValueAggregator.java:
##########
@@ -28,11 +33,31 @@
public class DistinctCountHLLValueAggregator implements
ValueAggregator<Object, HyperLogLog> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
- private static final int DEFAULT_LOG2M_BYTE_SIZE = 180;
-
+ private int _log2m = CommonConstants.Helix.DEFAULT_HYPERLOGLOG_LOG2M;
+ private int _log2MByteSize = 180;
Review Comment:
(minor)
```suggestion
private int _log2mByteSize = 180;
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/SumValueAggregator.java:
##########
@@ -37,11 +37,17 @@ public DataType getAggregatedValueType() {
@Override
public Double getInitialAggregatedValue(Number rawValue) {
+ if (rawValue == null) {
Review Comment:
Can this ever be `null`?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLValueAggregator.java:
##########
@@ -28,11 +33,31 @@
public class DistinctCountHLLValueAggregator implements
ValueAggregator<Object, HyperLogLog> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
- private static final int DEFAULT_LOG2M_BYTE_SIZE = 180;
-
+ private int _log2m = CommonConstants.Helix.DEFAULT_HYPERLOGLOG_LOG2M;
+ private int _log2MByteSize = 180;
// Byte size won't change once we get the initial aggregated value
private int _maxByteSize;
+ public DistinctCountHLLValueAggregator() {
+ }
+
+ public DistinctCountHLLValueAggregator(List<ExpressionContext> arguments) {
+ // length 1 means we use the default _log2m of 8
+ if (arguments.size() <= 1) {
+ return;
+ }
+
+ String log2mLiteral = arguments.get(1).getLiteral().getStringValue();
+ Preconditions.checkState(StringUtils.isNumeric(log2mLiteral), "log2m
argument must be a numeric literal");
Review Comment:
(minor) This can be done with a try catch over `Integer.parseInt()` in case
it is an invalid int
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLValueAggregator.java:
##########
@@ -100,6 +124,9 @@ public byte[] serializeAggregatedValue(HyperLogLog value) {
@Override
public HyperLogLog deserializeAggregatedValue(byte[] bytes) {
+ if (bytes == null || bytes.length == 0) {
+ return new HyperLogLog(_log2m);
+ }
Review Comment:
Is this needed? We don't have this special handling in other functions
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
