Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Convert stddev to udaf #10827

Closed
jayzhan211 opened this issue Jun 7, 2024 · 4 comments · Fixed by #10834
Closed

Convert stddev to udaf #10827

jayzhan211 opened this issue Jun 7, 2024 · 4 comments · Fixed by #10834
Assignees
@goldmedal
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@jayzhan211
Copy link
Contributor

jayzhan211 commented Jun 7, 2024 •
edited
Loading

Is your feature request related to a problem or challenge?

Similar to #10713 and others in #8708

  1. Move code from datafusion/physical-expr/src/aggregate/stddev.rs to datafusion/functions-aggregate/src/stddev.rs
  2. Add expression api in roundtrip_expr_api in datafusion/proto/tests/cases/roundtrip_logical_plan.rs
  3. Use lowercase for name

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response
@jayzhan211 jayzhan211 added enhancement New feature or request good first issue Good for newcomers labels Jun 7, 2024
@jayzhan211 jayzhan211 changed the title Conver stddev to udaf Convert stddev to udaf Jun 7, 2024
@jayzhan211 jayzhan211 mentioned this issue Jun 7, 2024
Open
@goldmedal
Copy link
Contributor

take

@goldmedal
Copy link
Contributor

Hi @jayzhan211,
Although this issue only mentions stddev, I'm thinking about stddev_pop. Should I also convert it to UDAF? I found that #10713 only converted var_sample but left variance_pop. Is there any reason we didn't convert variance_pop too?

@jayzhan211
Copy link
Contributor Author

Hi @jayzhan211, Although this issue only mentions stddev, I'm thinking about stddev_pop. Should I also convert it to UDAF? I found that #10713 only converted var_sample but left variance_pop. Is there any reason we didn't convert variance_pop too?

We should also convert population function, variance pop is tracked at #10668.
stddev_pop is also on the todo list in #8708.

@goldmedal
Copy link
Contributor

Got it. I think I can convert them in the same PR. Thanks.

@goldmedal goldmedal mentioned this issue Jun 8, 2024
Merged
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this issue Jul 24, 2024
@Michael-J-Ward
migrate stddev to UDAF
c08a35d 
Ref: apache/datafusion#10827
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this issue Jul 24, 2024
@Michael-J-Ward
migrate stddev to UDAF
de5d082 
Ref: apache/datafusion#10827
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this issue Jul 25, 2024
@Michael-J-Ward
migrate stddev to UDAF
d0018ea 
Ref: apache/datafusion#10827
andygrove pushed a commit to apache/datafusion-python that referenced this issue Jul 31, 2024
@Michael-J-Ward
Upgrade Datafusion 40 (#771)
f580155 
* 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
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@goldmedal goldmedal

Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Convert stddev and stddev_pop to UDAF goldmedal/datafusion
2 participants
@goldmedal @jayzhan211