Jackie-Jiang commented on code in PR #11744:
URL: https://github.com/apache/pinot/pull/11744#discussion_r1361245255
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/StarTreeIndexConfig.java:
##########
@@ -34,23 +35,29 @@ public class StarTreeIndexConfig extends BaseJsonConfig {
private final List<String> _skipStarNodeCreationForDimensions;
// Function column pairs with delimiter "__", e.g. SUM__col1, MAX__col2,
COUNT__*
private final List<String> _functionColumnPairs;
+ // Function column pairs config, currently only handling compression.
+ private final List<StarTreeAggregationConfig> _aggregationConfigs;
// The upper bound of records to be scanned at the leaf node
private final int _maxLeafRecords;
@JsonCreator
public StarTreeIndexConfig(
@JsonProperty(value = "dimensionsSplitOrder", required = true)
List<String> dimensionsSplitOrder,
- @JsonProperty("skipStarNodeCreationForDimensions") @Nullable
List<String> skipStarNodeCreationForDimensions,
- @JsonProperty(value = "functionColumnPairs", required = true)
List<String> functionColumnPairs,
- @JsonProperty("maxLeafRecords") int maxLeafRecords) {
- Preconditions
- .checkArgument(CollectionUtils.isNotEmpty(dimensionsSplitOrder),
"'dimensionsSplitOrder' must be configured");
- Preconditions
- .checkArgument(CollectionUtils.isNotEmpty(functionColumnPairs),
"'functionColumnPairs' must be configured");
+ @JsonProperty(value = "skipStarNodeCreationForDimensions") @Nullable
+ List<String> skipStarNodeCreationForDimensions,
+ @JsonProperty(value = "functionColumnPairs") @Nullable List<String>
functionColumnPairs,
+ @JsonProperty(value = "aggregationConfigs") @Nullable
List<StarTreeAggregationConfig> aggregationConfigs,
+ @JsonProperty(value = "maxLeafRecords") int maxLeafRecords) {
+
Preconditions.checkArgument(CollectionUtils.isNotEmpty(dimensionsSplitOrder),
+ "'dimensionsSplitOrder' must be configured");
_dimensionsSplitOrder = dimensionsSplitOrder;
_skipStarNodeCreationForDimensions = skipStarNodeCreationForDimensions;
- _functionColumnPairs = functionColumnPairs;
+ _functionColumnPairs = functionColumnPairs != null ? functionColumnPairs :
new ArrayList<>();
Review Comment:
Suggest treating empty as `null` instead of treating `null` as empty. In
most of the cases, only one will be configured. We may also add this logic to
`_skipStarNodeCreationForDimensions`
```suggestion
_skipStarNodeCreationForDimensions =
CollectionUtils.isNotEmpty(skipStarNodeCreationForDimensions) ?
skipStarNodeCreationForDimensions : null;
_functionColumnPairs = CollectionUtils.isNotEmpty(functionColumnPairs) ?
functionColumnPairs : null;
_aggregationConfigs = CollectionUtils.isNotEmpty(aggregationConfigs) ?
aggregationConfigs : null;
Preconditions.checkArgument(_functionColumnPairs != null ||
_aggregationConfigs != null, ...)
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/StarTreeAggregationConfig.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.pinot.spi.config.BaseJsonConfig;
+
+
+public class StarTreeAggregationConfig extends BaseJsonConfig {
+
+ private final String _columnName;
+ private final String _aggregationFunction;
+ private final String _chunkCompressionType;
+
+ @JsonCreator
+ public StarTreeAggregationConfig(@JsonProperty(value = "columnName",
required = true) String columnName,
+ @JsonProperty(value = "aggregationFunction", required = true) String
aggregationFunction,
+ @JsonProperty(value = "chunkCompressionType", required = true) String
chunkCompressionType) {
Review Comment:
This should be optional
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/StarTreeV2Metadata.java:
##########
@@ -44,8 +44,12 @@ public StarTreeV2Metadata(Configuration metadataProperties) {
_functionColumnPairs = new HashSet<>();
for (String functionColumnPair :
metadataProperties.getStringArray(MetadataKey.FUNCTION_COLUMN_PAIRS)) {
_functionColumnPairs.add(AggregationFunctionColumnPair.fromColumnName(functionColumnPair));
- }
- _maxLeafRecords = metadataProperties.getInt(MetadataKey.MAX_LEAF_RECORDS);
+ Configuration functionColPairsConfig =
+ metadataProperties.subset(MetadataKey.AGGREGATION_CONFIG + "." +
functionColumnPair);
+ if (!functionColPairsConfig.isEmpty()) {
+
_functionColumnPairs.add(AggregationFunctionColumnPair.fromConfiguration(functionColPairsConfig));
+ }
+ } _maxLeafRecords =
metadataProperties.getInt(MetadataKey.MAX_LEAF_RECORDS);
Review Comment:
(minor) Reformat
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/StarTreeAggregationConfig.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.pinot.spi.config.BaseJsonConfig;
+
+
+public class StarTreeAggregationConfig extends BaseJsonConfig {
+
+ private final String _columnName;
+ private final String _aggregationFunction;
+ private final String _chunkCompressionType;
+
+ @JsonCreator
+ public StarTreeAggregationConfig(@JsonProperty(value = "columnName",
required = true) String columnName,
+ @JsonProperty(value = "aggregationFunction", required = true) String
aggregationFunction,
+ @JsonProperty(value = "chunkCompressionType", required = true) String
chunkCompressionType) {
Review Comment:
We can consider using `FieldConfig.CompressionCodec` as the type, and name
the config `_compressionCodec`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/StarTreeV2Metadata.java:
##########
@@ -44,8 +44,12 @@ public StarTreeV2Metadata(Configuration metadataProperties) {
_functionColumnPairs = new HashSet<>();
for (String functionColumnPair :
metadataProperties.getStringArray(MetadataKey.FUNCTION_COLUMN_PAIRS)) {
_functionColumnPairs.add(AggregationFunctionColumnPair.fromColumnName(functionColumnPair));
- }
- _maxLeafRecords = metadataProperties.getInt(MetadataKey.MAX_LEAF_RECORDS);
+ Configuration functionColPairsConfig =
+ metadataProperties.subset(MetadataKey.AGGREGATION_CONFIG + "." +
functionColumnPair);
+ if (!functionColPairsConfig.isEmpty()) {
+
_functionColumnPairs.add(AggregationFunctionColumnPair.fromConfiguration(functionColPairsConfig));
Review Comment:
(CRITICAL) This will cause the same aggregation being added twice if the
codec is different
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumnPair.java:
##########
@@ -18,26 +18,61 @@
*/
package org.apache.pinot.segment.spi.index.startree;
+import java.util.Arrays;
import java.util.Comparator;
+import java.util.Objects;
+import org.apache.commons.configuration.Configuration;
import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.spi.config.table.StarTreeAggregationConfig;
public class AggregationFunctionColumnPair implements
Comparable<AggregationFunctionColumnPair> {
public static final String DELIMITER = "__";
public static final String STAR = "*";
+
public static final AggregationFunctionColumnPair COUNT_STAR =
new AggregationFunctionColumnPair(AggregationFunctionType.COUNT, STAR);
private final AggregationFunctionType _functionType;
private final String _column;
+ private final ChunkCompressionType _chunkCompressionType;
- public AggregationFunctionColumnPair(AggregationFunctionType functionType,
String column) {
+ public AggregationFunctionColumnPair(AggregationFunctionType functionType,
String column,
+ ChunkCompressionType compressionType) {
_functionType = functionType;
if (functionType == AggregationFunctionType.COUNT) {
_column = STAR;
} else {
_column = column;
}
+ _chunkCompressionType = compressionType;
+ }
+
+ public AggregationFunctionColumnPair(AggregationFunctionType functionType,
String column) {
+ this(functionType, column, ChunkCompressionType.PASS_THROUGH);
+ }
+
+ public static AggregationFunctionColumnPair fromConfiguration(Configuration
metadataProperties) {
+ if (metadataProperties == null) {
+ return null;
+ }
+ AggregationFunctionType functionType =
AggregationFunctionType.getAggregationFunctionType(
+
metadataProperties.getString(StarTreeV2Constants.MetadataKey.FUNCTION_TYPE));
+ String columnName =
metadataProperties.getString(StarTreeV2Constants.MetadataKey.COLUMN_NAME);
+ ChunkCompressionType compressionType =
+
ChunkCompressionType.valueOf(metadataProperties.getString(StarTreeV2Constants.MetadataKey.COMPRESSION_TYPE));
+ return new AggregationFunctionColumnPair(functionType, columnName,
compressionType);
+ }
+
+ // Adds current object to Configuration by reference
+ public void addToConfiguration(Configuration configuration) {
+ String configPrefix = StarTreeV2Constants.MetadataKey.AGGREGATION_CONFIG +
"." + this.toColumnName();
Review Comment:
Suggest not using column name as the prefix because in the future we might
want to deprecate it. Using index might be good: `aggregate.0.function`,
`aggregate.0.column`, `aggregate.0.codec`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/StarTreeV2Constants.java:
##########
@@ -47,8 +47,12 @@ public static class MetadataKey {
public static final String TOTAL_DOCS = "total.docs";
public static final String DIMENSIONS_SPLIT_ORDER = "split.order";
public static final String FUNCTION_COLUMN_PAIRS = "function.column.pairs";
+ public static final String AGGREGATION_CONFIG = "aggregation.config";
public static final String MAX_LEAF_RECORDS = "max.leaf.records";
public static final String SKIP_STAR_NODE_CREATION_FOR_DIMENSIONS =
"skip.star.node.creation";
+ public static final String FUNCTION_TYPE = "function_type";
Review Comment:
Let's use dot separated value as metadata key
--
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
