Jackie-Jiang commented on a change in pull request #8408:
URL: https://github.com/apache/pinot/pull/8408#discussion_r836870765
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/DataSourceBasedAggregationOperator.java
##########
@@ -40,10 +39,13 @@
import
org.apache.pinot.core.query.aggregation.function.DistinctCountRawHLLAggregationFunction;
import
org.apache.pinot.core.query.aggregation.function.DistinctCountSmartHLLAggregationFunction;
import org.apache.pinot.segment.local.customobject.MinMaxRangePair;
+import org.apache.pinot.segment.spi.datasource.DataSource;
import org.apache.pinot.segment.spi.index.reader.Dictionary;
import org.apache.pinot.spi.data.FieldSpec;
import org.apache.pinot.spi.utils.ByteArray;
+import static
org.apache.pinot.core.operator.query.AggregationOperatorUtils.toDouble;
Review comment:
Since we merged 2 implementation, shall we keep `toDouble()` within this
class for now? We may revisit our code style in a separate thread
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java
##########
@@ -226,33 +228,53 @@ private TransformOperator
buildTransformOperatorForFilteredAggregates(BaseFilter
* Returns {@code true} if the given aggregations can be solved with segment
metadata, {@code false} otherwise.
* <p>Aggregations supported: COUNT
*/
- private static boolean isFitForMetadataBasedPlan(AggregationFunction[]
aggregationFunctions) {
+ private static boolean isFitForMetadataBasedPlan(AggregationFunction[]
aggregationFunctions,
Review comment:
This can be removed
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/DataSourceBasedAggregationOperator.java
##########
@@ -56,64 +58,69 @@
* For max value we use the last value from dictionary
*/
@SuppressWarnings("rawtypes")
-public class DictionaryBasedAggregationOperator extends
BaseOperator<IntermediateResultsBlock> {
+public class DataSourceBasedAggregationOperator extends
BaseOperator<IntermediateResultsBlock> {
Review comment:
`NonScanAggregationOperator` could be a better name because all indexes
are part of the data source.
We should also update the javadoc for this class
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java
##########
@@ -226,33 +228,53 @@ private TransformOperator
buildTransformOperatorForFilteredAggregates(BaseFilter
* Returns {@code true} if the given aggregations can be solved with segment
metadata, {@code false} otherwise.
* <p>Aggregations supported: COUNT
*/
- private static boolean isFitForMetadataBasedPlan(AggregationFunction[]
aggregationFunctions) {
+ private static boolean isFitForMetadataBasedPlan(AggregationFunction[]
aggregationFunctions,
+ IndexSegment indexSegment) {
for (AggregationFunction aggregationFunction : aggregationFunctions) {
- if (aggregationFunction.getType() != COUNT) {
+ if (!METADATA_BASED_FUNCTIONS.contains(aggregationFunction.getType())) {
return false;
}
+ if (aggregationFunction.getType() != COUNT) {
+ ExpressionContext argument = (ExpressionContext)
aggregationFunction.getInputExpressions().get(0);
+ if (argument.getType() != ExpressionContext.Type.IDENTIFIER) {
+ return false;
+ }
+ String column = argument.getIdentifier();
+ DataSourceMetadata metadata =
indexSegment.getDataSource(column).getDataSourceMetadata();
+ if (metadata.getMinValue() == null || metadata.getMaxValue() == null) {
+ return false;
+ }
+ }
}
return true;
}
/**
* Returns {@code true} if the given aggregations can be solved with
dictionary, {@code false} otherwise.
Review comment:
Update the javadoc
--
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]
