Skip to content

Move Regr_* functions to use UDAF - #10898

Merged
jayzhan211 merged 5 commits into
apache:mainfrom
eejbyfeldt:i10883-regr-to-udaf
Jun 13, 2024
Merged

Move Regr_* functions to use UDAF#10898
jayzhan211 merged 5 commits into
apache:mainfrom
eejbyfeldt:i10883-regr-to-udaf

Conversation

@eejbyfeldt

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#10883 and is part of #8708

Rationale for this change

The motivation is in #8708

What changes are included in this PR?

This moves Regr to be implemented using UDAF as decided in #8708. To avoid duplicating to much code I had to modify an existing macros and add new one to avoid creating a lot of boilerplate for each one. Let me know if there is some better way of doing it.

Are these changes tested?

Existing tests cases.

Are there any user-facing changes?

I think so since we moved the implementations to a different package.

@github-actionsgithub-actionsBot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jun 13, 2024

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @eejbyfeldt -- this looks awesome

@alamb

Copy link
Copy Markdown
Contributor

I took the liberty of running cargo fmt and regenerated the vendored data

@alambalamb mentioned this pull request Jun 13, 2024
@alamb

Copy link
Copy Markdown
Contributor

The macos failure https://github.com/apache/datafusion/actions/runs/9502875641/job/26191967199?pr=10898 appears to be related to #10904 (a flaky fuzz test)

@alamb

Copy link
Copy Markdown
Contributor

Merged up to get the fix for #10904

@alamb

Copy link
Copy Markdown
Contributor

🎉

@jayzhan211

Copy link
Copy Markdown
Contributor

Thanks @eejbyfeldt and @alamb

@jayzhan211
jayzhan211 merged commit cc60278 into apache:mainJun 13, 2024
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
* Move Regr_* functions to use UDAF
Closesapache#10883 and is part of apache#8708
* Format and regen
* tweak error check
---------
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jul 24, 2024
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jul 24, 2024
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jul 25, 2024
andygrove pushed a commit to apache/datafusion-python that referenced this pull request Jul 31, 2024
* 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
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-exprLogical plan and expressionsphysical-exprChanges to the physical-expr cratessqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert Regr to UDAF

3 participants

@eejbyfeldt@alamb@jayzhan211