Convert approx_median to UDAF #10838

Activity

1. jayzhan211

   added

   enhancementNew feature or requestNew feature or request

   good first issueGood for newcomersGood for newcomers

   on Jun 9, 2024
2. goldmedal commented on Jun 9, 2024

   

   goldmedal

   on Jun 9, 2024

   Contributor

   More actions

   take
3. github-actions

   assigned

   goldmedal

   on Jun 9, 2024
4. goldmedal commented on Jun 9, 2024

   

   goldmedal

   on Jun 9, 2024

   Contributor

   More actions

   Hi @jayzhan211,

   After some research, I found that approx_median heavily relies on ApproxPercentileCont for accumulation.

   https://github.com/goldmedal/datafusion/blob/1600a307f272f9c716131dcca7a55ba1761774a6/datafusion/physical-expr/src/aggregate/approx_median.rs#L46

   I think we should convert ApproxPercentileCont to a UDAF first. We can move ApproxPercentileCont and TDigest to function-aggregate, so that ApproxMedian can use them.

   WDYT?
5. jayzhan211 commented on Jun 9, 2024

   

   jayzhan211

   on Jun 9, 2024

   ContributorAuthor

   More actions

   approx_median

   I agree, we need to convert ApproxPercentileCont first. For TDigest, we should move it to datafusion/physical-expr-common/src/aggregate/mod.rs where it can used not only for functions-aggregate but also other users that import physical-expr-common
6. goldmedal commented on Jun 9, 2024

   

   goldmedal

   on Jun 9, 2024

   Last edited by goldmedal

   Contributor

   More actions

   I agree, we need to convert ApproxPercentileCont first. For TDigest, we should move it to datafusion/physical-expr-common/src/aggregate/mod.rs where it can used not only for functions-aggregate but also other users that import physical-expr-common

   Thanks. I will have a PR for it first.
7. goldmedal commented on Jun 9, 2024

   

   goldmedal

   on Jun 9, 2024

   Contributor

   More actions

   Hi @jayzhan211
   I tried to convert ApproxPercentileCont, but I encountered some challenges. ApproxPercentileCont creates the accumulator based on the percentile parameter, but I couldn't find a way to access this parameter when defining the UDAF.
   https://github.com/goldmedal/datafusion/blob/24a08465e12bc07275cafe5310a7ac44898e39de/datafusion/physical-expr/src/aggregate/approx_percentile_cont.rs#L104

   The AccumulatorArgs doesn't carry the original argument expression, making it difficult to create the accumulator for ApproxPercentileCont.
   https://github.com/goldmedal/datafusion/blob/24a08465e12bc07275cafe5310a7ac44898e39de/datafusion/expr/src/function.rs#L45

   I think we need another PR to address this issue. For now, I just moved TDigest and ApproxPercentileAccumulator in #10840 so that I can convert approx_median to a UDAF.
8. jayzhan211 commented on Jun 9, 2024

   

   jayzhan211

   on Jun 9, 2024

   Last edited by jayzhan211

   ContributorAuthor

   More actions

   Hi @jayzhan211 I tried to convert ApproxPercentileCont, but I encountered some challenges. ApproxPercentileCont creates the accumulator based on the percentile parameter, but I couldn't find a way to access this parameter when defining the UDAF. https://github.com/goldmedal/datafusion/blob/24a08465e12bc07275cafe5310a7ac44898e39de/datafusion/physical-expr/src/aggregate/approx_percentile_cont.rs#L104

   The AccumulatorArgs doesn't carry the original argument expression, making it difficult to create the accumulator for ApproxPercentileCont. https://github.com/goldmedal/datafusion/blob/24a08465e12bc07275cafe5310a7ac44898e39de/datafusion/expr/src/function.rs#L45

   I think we need another PR to address this issue. For now, I just moved TDigest and ApproxPercentileAccumulator in #10840 so that I can convert approx_median to a UDAF.

   We can add self.args to AccumulatorArgs and compute percentile and tdigest max size in accumulator.

   datafusion/datafusion/physical-expr-common/src/aggregate/mod.rs

   Lines 286 to 299 in 24a0846

   	fn create_accumulator(&self) -> Result<Box<dyn Accumulator>> {
   	let acc_args = AccumulatorArgs {
   	data_type: &self.data_type,
   	schema: &self.schema,
   	ignore_nulls: self.ignore_nulls,
   	sort_exprs: &self.sort_exprs,
   	is_distinct: self.is_distinct,
   	input_type: &self.input_type,
   	args_num: self.args.len(),
   	name: &self.name,
   	};
   	self.fun.accumulator(acc_args)
   	}
9. goldmedal commented on Jun 9, 2024

   

   goldmedal

   on Jun 9, 2024

   Contributor

   More actions

   We can add self.args to AccumulatorArgs and compute percentile and tdigest max size in accumulator.

   Thanks for the suggestion. I can make a PR after #10840.
10. jayzhan211

    closed this as completedin #10840on Jun 10, 2024