yashmayya commented on code in PR #19991:
URL: https://github.com/apache/pinot/pull/19991#discussion_r4382772538
##########
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:
More evidence from a baseline-vs-PR run. The default BOOLEAN/TIMESTAMP
rewrite changes SSE queries that work today:
- **Silent wrong rows:** `HAVING MODE(boolCol) = 1.0` now returns the groups
whose mode is `false`. The literal goes through `BooleanUtils.toBoolean`, which
reads everything except `true` and `1` as false. `< 2`, `>= 0.5` and `BETWEEN
0.5 AND 2` are also wrong.
- **New errors:** `HAVING MODE(tsCol) > 1.6001E12` fails with `Invalid
timestamp`. With null handling off, a GROUP BY + HAVING on `MODE(boolCol)
FILTER (...)` with an empty group fails with an NPE (`Boolean.booleanValue()`).
`MODE(b, AVG)` now fails.
- **Changed values and types:** `MODE(ts) + 1` is now LONG, not DOUBLE.
`CAST(MODE(b) AS STRING)` is now `"true"`, not `"1.0"`. Empty input is now
NULL, not `-Infinity`.
On top of that, old servers break during a rolling upgrade. Numeric MODE,
`TableResizer` and the reducer change give the same results on both trees.
Please keep BOOLEAN and TIMESTAMP on the old path by default. Rewrite only
STRING and JSON, and make the typed BOOLEAN/TIMESTAMP form opt-in, with an
explicit 3rd argument or an off-by-default option. That removes all of the
above.
##########
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:
More evidence from a baseline-vs-PR run. The default BOOLEAN/TIMESTAMP
rewrite changes SSE queries that work today:
- **Silent wrong rows:** `HAVING MODE(boolCol) = 1.0` now returns the groups
whose mode is `false`. The literal goes through `BooleanUtils.toBoolean`, which
reads everything except `true` and `1` as false. `< 2`, `>= 0.5` and `BETWEEN
0.5 AND 2` are also wrong.
- **New errors:** `HAVING MODE(tsCol) > 1.6001E12` fails with `Invalid
timestamp`. With null handling off, a GROUP BY + HAVING on `MODE(boolCol)
FILTER (...)` with an empty group fails with an NPE (`Boolean.booleanValue()`).
`MODE(b, 'AVG')` now fails.
- **Changed values and types:** `MODE(ts) + 1` is now LONG, not DOUBLE.
`CAST(MODE(b) AS STRING)` is now `"true"`, not `"1.0"`. Empty input is now
NULL, not `-Infinity`.
On top of that, old servers break during a rolling upgrade. Numeric MODE,
`TableResizer` and the reducer change give the same results on both trees.
Please keep BOOLEAN and TIMESTAMP on the old path by default. Rewrite only
STRING and JSON, and make the typed BOOLEAN/TIMESTAMP form opt-in, with an
explicit 3rd argument or an off-by-default option. That removes all of the
above.
--
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]
