Proposal: Remove Accumulator::update and Accumulator::merge #1549

Activity

1. alamb

   added

   enhancementNew feature or requestNew feature or request

   on Jan 11, 2022
2. liukun4515 commented on Jan 12, 2022

   

   liukun4515

   on Jan 12, 2022

   Last edited by liukun4515

   Contributor

   More actions

   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<()>;
   }
   
3. realno commented on Jan 12, 2022

   

   realno

   on Jan 12, 2022

   Contributor

   More actions

   +1 on this proposal.
4. alamb commented on Jan 13, 2022

   

   alamb

   on Jan 13, 2022

   Last edited by alamb

   ContributorAuthor

   More actions

   @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
5. liukun4515 commented on Jan 13, 2022

   

   liukun4515

   on Jan 13, 2022

   Contributor

   More actions

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

   👍React with 👍1Reacted by Andrew Lamb
6. jimexist

   closed this as completedin #1582on Jan 18, 2022
7. alamb

   added

   datafusion

   on Feb 10, 2022