Convert `approx_median` to UDAF #10840

goldmedal · 2024-06-09T09:38:34Z

Which issue does this PR close?

Closes #10838 .

Rationale for this change

What changes are included in this PR?

Convert approx_median to UDAF
Move ApproxPercentileAccumulator to function-aggregate
Move TDigest to physical-expr-common

Are these changes tested?

Yes

Are there any user-facing changes?

no

jayzhan211

👍

jayzhan211 · 2024-06-10T00:04:30Z

datafusion/functions-aggregate/src/sum.rs

@@ -46,7 +46,7 @@ make_udaf_expr_and_func!(
    Sum,
    sum,
    expression,
-    "Returns the first value in a group of values.",
+    "Returns the sum of a group of values.",


jayzhan211 · 2024-06-10T00:05:10Z

datafusion/physical-expr-common/Cargo.toml

@@ -37,6 +37,7 @@ path = "src/lib.rs"

 [dependencies]
 arrow = { workspace = true }
+arrow-array = { workspace = true }


could we use use arrow::datatypes::Float64Type; to avoid re-import? arrow-array is sub-crate in arrow

Thanks @jayzhan211. It looks better.

jayzhan211 · 2024-06-10T00:11:18Z

datafusion/sqllogictest/test_files/aggregate.slt

@@ -509,6 +509,10 @@ SELECT approx_median(c12) FROM aggregate_test_100
 ----
 0.555006541052

+# csv_query_approx_median_4
+statement error


It would be nice to add a comment here

# test with string, approx median only supports numeric

jayzhan211 · 2024-06-10T06:45:12Z

Thanks @goldmedal

goldmedal · 2024-06-10T08:02:22Z

Thanks again @jayzhan211

* move tdigest to physical-expr-common * move approx_percentile_cont_accumulator to function-aggregate * implement approx_meidan udaf * remove approx_median aggregation function * fix sqllogictests * add removed type tests * cargo fmt and clippy * add logical roundtrip test * fix dataframe test * fix test and proto gen * update lock in datafusion-cli * fix typo * fix test and doc * fix sql_integration * cargo fmt * follow the checking style like other udaf * add comment and modified dependency * update lock and fmt * add missing test annotation

… UDAF Ref: approx_distinct apache/datafusion#10851 Ref: approx_median apache/datafusion#10840 Ref: approx_percentile_cont and _with_weight apache/datafusion#10917

* chore: update datafusion deps * feat: impl ExecutionPlan::static_name() for DatasetExec This required trait method was added upstream [0] and recommends to simply forward to `static_name`. [0]: apache/datafusion#10266 * feat: update first_value and last_value wrappers. Upstream signatures were changed for the new new `AggregateBuilder` api [0]. This simply gets the code to work. We should better incorporate that API into `datafusion-python`. [0] apache/datafusion#10560 * migrate count to UDAF Builtin Count was removed upstream. TBD whether we want to re-implement `count_star` with new API. Ref: apache/datafusion#10893 * migrate approx_percentile_cont, approx_distinct, and approx_median to UDAF Ref: approx_distinct apache/datafusion#10851 Ref: approx_median apache/datafusion#10840 Ref: approx_percentile_cont and _with_weight apache/datafusion#10917 * migrate avg to UDAF Ref: apache/datafusion#10964 * migrage corr to UDAF Ref: apache/datafusion#10884 * migrate grouping to UDAF Ref: apache/datafusion#10906 * add alias `mean` for UDAF `avg` * migrate stddev to UDAF Ref: apache/datafusion#10827 * remove rust alias for stddev The python wrapper now provides stddev_samp alias. * migrage var_pop to UDAF Ref: apache/datafusion#10836 * migrate regr_* functions to UDAF Ref: apache/datafusion#10898 * migrate bitwise functions to UDAF The functions now take a single expression instead of a Vec<_>. Ref: apache/datafusion#10930 * add missing variants for ScalarValue with todo * fix typo in approx_percentile_cont * add distinct arg to count * comment out failing test `approx_percentile_cont` is now returning a DoubleArray instead of an IntArray. This may be a bug upstream; it requires further investigation. * update tests to expect lowercase `sum` in query plans This was changed upstream. Ref: apache/datafusion#10831 * update ScalarType data_type map * add docs dependency pickleshare * re-implement count_star * lint: ruff python lint * lint: rust cargo fmt * include name of window function in error for find_window_fn * refactor `find_window_fn` for debug clarity * search default aggregate functions by both name and aliases The alias list no longer includes the name of the function. Ref: apache/datafusion#10658 * fix markdown in find_window_fn docs * parameterize test_window_functions `first_value` and `last_value` are currently failing and marked as xfail. * add test ids to test_simple_select tests marked xfail * update find_window_fn to search built-ins first The behavior of `first_value` and `last_value` UDAFs currently does not match the built-in behavior. This allowed me to remove `marks=pytest.xfail` from the window tests. * improve first_call and last_call use of the builder API * remove trailing todos * fix examples/substrait.py * chore: remove explicit aliases from functions.rs Ref: #779 * remove `array_fn!` aliases * remove alias rules for `expr_fn_vec!` * remove alias rules from `expr_fn!` macro * remove unnecessary pyo3 var-arg signatures in functions.rs * remove pyo3 signatures that provided defaults for first_value and last_value * parametrize test_string_functions * test regr_ function wrappers Closes #778

goldmedal added 9 commits June 9, 2024 14:50

move tdigest to physical-expr-common

86e2469

move approx_percentile_cont_accumulator to function-aggregate

fca32c6

implement approx_meidan udaf

a917577

remove approx_median aggregation function

661ed0f

fix sqllogictests

321b565

add removed type tests

c193b08

cargo fmt and clippy

2767f92

add logical roundtrip test

35c62f1

Merge branch 'main' into feature/10838-approx-median-udaf

3399d75

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jun 9, 2024

goldmedal mentioned this pull request Jun 9, 2024

Convert approx_median to UDAF #10838

Closed

fix dataframe test

aa8e61d

github-actions bot added the core Core DataFusion crate label Jun 9, 2024

goldmedal marked this pull request as draft June 9, 2024 10:09

goldmedal added 5 commits June 9, 2024 18:20

fix test and proto gen

0b5e94b

update lock in datafusion-cli

583f2c4

fix typo

151ee20

fix test and doc

47e9808

fix sql_integration

a52c7c4

github-actions bot added the sql SQL Planner label Jun 9, 2024

goldmedal added 2 commits June 9, 2024 21:21

cargo fmt

a63cf4b

follow the checking style like other udaf

1845dba

goldmedal marked this pull request as ready for review June 9, 2024 13:52

jayzhan211 approved these changes Jun 10, 2024

View reviewed changes

goldmedal added 4 commits June 10, 2024 10:21

add comment and modified dependency

04a9195

Merge branch 'main' into feature/10838-approx-median-udaf

858b472

update lock and fmt

6673013

add missing test annotation

29eff0c

jayzhan211 merged commit 3773fb7 into apache:main Jun 10, 2024

goldmedal deleted the feature/10838-approx-median-udaf branch June 10, 2024 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Convert `approx_median` to UDAF #10840

Convert `approx_median` to UDAF #10840

Uh oh!

goldmedal commented Jun 9, 2024

Uh oh!

jayzhan211 left a comment

Uh oh!

jayzhan211 Jun 10, 2024

Uh oh!

jayzhan211 Jun 10, 2024

Uh oh!

goldmedal Jun 10, 2024

Uh oh!

jayzhan211 Jun 10, 2024 •

edited

Loading

Uh oh!

jayzhan211 commented Jun 10, 2024

Uh oh!

goldmedal commented Jun 10, 2024

Uh oh!

Uh oh!

Convert approx_median to UDAF #10840

Convert approx_median to UDAF #10840

Uh oh!

Conversation

goldmedal commented Jun 9, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

goldmedal Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Jun 10, 2024

Uh oh!

goldmedal commented Jun 10, 2024

Uh oh!

Uh oh!

Convert `approx_median` to UDAF #10840

Convert `approx_median` to UDAF #10840

jayzhan211 Jun 10, 2024 •

edited

Loading