havefunatcode opened a new pull request, #20845:
URL: https://github.com/apache/druid/pull/20845
Follow-up of #18010 and #20782.
### Description
As discussed in #18010, `SUM('abc')` and `AVG('abc')` return null instead of
failing. They declare a NUMERIC operand, so Calcite's implicit coercion casts
the string literal to DECIMAL, and `'abc'` becomes `null:DECIMAL(38, 19)`
before Druid sees it. The check added in #20782 in
`Aggregations.toDruidExpressionForNumericAggregator` never sees the literal for
this reason.
#### Reject non-numeric string literals before the implicit cast
`DruidTypeCoercion` now overrides `builtinFunctionCoercion`. If the operator
is an aggregate function, its first operand is a string literal, the operand
would be cast to a numeric type and the value can't be parsed as a number
(`Calcites.isNumericString`), it throws `InvalidSqlInput` with the same message
as the quantile aggregators:
```
Function [SUM] argument [1] must be numeric, got string literal ['abc']. If
you meant to refer to a column, use double quotes.
```
Some notes about the scope:
- Only aggregate functions. Scalar functions like `ABS('abc')` go through
the same method, but changing them felt out of scope for this PR.
- Only the first operand. Other aggregator operands are already validated by
the aggregators themselves, for example `ARRAY_AGG(dim1, 'abc')` keeps its
"parameter `maxBytes` must be a numeric literal" error
(`testArrayAggWithStringMaxBytes` and friends).
- Explicit casts like `SUM(CAST('abc' AS DOUBLE))` are not checked, same as
in #20782.
- `builtinFunctionCoercion` has no `throwOnFailure` flag, so the error is
thrown directly. For `OperandTypes.or(...)` checkers, Calcite first tries all
rules without coercion, so a rule that accepts the string literal as is still
wins. The error can only hide a later rule that needs its own coercion of the
string literal. I couldn't find such an aggregator.
#### Tests
`numeric_agg_string_args.iq` (added in #20782) now expects errors for
`SUM('abc')` and `AVG('abc')`. I also added SUM and AVG cases for a numeric
column, `__time`, `NULL`, `CAST(NULL AS DOUBLE)`, BOOLEAN, ARRAY, a complex
column and explicit `CAST('abc' AS DOUBLE)`, plus `SUM('abc') OVER ()`,
`SUM(DISTINCT 'abc')`, `SUM('abc') FILTER (...)`, `SUM('')`, `SUM('1e3')` and
`BIT_AND`/`BIT_OR`/`BIT_XOR('abc')`. Only the non-numeric literal cases have
different results from master. `SUM(DISTINCT 'abc')` already failed on master,
with the "DISTINCT is not supported when useApproximateCountDistinct is
enabled" error, and now fails with the string literal error first.
`VarianceSqlAggregatorTest` gets a test for `VARIANCE`, `VAR_POP`, `VAR_SAMP`,
`STDDEV`, `STDDEV_POP` and `STDDEV_SAMP` with `'m1'`.
#### Release note
`SUM`, `AVG`, `BIT_AND`, `BIT_OR`, `BIT_XOR`, `VARIANCE`, `VAR_POP`,
`VAR_SAMP`, `STDDEV`, `STDDEV_POP` and `STDDEV_SAMP` now fail at planning time
with an invalid input error when their input is a string literal that can't be
parsed as a number, such as `SUM('abc')` or `SUM('')`. Before, these queries
did not fail (`SUM`, `AVG` and the `BIT_*` functions returned null).
`SUM('abc') OVER ()` failed with a Calcite assertion error before and now
returns the same invalid input error. Numeric string literals such as
`SUM('1.5')`, STRING columns and explicit casts work as before. Extension
aggregators that declare a NUMERIC first operand with Calcite's checkers (for
example `BITMAP64_EXACT_COUNT`) should behave the same way, but I didn't test
them with a string literal.
<hr>
##### Key changed/added classes in this PR
* `DruidTypeCoercion`
* `numeric_agg_string_args.iq`
* `VarianceSqlAggregatorTest`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] a release note entry in the PR description.
- [x] added comments explaining the "why" and the intent of the code
wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths,
ensuring the threshold for [code
coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md)
is met.
--
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]
