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 approx_distinct to UDAF #10851

Merged
merged 2 commits into from
Jun 11, 2024
Merged

Convert approx_distinct to UDAF #10851

merged 2 commits into from
Jun 11, 2024

Conversation

Lordworms
Copy link
Contributor

Which issue does this PR close?

Closes #10837

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions labels Jun 10, 2024
@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jun 10, 2024
@Lordworms Lordworms marked this pull request as ready for review June 10, 2024 04:34
goldmedal
goldmedal reviewed Jun 10, 2024
View reviewed changes
Copy link
Contributor

@goldmedal goldmedal left a comment

Choose a reason for hiding this comment

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

Thanks @Lordworms
I think overall good but some suggestions.

datafusion/functions-aggregate/src/approx_distinct.rs Outdated

pub struct ApproxDistinct {
signature: Signature,
alias: Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

ApproxDistinct has no any alias. I think we can just remove this field and use default AggregateUDFImpl::alias

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

datafusion/physical-expr/src/aggregate/hyperloglog.rs Outdated
@@ -115,6 +115,7 @@ where

/// Get the register histogram (each value in register index into
/// the histogram; u32 is enough because we only have 2**14=16384 registers
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

After moving to function-aggregate, I think we can remove the original file datafusion/physical-expr/src/aggregate/hyperloglog.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

datafusion/physical-expr/src/expressions/mod.rs Outdated
@@ -64,6 +63,7 @@ pub use crate::window::ntile::Ntile;
pub use crate::window::rank::{dense_rank, percent_rank, rank, Rank, RankType};
pub use crate::window::row_number::RowNumber;
pub use crate::PhysicalSortExpr;
pub use datafusion_functions_aggregate::approx_distinct::ApproxDistinct;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we don't need to public use it.

Lordworms added 2 commits June 10, 2024 11:18
@Lordworms
Convert approx_distinct to UDAF
88a770a 
proto style

fix prost

fix clippy

fix doc

fix clippy
@Lordworms
refactor code
bc01392
@github-actions github-actions bot added the core Core DataFusion crate label Jun 10, 2024
@Lordworms
Copy link
Contributor Author

I have resolved all the reviews, plz merge this first to avoid an extra conflicts solve, Thanks for your review @goldmedal

@jayzhan211
Copy link
Contributor

Thanks @Lordworms and @goldmedal

@jayzhan211 jayzhan211 merged commit 11e143c into apache:main Jun 11, 2024
23 checks passed
@jayzhan211 jayzhan211 mentioned this pull request Jun 12, 2024
Open
@alamb alamb mentioned this pull request Jun 17, 2024
Closed
5 tasks
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
@Lordworms @findepi
Convert approx_distinct to UDAF (apache#10851)
c1e3b87 
* Convert approx_distinct to UDAF

proto style

fix prost

fix clippy

fix doc

fix clippy

* refactor code
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jul 24, 2024
@Michael-J-Ward
migrate approx_percentile_cont, approx_distinct, and approx_median to…
1400069 
… UDAF

Ref: approx_distinct apache/datafusion#10851
Ref: approx_median apache/datafusion#10840
Ref: approx_percentile_cont and _with_weight apache/datafusion#10917
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jul 24, 2024
@Michael-J-Ward
migrate approx_percentile_cont, approx_distinct, and approx_median to…
7d215ad 
… UDAF

Ref: approx_distinct apache/datafusion#10851
Ref: approx_median apache/datafusion#10840
Ref: approx_percentile_cont and _with_weight apache/datafusion#10917
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jul 25, 2024
@Michael-J-Ward
migrate approx_percentile_cont, approx_distinct, and approx_median to…
f4a0828 
… UDAF

Ref: approx_distinct apache/datafusion#10851
Ref: approx_median apache/datafusion#10840
Ref: approx_percentile_cont and _with_weight apache/datafusion#10917
andygrove pushed a commit to apache/datafusion-python that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goldmedal goldmedal goldmedal left review comments

@jayzhan211 jayzhan211 jayzhan211 approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert approx_distinct to UDAF
3 participants
@Lordworms @jayzhan211 @goldmedal