xiangfu0 commented on code in PR #19990:
URL: https://github.com/apache/pinot/pull/19990#discussion_r4365218811
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##########
@@ -428,6 +432,54 @@ public static AggregationFunctionType
getAggregationFunctionType(String function
}
}
+ /// Accepts `(expr)` and `(expr, 'dataType')`. The data type must be one the
value argument is read as, since the
+ /// multi-stage engine types the result after the value argument.
+ private static class AnyValueOperandTypeChecker implements
SqlOperandTypeChecker {
+ private static final SqlOperandTypeChecker DELEGATE =
+ OperandTypes.or(OperandTypes.ANY,
OperandTypes.family(SqlTypeFamily.ANY, SqlTypeFamily.CHARACTER));
+ // DECIMAL values are read as INT, LONG, DOUBLE or BIG_DECIMAL depending
on their precision and scale
+ private static final Map<String, Set<SqlTypeName>> SQL_TYPES_BY_DATA_TYPE
= Map.of(
+ "INT", Set.of(SqlTypeName.TINYINT, SqlTypeName.SMALLINT,
SqlTypeName.INTEGER, SqlTypeName.UTINYINT,
+ SqlTypeName.USMALLINT, SqlTypeName.DECIMAL),
+ "LONG", Set.of(SqlTypeName.BIGINT, SqlTypeName.UINTEGER,
SqlTypeName.DECIMAL),
+ "FLOAT", Set.of(SqlTypeName.REAL, SqlTypeName.FLOAT),
+ "DOUBLE", Set.of(SqlTypeName.DOUBLE, SqlTypeName.DECIMAL),
+ "BIG_DECIMAL", Set.of(SqlTypeName.DECIMAL),
+ "BOOLEAN", Set.of(SqlTypeName.BOOLEAN),
+ "TIMESTAMP", Set.of(SqlTypeName.DATE, SqlTypeName.TIME,
SqlTypeName.TIMESTAMP),
+ "STRING", Set.of(SqlTypeName.CHAR, SqlTypeName.VARCHAR),
+ "BYTES", Set.of(SqlTypeName.BINARY, SqlTypeName.VARBINARY),
+ "UUID", Set.of(SqlTypeName.UUID));
+
+ @Override
+ public boolean checkOperandTypes(SqlCallBinding callBinding, boolean
throwOnFailure) {
+ if (!DELEGATE.checkOperandTypes(callBinding, throwOnFailure)) {
+ return false;
+ }
+ if (callBinding.getOperandCount() == 2 &&
callBinding.isOperandLiteral(1, false)) {
Review Comment:
Nit: when the call has 2 operands but the second isn't a literal, this check
is skipped and the error only surfaces later as an `IllegalArgumentException`
from the function constructor on the server. Not a correctness problem, but
rejecting it here would give a cleaner planning-time message.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/AggregationDataTableReducer.java:
##########
@@ -242,7 +242,10 @@ private ResultTable reduceToResultTable(DataSchema
dataSchema, Object[] finalRes
int numColumns = columnDataTypes.length;
for (Object[] rewrittenRow : rows) {
for (int j = 0; j < numColumns; j++) {
- rewrittenRow[j] = columnDataTypes[j].format(rewrittenRow[j]);
+ Object value = rewrittenRow[j];
+ if (value != null) {
+ rewrittenRow[j] = columnDataTypes[j].format(value);
Review Comment:
This null-guard looks correct and necessary: `format(null)` throws an NPE
for BIG_DECIMAL/TIMESTAMP/BYTES/UUID, which is exactly the empty-input
ANY_VALUE case. Just confirming it's intentional and covered by
`testEmptyInput`.
--
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]
