rusackas commented on PR #40279:
URL: https://github.com/apache/superset/pull/40279#issuecomment-4503109688
Some humanized bot suggestions:
1) MIN/MAX are semi-additive, not fully additive. In #40221, the ROLLUP
reuse mode checks for "additive aggregations (SUM, COUNT, MIN, MAX)." Treating
MIN/MAX as safe for ROLLUP cache reuse is fine, but calling them "additive"
rather than "semi-additive" may be misleading. If anyone later adds a property
like `is_additive: bool` or a method on `AggregationType`, this ambiguity could
cause bugs. Consider adding a short note to the docstring (or a helper method)
to distinguish fully-additive from semi-additive values.
2) The parent PR will presumably need to check `aggregation in
{AggregationType.SUM, AggregationType.COUNT, AggregationType.MIN,
AggregationType.MAX}` inline wherever it decides whether ROLLUP is safe. You
could encode that on the enum itself:
```python
@property
def is_rollup_safe(self) -> bool:
return self in {
AggregationType.SUM,
AggregationType.COUNT,
AggregationType.MIN,
AggregationType.MAX,
}
```
3) `OTHER` is a catch-all but there's no guidance on its semantics. Does
`OTHER` mean "unknown / not safe to roll up" or "a custom aggregation the
system doesn't recognize"?
4) Since `AggregationType` is a `str` enum, a consumer could assign a raw
string that isn't a valid member without getting a type error at runtime.
There's no runtime validation because the dataclass uses
`@dataclass(frozen=True)` without `__post_init__`. E.g. `Metric(...,
aggregation="MEDIAN")` does not raise an error since `aggregation:
AggregationType | None` is just a type hint on a dataclass,
I don't think this blocks it, so stamping here, but... FWIW :D
--
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]
