xiangfu0 commented on code in PR #19986:
URL: https://github.com/apache/pinot/pull/19986#discussion_r4363498881
##########
pinot-query-runtime/src/test/resources/queries/UDFAggregates.json:
##########
@@ -48,6 +49,56 @@
"sql": "SELECT /*+ aggOptions(is_skip_leaf_stage_aggregate='true') */
string_col, LASTWITHTIME(float_col, timestamp_col, 'FLOAT'),
LASTWITHTIME(double_col, timestamp_col, 'DOUBLE'), last_with_time(long_col,
CAST(long_col AS TIMESTAMP), 'LONG') FROM {tbl} GROUP BY string_col",
"outputs": [["a", 400, 400, 2], ["b", 1, 1, 100], ["c", 1.01, 1.01,
175]]
},
+ {
+ "comment": "The data type argument of FIRST_WITH_TIME / LAST_WITH_TIME
is inferred when omitted",
+ "sql": "SELECT FIRSTWITHTIME(int_col, timestamp_col),
FIRSTWITHTIME(string_col, timestamp_col), first_with_time(bool_col,
CAST(long_col AS TIMESTAMP)) FROM {tbl}",
+ "outputs": [[2, "a", true]]
+ },
+ {
+ "sql": "SELECT bool_col, FIRSTWITHTIME(float_col, timestamp_col),
FIRSTWITHTIME(double_col, timestamp_col), first_with_time(long_col,
CAST(long_col AS TIMESTAMP)) FROM {tbl} GROUP BY bool_col",
+ "outputs": [[false, 100, 100, 3], [true, 300, 300, 2]]
+ },
+ {
+ "sql": "SELECT /*+ aggOptions(is_skip_leaf_stage_aggregate='true') */
string_col, FIRSTWITHTIME(float_col, timestamp_col), FIRSTWITHTIME(double_col,
timestamp_col), first_with_time(long_col, CAST(long_col AS TIMESTAMP)) FROM
{tbl} GROUP BY string_col",
+ "outputs": [["a", 300, 300, 2], ["b", 100, 100, 3], ["c", 1.75, 1.75,
101]]
+ },
+ {
+ "sql": "SELECT LASTWITHTIME(int_col, timestamp_col),
LASTWITHTIME(string_col, timestamp_col), last_with_time(bool_col, CAST(long_col
AS TIMESTAMP)) FROM {tbl}",
+ "outputs": [[101, "c", true]]
+ },
+ {
+ "sql": "SELECT bool_col, LASTWITHTIME(float_col, timestamp_col),
LASTWITHTIME(double_col, timestamp_col), last_with_time(long_col, CAST(long_col
AS TIMESTAMP)) FROM {tbl} GROUP BY bool_col",
+ "outputs": [[false, 1.01, 1.01, 150], [true, 1.75, 1.75, 175]]
+ },
+ {
+ "sql": "SELECT /*+ aggOptions(is_skip_leaf_stage_aggregate='true') */
string_col, LASTWITHTIME(float_col, timestamp_col), LASTWITHTIME(double_col,
timestamp_col), last_with_time(long_col, CAST(long_col AS TIMESTAMP)) FROM
{tbl} GROUP BY string_col",
+ "outputs": [["a", 400, 400, 2], ["b", 1, 1, 100], ["c", 1.01, 1.01,
175]]
+ },
+ {
+ "sql": "SELECT FIRST_WITH_TIME(timestamp_col, timestamp_col),
LAST_WITH_TIME(timestamp_col, timestamp_col), FIRST_WITH_TIME(timestamp_col,
timestamp_col, 'TIMESTAMP') FROM {tbl}",
+ "outputs": [["2020-01-01 03:10:12.0", "2020-05-01 03:32:12.0",
"2020-01-01 03:10:12.0"]]
+ },
+ {
+ "comment": "SMALLINT and TINYINT data arguments infer 'INT'",
+ "sql": "SELECT FIRST_WITH_TIME(CAST(int_col AS SMALLINT),
timestamp_col), LAST_WITH_TIME(CAST(int_col AS SMALLINT), timestamp_col),
LAST_WITH_TIME(CAST(int_col AS TINYINT), timestamp_col) FROM {tbl}",
+ "outputs": [[2, 101, 101]]
+ },
+ {
+ "sql": "SELECT string_col, FIRST_WITH_TIME(CAST(int_col AS SMALLINT),
timestamp_col), LAST_WITH_TIME(CAST(int_col AS SMALLINT), timestamp_col) FROM
{tbl} GROUP BY string_col",
+ "outputs": [["a", 2, 2], ["b", 3, 100], ["c", 175, 101]]
+ },
+ {
+ "sql": "SELECT FIRST_WITH_TIME(decimal_col, timestamp_col),
LAST_WITH_TIME(decimal_col, timestamp_col), FIRST_WITH_TIME(bytes_col,
timestamp_col), LAST_WITH_TIME(bytes_col, timestamp_col, 'BYTES') FROM {tbl}",
Review Comment:
MINOR (test coverage): the multi-stage runtime cases cover BYTES and
BIG_DECIMAL, but there's no UUID case here even though the planner rule and
`INFERRED_DATA_TYPES` both include UUID. Single-stage does cover UUID, so the
multi-stage UUID data-column path is the one spot left untested end-to-end.
Worth adding a small `FIRST_WITH_TIME(uuid_col, ...)` case to lock it in.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##########
@@ -426,6 +427,43 @@ public static AggregationFunctionType
getAggregationFunctionType(String function
}
}
+ /// Accepts `(dataColumn, timeColumn)` and `(dataColumn, timeColumn,
'dataType')`. The two-argument form is limited to
+ /// the types of the explicit form, since its data type argument is inferred
from the data argument.
+ private static class ValueWithTimeOperandTypeChecker implements
SqlOperandTypeChecker {
+ private static final SqlOperandTypeChecker DELEGATE = OperandTypes.or(
+ OperandTypes.family(SqlTypeFamily.ANY, SqlTypeFamily.ANY),
+ OperandTypes.family(SqlTypeFamily.ANY, SqlTypeFamily.ANY,
SqlTypeFamily.CHARACTER));
+ private static final Set<SqlTypeName> INFERRED_DATA_TYPES =
Set.of(SqlTypeName.TINYINT, SqlTypeName.SMALLINT,
+ SqlTypeName.INTEGER, SqlTypeName.BIGINT, SqlTypeName.REAL,
SqlTypeName.FLOAT, SqlTypeName.DOUBLE,
+ SqlTypeName.DECIMAL, SqlTypeName.CHAR, SqlTypeName.VARCHAR,
SqlTypeName.BOOLEAN, SqlTypeName.TIMESTAMP,
+ SqlTypeName.BINARY, SqlTypeName.VARBINARY, SqlTypeName.UUID);
+
+ @Override
+ public boolean checkOperandTypes(SqlCallBinding callBinding, boolean
throwOnFailure) {
+ if (!DELEGATE.checkOperandTypes(callBinding, throwOnFailure)) {
+ return false;
+ }
+ if (callBinding.getOperandCount() == 2
Review Comment:
MINOR (optional hardening): the type restriction only runs for the 2-arg
form. An explicit 3-arg call with an unsupported data column, e.g.
`FIRST_WITH_TIME(mvCol, timeCol, 'INT')`, still passes validation and fails
later at the server. This matches the pre-existing behavior so it's not a
regression, but you could apply the same `INFERRED_DATA_TYPES` check to operand
0 in the 3-arg branch for a cleaner validation-time error.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/statement/AggregateFunctionRewriteOptimizer.java:
##########
@@ -23,21 +23,29 @@
import org.apache.pinot.common.request.Expression;
import org.apache.pinot.common.request.Function;
import org.apache.pinot.common.request.PinotQuery;
+import org.apache.pinot.common.utils.request.RequestUtils;
import org.apache.pinot.segment.spi.AggregationFunctionType;
import org.apache.pinot.spi.data.FieldSpec;
import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.exception.BadQueryRequestException;
import org.apache.pinot.spi.utils.CommonConstants;
/// Rewrites certain aggregation functions based on operand types to support
polymorphic aggregations.
///
-/// Currently supported rewrites:
+/// Always applied:
+/// - FIRST_WITH_TIME(dataColumn, timeColumn) -> FIRST_WITH_TIME(dataColumn,
timeColumn, 'dataType')
+/// - LAST_WITH_TIME(dataColumn, timeColumn) -> LAST_WITH_TIME(dataColumn,
timeColumn, 'dataType')
+///
+/// Applied only when enabled through the `autoRewriteAggregationType` query
option:
Review Comment:
MINOR (release note): now that the traversal recurses,
`autoRewriteAggregationType` also rewrites MIN/MAX/SUM nested under an alias,
transform, FILTER, HAVING or ORDER BY, so e.g. `MIN(longCol) AS m` returns LONG
instead of DOUBLE in those positions. The option is opt-in and off by default,
so this is fine, but it's a result-type change worth a line in the
release/compat notes for anyone who has enabled the flag.
--
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]
