Jackie-Jiang commented on code in PR #12164:
URL: https://github.com/apache/pinot/pull/12164#discussion_r1456667781
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/AvgValueAggregator.java:
##########
@@ -26,10 +26,11 @@
public class AvgValueAggregator implements ValueAggregator<Object, AvgPair> {
public static final DataType AGGREGATED_VALUE_TYPE = DataType.BYTES;
+ public static final AggregationFunctionType AGGREGATION_FUNCTION_TYPE =
AggregationFunctionType.AVG;
Review Comment:
(minor) Seems no longer needed, consider reverting them (these types are
actually self-explained, so maybe not worth defining as constant)
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1041,6 +1042,13 @@ private static void
validateIndexingConfig(IndexingConfig indexingConfig, @Nulla
throw new IllegalStateException("Invalid StarTreeIndex config: "
+ functionColumnPair + ". Must be"
+ "in the form <Aggregation function>__<Column name>");
}
+ AggregationFunctionColumnPair aggregatedType =
+
AggregationFunctionColumnPair.resolveToAggregatedType(columnPair);
+ if (aggregatedTypes.contains(aggregatedType)) {
+ LOGGER.warn("StarTreeIndex config duplication: {} already
matches existing function column pair: {}. ",
+ columnPair, aggregatedType);
+ }
+ aggregatedTypes.add(aggregatedType);
Review Comment:
Can be simplified:
```suggestion
if (!aggregatedTypes.add(aggregatedType)) {
LOGGER.warn("StarTreeIndex config duplication: {} already
matches existing function column pair: {}. ",
columnPair, aggregatedType);
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/store/StarTreeLoaderUtils.java:
##########
@@ -100,7 +100,16 @@ public StarTreeV2Metadata getMetadata() {
@Override
public DataSource getDataSource(String columnName) {
- return dataSourceMap.get(columnName);
+ DataSource result = dataSourceMap.get(columnName);
+ // Some query columnNames could be supported by the underlying value
aggregation type; do a secondary lookup
Review Comment:
During query time, can we first resolve it instead of relying on the
fallback? Ideally we should resolve it before looking up the metadata
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##########
@@ -497,4 +497,35 @@ public static AggregationFunctionType
getAggregationFunctionType(String function
}
}
}
+
+ /**
+ * Returns the stored {@code AggregationFunctionType} used to create the
underlying value in the segment or index.
+ * Some aggregation function types share the same underlying value but are
used for different purposes in queries.
+ * @param aggregationType the aggregation type used in a query
+ * @return the underlying value aggregation type used in storage e.g.
StarTree index
+ */
+ public static AggregationFunctionType
getAggregatedFunctionType(AggregationFunctionType aggregationType) {
Review Comment:
The stored type only applies to star-tree, so consider moving it to
`AggregationFunctionColumnPair` and renaming to `getStoredType()`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumnPair.java:
##########
@@ -66,6 +66,19 @@ public static AggregationFunctionColumnPair
fromAggregationConfig(StarTreeAggreg
return
fromFunctionAndColumnName(aggregationConfig.getAggregationFunction(),
aggregationConfig.getColumnName());
}
+ /**
+ * Return a new {@code AggregationFunctionColumnPair} from an existing
functionColumnPair where the new pair
+ * has the {@link AggregationFunctionType} set to the aggregated function
type used in the segment or indexes.
+ * @param functionColumnPair the existing functionColumnPair
+ * @return the new functionColumnPair
+ */
+ public static AggregationFunctionColumnPair resolveToAggregatedType(
Review Comment:
Suggest renaming to `resolveToStoredType()` which IMO is more clear
--
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]
