havefunatcode opened a new pull request, #20782:
URL: https://github.com/apache/druid/pull/20782
Fixes #18010.
### Description
`APPROX_QUANTILE('m1', 0.99)`, with the column name in single quotes, is
planned with the string literal as the aggregator input. On master,
APPROX_QUANTILE and APPROX_QUANTILE_DS return NaN (or throw
`ClassCastException` when vectorized), and APPROX_QUANTILE_FIXED_BUCKETS fails
at runtime with `Only serialization version 1 is supported.`
The quantile aggregators declare their first operand as `ANY`, and
`DefaultOperandTypeChecker` doesn't check `ANY` operands at all. The fix
follows the approach @FrankChen021 suggested in the issue.
#### Opt-in options in `DefaultOperandTypeChecker`
Over 50 functions use this checker, so the new behavior is opt-in through
two builder options (also exposed on `OperatorConversions.OperatorBuilder`):
- `operandComplexTypes(int, ColumnType...)`: complex types a NUMERIC operand
also accepts, e.g. the aggregator's own sketch type.
- `coercibleOperands(int...)`: marks a NUMERIC operand as also accepting
STRING input. The checker adds an explicit CAST to DOUBLE, so the conversion
shows up in the plan.
For a coercible operand, a string literal that can't be parsed as a number
is rejected before the cast is added, since the cast would turn it into null:
```
Function [APPROX_QUANTILE] argument [column] must be numeric, got string
literal ['m1']. If you meant to refer to a column, use double quotes.
```
Numeric strings like `'1.5'`, STRING columns and DATETIME input such as
`__time` still work. Parsing uses `ExprEval.computeNumber`, the same as at
runtime, so `'true'` and `'false'` are accepted too.
#### Quantile aggregators
APPROX_QUANTILE, APPROX_QUANTILE_FIXED_BUCKETS, APPROX_QUANTILE_DS,
DS_QUANTILES_SKETCH, TDIGEST_QUANTILE and TDIGEST_GENERATE_SKETCH now declare
their first operand as a coercible NUMERIC that also accepts their own sketch
type. These inputs are now rejected at planning time:
- BOOLEAN and ARRAY
- complex types other than the aggregator's own sketch
- a NULL literal (`Illegal use of 'NULL'`, the same as other functions using
the checker)
The Spectator functions stay on `OperandTypes.ANY`, since their tests expect
`SPECTATOR_COUNT(null)` to return null.
#### `Aggregations`
`toDruidExpressionForNumericAggregator` also rejects non-numeric string
literals for aggregators that still declare `ANY`.
`getArgumentsForSimpleAggregator` skips the check, because COUNT and
SINGLE_VALUE use it and `COUNT(DISTINCT 'abc')` has to keep working.
`SUM('abc')` and `AVG('abc')` still return null. Calcite coerces the literal
to a null DECIMAL before Druid sees it. Per the issue, that's left for a
separate PR. `numeric_agg_string_args.iq` records the current behavior.
#### Release note
APPROX_QUANTILE, APPROX_QUANTILE_FIXED_BUCKETS, APPROX_QUANTILE_DS,
DS_QUANTILES_SKETCH, TDIGEST_QUANTILE and TDIGEST_GENERATE_SKETCH now check the
type of their first argument. They fail at planning time with a 400 error if
the first argument is a string literal that can't be parsed as a number (for
example a column name in single quotes), a BOOLEAN, an ARRAY, an unrelated
complex type or a NULL literal. Previously some of these returned NaN or failed
at runtime.
<hr>
##### Key changed/added classes in this PR
* `DefaultOperandTypeChecker`
* `OperatorConversions`
* `DruidTypeCoercion`
* `Calcites`
* `Aggregations`
* `QuantileSqlAggregator`, `FixedBucketsHistogramQuantileSqlAggregator`,
`DoublesSketchApproxQuantileSqlAggregator`, `DoublesSketchObjectSqlAggregator`,
`TDigestSketchQuantileSqlAggregator`, `TDigestGenerateSketchSqlAggregator`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked
related entities via Javadoc links.
- [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]
