yashmayya commented on code in PR #19991:
URL: https://github.com/apache/pinot/pull/19991#discussion_r4381238531
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/ModeAggregationFunction.java:
##########
@@ -495,18 +587,22 @@ public SerializedIntermediateResult
serializeIntermediateResult(Map<? extends Nu
}
@Override
- public Map<? extends Number, Long>
deserializeIntermediateResult(CustomObject customObject) {
+ public Map<?, Long> deserializeIntermediateResult(CustomObject customObject)
{
return ObjectSerDeUtils.deserialize(customObject);
}
@Override
public ColumnDataType getFinalResultColumnType() {
- return ColumnDataType.DOUBLE;
+ return _resultType;
}
@Nullable
@Override
- public Double extractFinalResult(@Nullable Map<? extends Number, Long>
intermediateResult) {
+ public Comparable<?> extractFinalResult(@Nullable Map<?, Long>
intermediateResult) {
+ if (_resultType != ColumnDataType.DOUBLE) {
+ return intermediateResult == null || intermediateResult.isEmpty() ? null
Review Comment:
With null handling off, an empty group now gives `null` for BOOLEAN and
TIMESTAMP. Before, it gave the -Infinity sentinel. An ORDER BY on this value
then fails with an NPE in `TableResizer`:
```sql
SELECT intColumn, MODE(booleanColumn) FILTER (WHERE intColumn < 5) FROM
testTable
GROUP BY intColumn ORDER BY MODE(booleanColumn) FILTER (WHERE intColumn < 5)
DESC LIMIT 3
```
MINSTRING and ANYVALUE fail the same way today, so the root cause is the
comparator in `TableResizer`. Please make it null-safe, or at least add a test
for this case. Not blocking.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/statement/AggregateFunctionRewriteOptimizer.java:
##########
@@ -134,6 +139,31 @@ private static void addDataTypeArgument(Function function,
String functionName,
functionName, fieldSpec.isSingleValueField() ? "" : "multi-value ",
fieldSpec.getDataType()));
}
+ /// Appends the default tie reducer when missing, then the data column's
logical type when it is non-numeric.
+ private static void addModeDataTypeArgument(Function function, FieldSpec
fieldSpec) {
+ if (!fieldSpec.isSingleValueField()) {
+ return;
+ }
+ String dataType;
+ switch (fieldSpec.getDataType()) {
+ case STRING:
+ case BOOLEAN:
Review Comment:
This breaks working queries during a rolling upgrade. The broker upgrades
before the servers. It rewrites `MODE(boolCol)` and `MODE(tsCol, 'MAX')` to the
3-arg form, and an old server rejects that form with `Mode expects at most 2
arguments, got: 3`. Today these queries work (they return DOUBLE), so the
labels alone do not cover it. This is not the same as FIRST_WITH_TIME in
#19986, because that form never worked before. STRING and JSON also failed
before, so they are safe to rewrite now.
Also, when MODE is inside an expression, the column name now shows the added
arguments, for example
`datetrunc('DAY',mode(timestampColumn,'MIN','TIMESTAMP'))`.
Suggestion: rewrite only STRING and JSON in this PR. Put BOOLEAN and
TIMESTAMP behind a broker config that is off by default (like
`inferAnyValueType` in #19990), or move them to a later release.
##########
pinot-query-runtime/src/test/resources/queries/UDFAggregates.json:
##########
@@ -108,6 +127,28 @@
"sql": "SELECT /*+ aggOptions(is_skip_leaf_stage_aggregate='true') */
string_col, 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) FROM {tbl} GROUP BY
string_col",
"outputs": [["a", "1E-307", "1E+307", "deadbeef", "fdeadbee"], ["b",
"1E-308", "1E+308", "efdeadbe", "beefdead"], ["c", "1E+328", "1E-317",
"deadbe02", "adbeefde"]]
},
+ {
+ "comment": "MODE returns the type of a non-numeric argument, with ties
broken by the smallest value",
+ "sql": "SELECT MODE(string_col), MODE(UPPER(string_col)),
MODE(bool_col), MODE(timestamp_col), MODE(int_col) FROM {tbl}",
+ "outputs": [["c", "C", false, "2020-01-01 03:10:12.0", 2]]
+ },
+ {
+ "sql": "SELECT bool_col, MODE(string_col), MODE(timestamp_col) FROM
{tbl} GROUP BY bool_col",
+ "outputs": [[false, "b", "2020-01-01 08:32:12.0"], [true, "a",
"2020-01-01 03:10:12.0"]]
+ },
+ {
+ "sql": "SELECT /*+ aggOptions(is_skip_leaf_stage_aggregate='true') */
int_col, MODE(string_col), MODE(bool_col), MODE(timestamp_col) FROM {tbl} GROUP
BY int_col",
Review Comment:
`is_skip_leaf_stage_aggregate` is not a hint (only
`is_skip_leaf_stage_group_by` exists), and unknown hints are ignored. So this
case runs the normal leaf plan. Use `is_skip_leaf_stage_group_by='true'`. I ran
it locally with that hint and it passes.
--
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]
