PDGGK opened a new issue, #39751:
URL: https://github.com/apache/beam/issues/39751
### What happened?
SQL `AVG` rounds every result to **10 significant digits**, so an average
over a single row does not equal that row. On `BIGINT` it can come back with
the opposite sign.
`BeamBuiltinAggregations.java:93`
```java
private static MathContext mc = new MathContext(10, RoundingMode.HALF_UP);
```
`:468`
```java
protected BigDecimal prepareOutput(KV<Integer, BigDecimal> accumulator) {
return accumulator.getValue().divide(new BigDecimal(accumulator.getKey()),
mc);
}
```
Every AVG subtype (`:481` INT32, `:493` INT64, `:505` INT16, `:517` INT8,
`:529` FLOAT, `:541` DOUBLE, `:553` DECIMAL) goes through it.
Running that division directly, one row in:
| type | input | `AVG` returns |
|---|---|---|
| `BIGINT` | `9223372036854775807` | **`-9223372036709551616`** — sign
flipped |
| `BIGINT` | `-9223372036854775808` | **`9223372036709551616`** — sign
flipped |
| `BIGINT` | `1786500000123` | `1786500000000` — last three digits zeroed |
| `DECIMAL` | `123456789.99` | `123456790.0` |
The third row is the one that makes this ordinary rather than exotic: any
id, epoch-millis or cent-denominated amount above 10 digits is silently rounded.
### Why the path is live
- `BeamRuleSets.java:117` has `// CoreRules.AGGREGATE_REDUCE_FUNCTIONS`
**commented out**, so `AVG` is not rewritten into `SUM/COUNT` and survives to
`BeamAggregationRel` → `AggregationCombineFnAdapter.createCombineFn` →
`BeamBuiltinAggregations.create`.
- `BeamRelDataTypeSystem` does not override `deriveAvgAggType`, so Calcite's
default applies and the declared output type is the argument type.
### Why I am not sending a patch
The obvious change — `MathContext.DECIMAL128`, or dropping the `MathContext`
— is **also wrong**, just differently:
- `AVG` over `DECIMAL(18,2)` should be `DECIMAL(18,2)` per SQL and per
Calcite's `deriveAvgAggType`. `DECIMAL128` gives scale 33 for `{1,1,2}`, which
leaks 34-digit noise into user `Row`s where `BigDecimal.equals` is
scale-sensitive.
- Dropping the `MathContext` entirely turns a non-terminating division such
as `AVG` of `{1,2}` over 3 rows into `ArithmeticException`.
- Some `DOUBLE` results get uglier: `AVG` of `{0.1, 0.2}` is `0.15` today
and `0.15000000000000002` without the rounding.
The fix that actually matches the declared type looks like threading the
output `RelDataType`'s precision and scale from `AggregateCall` into the
`CombineFn`. That information exists at `AggregationCombineFnAdapter:142` but
is discarded — only the unparameterised `field.getType()` reaches
`BeamBuiltinAggregations.create`. Changing that touches `AVG`, `VAR_*`,
`STDDEV_*`, `COVAR_*` and their coders, which is a design call for someone who
owns this area rather than something to bolt on.
Happy to implement whichever direction a maintainer prefers.
### Note
#39507 is open against this same file (`Add SINGLE_VALUE aggregate
function`), so whoever picks this up may want to sequence after it.
### Issue Priority
Priority: 2 (default / most bugs should be filed as P2)
### Issue Components
- [x] Component: Java SDK
- [x] Component: dsl-sql
--
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]
