Jackie-Jiang commented on code in PR #13835:
URL: https://github.com/apache/pinot/pull/13835#discussion_r1742857916
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumn.java:
##########
@@ -127,9 +149,14 @@ public boolean equals(Object obj) {
if (this == obj) {
return true;
}
- if (obj instanceof AggregationFunctionColumnPair) {
- AggregationFunctionColumnPair anotherPair =
(AggregationFunctionColumnPair) obj;
- return _functionType == anotherPair._functionType &&
_column.equals(anotherPair._column);
+ if (obj instanceof AggregationFunctionColumn) {
+ AggregationFunctionColumn other = (AggregationFunctionColumn) obj;
+ // TODO: Revisit this since it means that for aggregation functions
where a certain config parameter need not be
+ // checked to determine whether a query can be served by a star-tree
index, we won't rebuild a star-tree index
+ // if the parameter value is changed in the index configuration.
+ return _functionType == other._functionType &&
_column.equals(other._column)
+ &&
AggregationFunctionType.compareFunctionParametersForStarTree(_functionType,
_functionParameters,
+ other._functionParameters) == 0;
Review Comment:
I believe we made it comparable because we are using it as the key in a
`TreeMap` in `StarTreeV2Metadata`. Since currently we are adding a map inside,
we can consider changing it to a hash map and do exact match.
We should also update the `hashCode()` method
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java:
##########
@@ -282,8 +282,6 @@ public void startController(Map<String, Object> properties)
}
// Enable case-insensitive for test cases.
configAccessor.set(scope, Helix.ENABLE_CASE_INSENSITIVE_KEY,
Boolean.toString(true));
- // Set hyperloglog log2m value to 12.
Review Comment:
(minor) We can also remove the `ENABLE_CASE_INSENSITIVE_KEY` (default is
already true), so that there is no special override in `ControllerTest`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumn.java:
##########
@@ -19,26 +19,35 @@
package org.apache.pinot.segment.spi.index.startree;
import java.util.Comparator;
+import java.util.Map;
+import javax.annotation.Nullable;
import org.apache.pinot.segment.spi.AggregationFunctionType;
import org.apache.pinot.spi.config.table.StarTreeAggregationConfig;
-public class AggregationFunctionColumnPair implements
Comparable<AggregationFunctionColumnPair> {
+public class AggregationFunctionColumn implements
Comparable<AggregationFunctionColumn> {
Review Comment:
Suggest naming it `AggregationFunctionInfo`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumn.java:
##########
@@ -127,9 +149,14 @@ public boolean equals(Object obj) {
if (this == obj) {
return true;
}
- if (obj instanceof AggregationFunctionColumnPair) {
- AggregationFunctionColumnPair anotherPair =
(AggregationFunctionColumnPair) obj;
- return _functionType == anotherPair._functionType &&
_column.equals(anotherPair._column);
+ if (obj instanceof AggregationFunctionColumn) {
+ AggregationFunctionColumn other = (AggregationFunctionColumn) obj;
+ // TODO: Revisit this since it means that for aggregation functions
where a certain config parameter need not be
+ // checked to determine whether a query can be served by a star-tree
index, we won't rebuild a star-tree index
+ // if the parameter value is changed in the index configuration.
+ return _functionType == other._functionType &&
_column.equals(other._column)
+ &&
AggregationFunctionType.compareFunctionParametersForStarTree(_functionType,
_functionParameters,
+ other._functionParameters) == 0;
Review Comment:
Actually after a second thought, there are usually not that many
aggregations configured in the star-tree, especially for the same
aggregate-column pair. We can consider managing the info as
`Map<FunctionColumnPair, List<Pair<Map<String, Object>, AggregationSpec>>`.
--
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
