Proposal: Remove Accumulator::update and Accumulator::merge

[alamb]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

It is very non obvious that either one or the other of Accumulator::update or Accumulator::update_batch are called. This results in unfortunate things like #1525 (implementation in terms of ScalarValue) only then to be followed by #1546 (implementation in terms of Array)

In addition, update_batch and merge_batch will always be the most performant, so they should always be what is implemented.

The same issue applies to Accumulator::merge and Accumulator::merge_batch

I tried making this clearer in the docs via #1542

Describe the solution you'd like
I think a clearer thing to do would be to remove update and merge entirely from the traits and ensure the examples are clear to keep the barrier to entry as low as possible.

Additional context
See additional context on #1547 (review)

[liukun4515]

Can we change the default implementation of the accumulator trait to this?
@alamb

pub trait Accumulator{

fn update(&mut self, values: &[ScalarValue]) -> Result<()> {
   // convert the values to &[Arrayref]
   // call the update_batch method
}
fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()>;
}

[realno]

+1 on this proposal.

[alamb]

@liukun4515

Can we change the default implementation of the accumulator trait to this?

We could change the default implementation, however, I am not sure what purpose that would serve -- the code in datafusion that calls Accumulator only (should) use Accumulator::update_batch

I was thinking of moving the current default implementation of Accumulator::update_batch which calls update() with ScalarValues to some sort of public example or adapter function, so anyone who had existing Accumulator implementations can continue to use them until they move over to update_batch and merge_batch

[liukun4515]

It makes sense to me.
Removing the update and merge is more clear for developers.