xiangfu0 commented on code in PR #19990:
URL: https://github.com/apache/pinot/pull/19990#discussion_r4383000649
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AnyValueAggregationFunction.java:
##########
@@ -178,78 +242,83 @@ public Object deserializeIntermediateResult(CustomObject
customObject) {
}
@FunctionalInterface
- private interface ValueProcessor<T> {
- boolean process(int index, T value); // Returns true to stop processing,
false to continue
+ private interface ValueProcessor {
+ /// Returns true to stop processing, false to continue. `values` reads the
value of a row, so that a row is read
+ /// only when it is needed.
+ boolean process(int index, IntFunction<Object> values);
}
/// Generic helper for processing values with dictionary optimization for
all supported data types
- private void aggregateHelper(int length, BlockValSet bvs,
ValueProcessor<Object> processor) {
- // Use dictionary-based access for efficiency when available
- if (bvs.isDictionaryEncoded()) {
- final int[] dictIds = bvs.getDictionaryIdsSV();
- final Dictionary dict = bvs.getDictionary();
- forEachNotNull(length, bvs, (from, to) -> {
- for (int i = from; i < to; i++) {
- Object value = getDictionaryValue(dict, dictIds[i],
bvs.getValueType().getStoredType());
- if (processor.process(i, value)) {
- break;
- }
- }
- });
- } else {
- // Fall back to direct value access based on type
- forEachNotNull(length, bvs, (from, to) -> {
- for (int i = from; i < to; i++) {
- Object value = getDirectValue(bvs, i);
- if (value != null && processor.process(i, value)) {
- break;
- }
+ private void aggregateHelper(int length, BlockValSet bvs, ValueProcessor
processor) {
+ // Reading through the bound type keeps values consistent when segments
store the column as different types
+ DataType storedType = _dataType != null ? _dataType.getStoredType() :
bvs.getValueType().getStoredType();
+ IntFunction<Object> values = getValueReader(bvs, storedType);
Review Comment:
Fixed in 4891838b75: the reader is now created for the first non-null range,
so an all-null block never evaluates the expression;
`testAllNullSegmentDoesNotEvaluateExpression` covers an all-null segment plus a
non-null one for `ANY_VALUE(CAST(myField AS INT))` with and without `'INT'`,
aggregation-only and group-by (it fails on a8ab4e2b47 with `For input string:
"null"`).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -33,31 +34,50 @@
import org.apache.calcite.rel.hint.RelHint;
import org.apache.calcite.rel.logical.LogicalAggregate;
import org.apache.calcite.rel.logical.LogicalProject;
+import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
import org.apache.calcite.rex.RexNode;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.tools.RelBuilderFactory;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.query.planner.logical.RelToPlanNodeConverter;
import org.apache.pinot.segment.spi.AggregationFunctionType;
/// Supplies the explicit data type argument from the type of the data
argument:
/// - FIRST_WITH_TIME(dataColumn, timeColumn) -> FIRST_WITH_TIME(dataColumn,
timeColumn, 'dataType')
/// - LAST_WITH_TIME(dataColumn, timeColumn) -> LAST_WITH_TIME(dataColumn,
timeColumn, 'dataType')
+/// - ANY_VALUE(expr) -> ANY_VALUE(expr, 'dataType'), except by
[#WITHOUT_ANY_VALUE]
///
/// The result type of the call is unchanged. The literal is projected
directly beneath the aggregate, appended to the
/// existing project when there is one, because
[PinotAggregateExchangeNodeInsertRule] and `AggregatePushdownRule`
/// inline literal arguments only from the immediate project.
///
-/// The servers require the data type argument, so the rule is required rather
than an optimization: `QueryEnvironment`
-/// always applies it, and `skipPlannerRules` cannot turn it off.
+/// The servers require the data type argument of FIRST_WITH_TIME /
LAST_WITH_TIME, so the rule is required rather than
+/// an optimization: `QueryEnvironment` always applies [#WITHOUT_ANY_VALUE],
or [#INSTANCE] when the `inferAnyValueType`
+/// query option is true, and `skipPlannerRules` cannot turn it off.
public class PinotAggregateTypeArgumentRewriteRule extends RelOptRule {
+ private static final Set<ColumnDataType> ANY_VALUE_DATA_TYPES =
+ EnumSet.of(ColumnDataType.INT, ColumnDataType.LONG,
ColumnDataType.FLOAT, ColumnDataType.DOUBLE,
+ ColumnDataType.BIG_DECIMAL, ColumnDataType.BOOLEAN,
ColumnDataType.TIMESTAMP, ColumnDataType.STRING,
+ ColumnDataType.BYTES, ColumnDataType.UUID);
+
public static final PinotAggregateTypeArgumentRewriteRule INSTANCE =
- new
PinotAggregateTypeArgumentRewriteRule(PinotRuleUtils.PINOT_REL_FACTORY);
+ new PinotAggregateTypeArgumentRewriteRule(true,
PinotRuleUtils.PINOT_REL_FACTORY, null);
+ /// Leaves ANY_VALUE without the data type argument.
+ public static final PinotAggregateTypeArgumentRewriteRule WITHOUT_ANY_VALUE =
+ new PinotAggregateTypeArgumentRewriteRule(false,
PinotRuleUtils.PINOT_REL_FACTORY,
+ "PinotAggregateTypeArgumentRewriteRuleWithoutAnyValue");
Review Comment:
Fixed in 4891838b75: `testTypeArgumentRewriteIgnoresSkipPlannerRules` now
skips `WITHOUT_ANY_VALUE` for the default program and `INSTANCE` with
`inferAnyValueType=true`, asserting the FIRST_WITH_TIME and ANY_VALUE plans for
each.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
