FrankChen021 commented on PR #19063:
URL: https://github.com/apache/druid/pull/19063#issuecomment-5742020387
Found two related correctness issues while re-reviewing, both stemming from
`elseValue` being typed as `Long` regardless of the aggregator's actual value
type:
-
`sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumFilterElseSqlAggregator.java:112`
```java
// The producing rule only emits this aggregate when elseValue is an integer
literal of value zero,
// so Long is a safe target type.
final Number elseValue = ((RexLiteral) elseRex).getValueAs(Long.class);
```
This is only safe if the *value type* of the SUM is also integral. But a
few lines later, `valueType`
(`Calcites.getColumnTypeForRelDataType(aggregateCall.getType())`) can resolve
to FLOAT/DOUBLE, and `sumFactory` is built as a
`DoubleSumAggregatorFactory`/`FloatSumAggregatorFactory` accordingly.
`elseValue` stays a boxed `Long(0)` even though the delegate aggregator is a
double/float sum.
Repro: `SELECT SUM(CASE WHEN dim1 = 'nonexistent' THEN m1 ELSE 0 END) FROM
foo` where `m1` is DOUBLE, over a nonempty relation where the filter matches
zero rows. The result comes back as a boxed `Long` instead of `Double`.
-
`processing/src/main/java/org/apache/druid/query/aggregation/FilteredAggregator.java:62`
(and the same pattern in `FilteredBufferAggregator.get()` /
`FilteredVectorAggregator.get()`):
```java
public Object get()
{
if (elseValue != null && hasUnmatchedRow && delegate.isNull()) {
return elseValue;
} else {
return delegate.get();
}
}
```
`get()` returns the raw `elseValue` `Number` unmodified, whereas
`getFloat()`/`getLong()`/`getDouble()` correctly coerce via
`.floatValue()`/`.longValue()`/`.doubleValue()`. So `get()` is the odd one out
— it can hand back a boxed type that doesn't match what the column's declared
type promises. This is reachable independent of issue 1 too: `elseValue` is
`@JsonProperty`-deserialized on `FilteredAggregatorFactory` with no validation
against the delegate's result type, so a hand-written native query combining a
`doubleSum`/`floatSum` delegate with an integer `elseValue` hits the same
mismatch.
Suggest coercing `elseValue` to `valueType` at construction in
`SumFilterElseSqlAggregator` (fixes the root cause for the current SQL-only
producer), and/or coercing in each `get()` to close the gap for any
future/native caller.
--
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]
