kosiew commented on code in PR #23275:
URL: https://github.com/apache/datafusion/pull/23275#discussion_r3548760394
##########
datafusion/functions-aggregate/src/variance.rs:
##########
@@ -679,4 +717,89 @@ mod tests {
assert_eq!(result.value(0), 1.0);
Ok(())
}
+
+ #[test]
+ fn convert_to_state_roundtrips_through_merge() -> Result<()> {
Review Comment:
Nice, these new unit tests directly cover `convert_to_state -> merge_batch`,
which is great to see.
One small follow-up idea: it may be worth adding a SQLLogicTest case in
`aggregate_skip_partial.slt` for the newly enabled public skip-partial path for
`variance`, `stddev`, and `corr`. Including a `FILTER` or NULL row would help
catch wiring regressions outside the accumulator unit tests. `COUNT(DISTINCT)`
could be included too if it is practical.
This is non-blocking since the accumulator-level roundtrip tests already
exercise the core state conversion logic.
--
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]
