xiangfu0 commented on code in PR #19988:
URL: https://github.com/apache/pinot/pull/19988#discussion_r4365546351
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -143,6 +170,12 @@ private static String getDataTypeArgument(AggregateCall
aggCall, RelNode input)
case DOUBLE:
return "DOUBLE";
case DECIMAL:
+ // The planner types an array of DECIMAL with scale 0 as INT_ARRAY or
LONG_ARRAY, see
+ // RelToPlanNodeConverter.resolveDecimal
+ if (name.equals(AggregationFunctionType.ARRAYAGG.name()) &&
dataType.getScale() == 0
+ && dataType.getPrecision() <= 38) {
+ return dataType.getPrecision() <= 10 ? "INT" : "LONG";
Review Comment:
This scale-0 DECIMAL branch is the subtlest piece of the change, and I don't
see a test that reaches it. The existing `decimal_col` cases all carry a
non-zero scale, so they exercise the `BIG_DECIMAL` path instead. Could you add
a case like `ARRAY_AGG(CAST(x AS DECIMAL(5,0)))` (and one with precision > 10
for the LONG side) to `UDFAggregates.json` or `AggregatePlans.json`? It would
lock in the match with `resolveDecimal` so a future change to either side can't
silently drift.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -116,21 +124,40 @@ public void onMatch(RelOptRuleCall call) {
aggRel.copy(aggRel.getTraitSet(), newInput, aggRel.getGroupSet(),
aggRel.getGroupSets(), aggCalls));
}
- /// Returns the data type argument to append, or `null` when the call
already has one or is not such a call. Throws
- /// for a data argument without an explicit form, such as an array from a
multi-value column.
+ /// Returns the data type argument to add, or `null` when the call already
has one or is not such a call. Throws for
+ /// a data argument without an explicit form, such as an array from a
multi-value column in FIRST_WITH_TIME. The
+ /// mapped types match the single-stage `AggregateFunctionRewriteOptimizer`.
Review Comment:
Minor: this line still says the mapped types match the single-stage
optimizer, but the scale-0 DECIMAL -> INT/LONG special case just below only
exists here. The single-stage path always maps a BIG_DECIMAL column to
`BIG_DECIMAL`. Worth tweaking the wording so the comment doesn't imply a strict
parity that no longer holds.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -143,6 +170,12 @@ private static String getDataTypeArgument(AggregateCall
aggCall, RelNode input)
case DOUBLE:
return "DOUBLE";
case DECIMAL:
+ // The planner types an array of DECIMAL with scale 0 as INT_ARRAY or
LONG_ARRAY, see
+ // RelToPlanNodeConverter.resolveDecimal
+ if (name.equals(AggregationFunctionType.ARRAYAGG.name()) &&
dataType.getScale() == 0
+ && dataType.getPrecision() <= 38) {
+ return dataType.getPrecision() <= 10 ? "INT" : "LONG";
Review Comment:
This scale-0 DECIMAL branch is the subtlest piece of the change, and I don't
see a test that reaches it. The existing `decimal_col` cases all carry a
non-zero scale, so they exercise the `BIG_DECIMAL` path instead. Could you add
a case like `ARRAY_AGG(CAST(x AS DECIMAL(5,0)))` (and one with precision > 10
for the LONG side) to `UDFAggregates.json` or `AggregatePlans.json`? It would
lock in the match with `resolveDecimal` so a future change to either side can't
silently drift.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -116,21 +124,40 @@ public void onMatch(RelOptRuleCall call) {
aggRel.copy(aggRel.getTraitSet(), newInput, aggRel.getGroupSet(),
aggRel.getGroupSets(), aggCalls));
}
- /// Returns the data type argument to append, or `null` when the call
already has one or is not such a call. Throws
- /// for a data argument without an explicit form, such as an array from a
multi-value column.
+ /// Returns the data type argument to add, or `null` when the call already
has one or is not such a call. Throws for
+ /// a data argument without an explicit form, such as an array from a
multi-value column in FIRST_WITH_TIME. The
+ /// mapped types match the single-stage `AggregateFunctionRewriteOptimizer`.
Review Comment:
Minor: this line still says the mapped types match the single-stage
optimizer, but the scale-0 DECIMAL -> INT/LONG special case just below only
exists here. The single-stage path always maps a BIG_DECIMAL column to
`BIG_DECIMAL`. Worth tweaking the wording so the comment doesn't imply a strict
parity that no longer holds.
--
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]
