adasari commented on code in PR #18334:
URL: https://github.com/apache/pinot/pull/18334#discussion_r3599584971
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -180,33 +213,73 @@ private boolean isFitForNonScanBasedPlan() {
AggregationFunction[] aggregationFunctions =
_queryContext.getAggregationFunctions();
assert aggregationFunctions != null;
for (AggregationFunction<?, ?> aggregationFunction : aggregationFunctions)
{
- if (aggregationFunction.getType() == COUNT) {
- continue;
- }
- ExpressionContext argument =
aggregationFunction.getInputExpressions().get(0);
- if (argument.getType() != ExpressionContext.Type.IDENTIFIER) {
+ if (!isFitForNonScanBasedPlan(aggregationFunction)) {
return false;
}
- DataSource dataSource =
_indexSegment.getDataSource(argument.getIdentifier(),
_queryContext.getSchema());
- if (DICTIONARY_BASED_FUNCTIONS.contains(aggregationFunction.getType())) {
- if (dataSource.getDictionary() != null) {
- continue;
- }
- }
- if (METADATA_BASED_FUNCTIONS.contains(aggregationFunction.getType())) {
- if (dataSource.getDataSourceMetadata().getMaxValue() != null
- && dataSource.getDataSourceMetadata().getMinValue() != null) {
- continue;
- }
- }
- return false;
}
return true;
}
+ /**
+ * Returns {@code true} if the given aggregation function can be resolved
from the column dictionary or metadata
+ * (without scanning the segment), {@code false} otherwise. {@code COUNT} is
always eligible. Functions whose result
+ * is derived numerically from the column min/max (e.g. MIN, MAX,
MINMAXRANGE) are only eligible for numeric columns,
+ * since non-numeric columns (e.g. BYTES) store min/max as raw values that
cannot be parsed as numbers.
+ */
+ private boolean isFitForNonScanBasedPlan(AggregationFunction<?, ?>
aggregationFunction) {
+ AggregationFunctionType functionType = aggregationFunction.getType();
+ if (functionType == COUNT) {
+ return true;
+ }
+
+ DataSource dataSource =
getDataSourceForAggregationFunction(aggregationFunction);
+ if (dataSource == null) {
+ // Aggregation function does not have a single identifier argument (e.g.
COUNT(*) or COUNT(1)),
+ // so it cannot be resolved from metadata
+ return false;
+ }
+
+ // MIN/MAX/MINMAXRANGE derive their result numerically from the column
min/max, which is only valid for numeric
+ // columns. Non-numeric columns (e.g. BYTES) store min/max as raw values
that cannot be parsed as numbers.
+ if (NUMERIC_METADATA_FUNCTIONS.contains(functionType)
Review Comment:
it fixes existing bug for queries `SELECT MINMAXRANGE(bytesCol) from
test_table;` where max/min values are not numbers for bytes columns.
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -180,33 +213,73 @@ private boolean isFitForNonScanBasedPlan() {
AggregationFunction[] aggregationFunctions =
_queryContext.getAggregationFunctions();
assert aggregationFunctions != null;
for (AggregationFunction<?, ?> aggregationFunction : aggregationFunctions)
{
- if (aggregationFunction.getType() == COUNT) {
- continue;
- }
- ExpressionContext argument =
aggregationFunction.getInputExpressions().get(0);
- if (argument.getType() != ExpressionContext.Type.IDENTIFIER) {
+ if (!isFitForNonScanBasedPlan(aggregationFunction)) {
return false;
}
- DataSource dataSource =
_indexSegment.getDataSource(argument.getIdentifier(),
_queryContext.getSchema());
- if (DICTIONARY_BASED_FUNCTIONS.contains(aggregationFunction.getType())) {
- if (dataSource.getDictionary() != null) {
- continue;
- }
- }
- if (METADATA_BASED_FUNCTIONS.contains(aggregationFunction.getType())) {
- if (dataSource.getDataSourceMetadata().getMaxValue() != null
- && dataSource.getDataSourceMetadata().getMinValue() != null) {
- continue;
- }
- }
- return false;
}
return true;
}
+ /**
+ * Returns {@code true} if the given aggregation function can be resolved
from the column dictionary or metadata
+ * (without scanning the segment), {@code false} otherwise. {@code COUNT} is
always eligible. Functions whose result
+ * is derived numerically from the column min/max (e.g. MIN, MAX,
MINMAXRANGE) are only eligible for numeric columns,
+ * since non-numeric columns (e.g. BYTES) store min/max as raw values that
cannot be parsed as numbers.
+ */
+ private boolean isFitForNonScanBasedPlan(AggregationFunction<?, ?>
aggregationFunction) {
+ AggregationFunctionType functionType = aggregationFunction.getType();
+ if (functionType == COUNT) {
+ return true;
+ }
+
+ DataSource dataSource =
getDataSourceForAggregationFunction(aggregationFunction);
+ if (dataSource == null) {
+ // Aggregation function does not have a single identifier argument (e.g.
COUNT(*) or COUNT(1)),
+ // so it cannot be resolved from metadata
+ return false;
+ }
+
+ // MIN/MAX/MINMAXRANGE derive their result numerically from the column
min/max, which is only valid for numeric
+ // columns. Non-numeric columns (e.g. BYTES) store min/max as raw values
that cannot be parsed as numbers.
+ if (NUMERIC_METADATA_FUNCTIONS.contains(functionType)
Review Comment:
@yashmayya - it fixes existing bug for queries `SELECT MINMAXRANGE(bytesCol)
from test_table;` where max/min values are not numbers for bytes columns.
--
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]
