xiangfu0 commented on code in PR #19986:
URL: https://github.com/apache/pinot/pull/19986#discussion_r4364529772
##########
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:
Already covered: f5f56c0b3a added `uuid_col` with multi-stage
FIRST/LAST_WITH_TIME queries (aggregation-only and group-by), plus UUID support
in `QueryRunnerTestBase`.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Not applied. At Jackie's request, e1b44ffe17 removed the validation
whitelist, so the rewrite rule is the single check for the inferred form, and
the explicit form keeps its existing server-side check.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Already noted in the PR description, under "Behavior change for
autoRewriteAggregationType".
_🤖 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]
