Skip to content

Minor: Extend more style of udaf expr_fn, Remove order args forcovar_samp and covar_pop - #10492

Merged
jayzhan211 merged 2 commits into
apache:mainfrom
jayzhan211:udaf-macro
May 15, 2024
Merged

Minor: Extend more style of udaf expr_fn, Remove order args forcovar_samp and covar_pop#10492
jayzhan211 merged 2 commits into
apache:mainfrom
jayzhan211:udaf-macro

Conversation

@jayzhan211

@jayzhan211jayzhan211 commented May 14, 2024

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #.

Part of #10484

Rationale for this change

Improve expr_fn usage

  1. add expr_fn for count(arg, dictinct) where has additional argument distinct
  2. covariance_pop(arg) where has no any additional argument

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
jayzhan211 marked this pull request as ready for review May 14, 2024 02:21
),
array_replace_all(make_array(vec![lit(1), lit(2), lit(3)]), lit(2), lit(4)),
first_value(vec![lit(1)], false, None, None, None),
covar_samp(lit(1.5), lit(2.2), false, None, None, None),

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.

This change now means the expr_fn can't be used with ordering / null_treatment right? I think that is probably ok

Perhaps we can consider some better API for creating aggregate functions with ORDER BY, perhaps inspired by #6747

Perhaps something like

// form `FIRST_VALUE(a ORDER BY b)`let agg_expr = AggregateUDF::call(first_value()).args(col("a")).order_by(vec![col("b")).build()?;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This change now means the expr_fn can't be used with ordering / null_treatment right? I think that is probably ok

Perhaps we can consider some better API for creating aggregate functions with ORDER BY, perhaps inspired by #6747

Perhaps something like

// form `FIRST_VALUE(a ORDER BY b)`let agg_expr = AggregateUDF::call(first_value()).args(col("a")).order_by(vec![col("b")).build()?;

This looks nice

@alambalamb changed the title Minor: Extend more style of udaf expr_fnMinor: Extend more style of udaf expr_fn, Remove order args forcovar_samp and covar_popMay 14, 2024
@alambalamb added the api change Changes the API exposed to users of the crate label May 14, 2024
@jayzhan211
jayzhan211 merged commit e859426 into apache:mainMay 15, 2024
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jun 13, 2024
AggregateUDF expressions now have a builder API design, which removes arguments like filter and order_by
Ref apache/datafusion#10545
Ref apache/datafusion#10492
andygrove pushed a commit to apache/datafusion-python that referenced this pull request Jun 14, 2024
* deps: update datafusion to 39.0.0, pyo3 to 0.21, and object_store to 0.10.1
`datafusion-common` also depends on `pyo3`, so they need to be upgraded together.
* feat: remove GetIndexField
datafusion replaced Expr::GetIndexField with a FieldAccessor trait.
Ref apache/datafusion#10568
Ref apache/datafusion#10769
* feat: update ScalarFunction
The field `func_name` was changed to `func` as part of removing `ScalarFunctionDefinition` upstream.
Ref apache/datafusion#10325
* feat: incorporate upstream array_slice fixes
Fixes#670
* update ExectionPlan::children impl for DatasetExec
Ref apache/datafusion#10543
* update value_interval_daytime
Ref apache/arrow-rs#5769
* update regexp_replace and regexp_match
Fixes#677
* add gil-refs feature to pyo3
This silences pyo3's deprecation warnings for its new Bounds api.
It's the 1st step of the migration, and should be removed before merge.
Ref https://pyo3.rs/v0.21.0/migration#from-020-to-021
* fix signature for octet_length
Ref apache/datafusion#10726
* update signature for covar_samp
AggregateUDF expressions now have a builder API design, which removes arguments like filter and order_by
Ref apache/datafusion#10545
Ref apache/datafusion#10492
* convert covar_pop to expr_fn api
Ref: https://github.com/apache/datafusion/pull/10418/files
* convert median to expr_fn api
Ref apache/datafusion#10644
* convert variance sample to UDF
Ref apache/datafusion#10667
* convert first_value and last_value to UDFs
Ref apache/datafusion#10648
* checkpointing with a few todos to fix remaining compile errors
* impl PyExpr::python_value for IntervalDayTime and IntervalMonthDayNano
* convert sum aggregate function to UDF
* remove unnecessary clone on double reference
* apply cargo fmt
* remove duplicate allow-dead-code annotation
* update tpch examples for new pyarrow interval
Fixes#665
* marked q11 tpch example as expected fail
Ref #730
* add default stride of None back to array_slice
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
…ar_samp` and `covar_pop` (apache#10492)
* adjust macro args
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
* adjust macro rule
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
---------
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api changeChanges the API exposed to users of the crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jayzhan211@alamb