yashmayya commented on code in PR #19990:
URL: https://github.com/apache/pinot/pull/19990#discussion_r4382872091
##########
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:
Regression with the flag off: `getValueReader` evaluates the expression
before `forEachNotNull`, so it also runs on blocks that are all null. Master
read values only inside non-null ranges. If the expression fails on the null
placeholder, a query that works today now fails. With null handling on (or a
table with column-based null handling, in MSE with no `SET` needed):
```sql
SELECT ANY_VALUE(CAST(s_null AS INT)) FROM t -- master: null, PR:
For input string: "null"
SELECT ANY_VALUE(JSON_EXTRACT_SCALAR(j, '$.a', 'INT')) FROM t -- PR: Cannot
resolve JSON path
```
The GROUP BY form fails the same way. With one all-null segment and one
segment with `"123"`, master returns 123 and the PR fails.
Suggestion: create the reader lazily in the first non-null range, or return
early when the null bitmap covers the whole block. Please add a test: null
handling on, an all-null segment plus a non-null segment, with
`ANY_VALUE(CAST(strCol AS INT))`.
##########
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:
Minor: the default program now runs `WITHOUT_ANY_VALUE`, but
`QueryPlannerRuleOptionsTest` (line 101) still disables `INSTANCE.toString()`.
So that test does not cover the rule that runs by default. It passes only
because the rule is always added. Use `WITHOUT_ANY_VALUE.toString()`, or test
both.
--
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]
