Convert approx_median to UDAF

[jayzhan211]

Is your feature request related to a problem or challenge?

Similar to #10713 #10836 #10834 and others in #8708

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response

[goldmedal]

take

[goldmedal]

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?

[jayzhan211]

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

[goldmedal]

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.

[goldmedal]

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.

[jayzhan211]

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)
	}

[goldmedal]

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.