Add batch operations to stddev

[realno]

Which issue does this PR close?

Closes #1546 .

Rationale for this change

For more efficient calculation, we want to implement the batch methods.

What changes are included in this PR?

Are there any user-facing changes?

No user facing changes.

No break changes.

[realno]

@alamb per our discussion, this is to add batch operations for stddev and var.

[alamb]

Thank you @realno -- I will try and review this later today

[alamb]

Looks good to me -- thanks @realno

This PR makes it very clear that the Aggregate API is very confusing.

For aggregates, only update_batch and merge_batch are needed. The extra implementations of update and merge_batch in terms of ScalarValue is not ever called (I actually deleted the implementation of update and merge locally and all the tests still pass)

Thus, my plan is to:

1. Make a follow on PR that removes the implementation of Variance in terms of ScalarValue (as well as the supporting math functions)
2. Create a proposed PR that removes update and merge completely in favor of some adapter functions and documentation

Thanks again for this very high quality work 🏅

[alamb]

Here is a more idiomatic way to iterate over the array and skip nulls (and also faster as it doesn't check the bounds on each access to arr.value(i):

Suggested change

	for i in 0..arr.len() {
	let value = arr.value(i);
	if value == 0_f64 && values.is_null(i) {
	continue;
	}
	let new_count = self.count + 1;
	// NB: filter map skips `None` (null) values
	for value in arr.iter().filter_map(|v| v) {
	let new_count = self.count + 1;

[realno]

Great suggestion!

[realno]

After some investigation, this approach does work as expected. The reason for the null check is because downcast_ref replace the None values into 0_f64 so we need to check in the original array when a 0 is observed. The proposed code checks the array after the type cast so it can't catch the nulls. I tried to find a good way to do similar things on the original array but yet to have any luck. I will dig a bit deeper later, please let me know if you a way to achieve this.

[alamb]

After some investigation, this approach does work as expected.

FWIW I tried making these changes locally and all the tests passed for me

The reason for the null check is because downcast_ref replace the None values into 0_f64 so we need to check in the original array when a 0 is observed.

I am not sure this is accurate. The way arrow works is that the values and "validity" are tracked in separate structures.

Thus for elements that are NULL there is some arbitrary value in the array (which will likely be 0.0f, though that is not guaranteed by the arrow spec).

The construct of arr.iter() returns an iterator of Option<f64> that is None if the element is NULL, and Some(f64_value) if the element is non-NULL.

the use of filter_map then filters out the None elements, somewhat non obviously

This

        for value in arr.iter().filter_map(|v| v) {

Is effectively the same as

        for value in arr.iter() {
          let value = match value {
            Some(v) => v,
            None => continue,
        };

So I actually think there is a bug in this code as written with nulls -- the check should be

            if values.is_null(i) {

Rather than

            if value == 0_f64 && values.is_null(i) {

(as null values are not guaranteed to be 0.0f)

[realno]

Thanks for clarifying, I will do some more testing locally and follow up with a PR (or more questions :D).

[alamb]

Suggested change

	for i in 0..counts.len() {
	let c = counts.value(i);
	if c == 0_u64 {
	continue;
	}
	let new_count = self.count + c;
	let non_null_counts = counts
	.iter()
	.enumerate()
	.filter_map(|(i, c)| c.map(|c| (i, c)));
	for (i,c) in non_null_counts {
	let new_count = self.count + c;

By the same logic as above this also skips checking bounds on each row. Though for sure I would say this is less readable :(

[realno]

Great suggestion! For this part of the code the length of the array is pretty small (number of partitions to merge), so maybe we can opt for readability here.

[alamb]

seems reasonable to me

[Dandandan]

👍

[alamb]

See #1550 for a cleanup of some of this code.