Move Median to `functions-aggregate` and Introduce Numeric signature #10644

jayzhan211 · 2024-05-24T01:30:29Z

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-05-24T14:04:20Z

datafusion/functions-aggregate/src/median.rs

-            data_type,
-            distinct,
+            aliases: vec!["median".to_string()],
+            signature: Signature::numeric(1, Volatility::Immutable),


I introduce this signature that computes the final coerce types instead of generating tons of valid types with the possible same type. The numeric signature considers decimal types too, which is needed but not included in the Numeric types array.

jayzhan211 · 2024-05-24T14:12:05Z

datafusion/functions-aggregate/Cargo.toml

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

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


for downcast_integer

is the issue that not re-exported in arrow?

yes, some macro has inevitable arrow-schema dependency apache/arrow-rs#5676.

alamb

Love it -- thank you @jayzhan211

alamb · 2024-05-25T10:17:15Z

datafusion/expr/src/signature.rs

@@ -119,6 +119,8 @@ pub enum TypeSignature {
    OneOf(Vec<TypeSignature>),
    /// Specifies Signatures for array functions
    ArraySignature(ArrayFunctionSignature),
+    /// Fixed number of arguments of numeric types


Could we please document (or link to documentation) about what types are "numeric"? Is it https://docs.rs/arrow/latest/arrow/datatypes/enum.DataType.html#method.is_numeric ?

alamb · 2024-05-25T10:18:05Z

datafusion/functions-aggregate/Cargo.toml

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

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


is the issue that not re-exported in arrow?

alamb · 2024-05-25T10:19:24Z

datafusion/functions-aggregate/src/median.rs


-/// MEDIAN aggregate expression. If using the non-distinct variation, then this uses a


I think this comment still holds and would be valuable to bring to the new Median UDF

Specifically that median uses substantial memory

alamb · 2024-05-25T10:21:14Z

datafusion/optimizer/src/single_distinct_to_groupby.rs

@@ -257,6 +257,56 @@ impl OptimizerRule for SingleDistinctToGroupBy {
                                )))
                            }
                        }
+                        Expr::AggregateFunction(AggregateFunction {


From what I can tell this is now a general optimization (as in it will rewrite any distinct user defined aggregate as well). If so, that is quite cool. Is it correct?

Can we please add a test (if not already done) showing that the distinct aggregate is rewritten? Or perhaps there is already a test for median 🤔

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-05-25T23:43:33Z

datafusion/sqllogictest/test_files/aggregate.slt

+logical_plan
+01)Projection: MEDIAN(alias1) AS MEDIAN(DISTINCT t.c)
+02)--Aggregate: groupBy=[[]], aggr=[[MEDIAN(alias1)]]
+03)----Aggregate: groupBy=[[t.c AS alias1]], aggr=[[]]


This shows distinct is optimzed

Signed-off-by: jayzhan211 <[email protected]>

alamb · 2024-05-26T10:02:35Z

Thanks again @jayzhan211

…pache#10644) * introduce median udaf Signed-off-by: jayzhan211 <[email protected]> * rm agg median Signed-off-by: jayzhan211 <[email protected]> * rm old median Signed-off-by: jayzhan211 <[email protected]> * introduce numeric signature Signed-off-by: jayzhan211 <[email protected]> * address comment Signed-off-by: jayzhan211 <[email protected]> * fix doc Signed-off-by: jayzhan211 <[email protected]> * add proto roundtrip Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]>

Ref apache/datafusion#10644

* 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

…pache#10644) * introduce median udaf Signed-off-by: jayzhan211 <[email protected]> * rm agg median Signed-off-by: jayzhan211 <[email protected]> * rm old median Signed-off-by: jayzhan211 <[email protected]> * introduce numeric signature Signed-off-by: jayzhan211 <[email protected]> * address comment Signed-off-by: jayzhan211 <[email protected]> * fix doc Signed-off-by: jayzhan211 <[email protected]> * add proto roundtrip Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]>

introduce median udaf

d162015

Signed-off-by: jayzhan211 <[email protected]>

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels May 24, 2024

jayzhan211 mentioned this pull request May 24, 2024

[Epic] Unify AggregateFunction Interface (remove built in list of AggregateFunction s), improve the system #8708

Open

jayzhan211 added 2 commits May 24, 2024 19:22

rm agg median

c9f6386

Signed-off-by: jayzhan211 <[email protected]>

rm old median

6e8e18d

Signed-off-by: jayzhan211 <[email protected]>

github-actions bot added physical-expr Physical Expressions core Core DataFusion crate labels May 24, 2024

jayzhan211 marked this pull request as ready for review May 24, 2024 13:25

jayzhan211 marked this pull request as draft May 24, 2024 13:25

introduce numeric signature

b4ff8ae

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 changed the title ~~Move Median to functions-aggregate~~ Move Median to functions-aggregate and Introduce Numeric signature May 24, 2024

jayzhan211 commented May 24, 2024

View reviewed changes

jayzhan211 marked this pull request as ready for review May 24, 2024 14:31

alamb approved these changes May 25, 2024

View reviewed changes

address comment

f055acb

Signed-off-by: jayzhan211 <[email protected]>

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label May 25, 2024

fix doc

2b2582a

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 commented May 25, 2024

View reviewed changes

add proto roundtrip

371290d

Signed-off-by: jayzhan211 <[email protected]>

alamb merged commit 26b44f4 into apache:main May 26, 2024
25 checks passed

alamb mentioned this pull request May 26, 2024

Fix compilation "comparison_binary_numeric_coercion not found" #10677

Merged

goldmedal mentioned this pull request May 26, 2024

Remove duplicate function name in its aliases list #10661

Merged

alamb mentioned this pull request May 28, 2024

DataFusion weekly project plan (Andrew Lamb) - May 27, 2024 #10699

Closed

9 tasks

Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jun 13, 2024

convert median to expr_fn api

8a3a606

Ref apache/datafusion#10644

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Median to `functions-aggregate` and Introduce Numeric signature #10644

Move Median to `functions-aggregate` and Introduce Numeric signature #10644

jayzhan211 commented May 24, 2024

jayzhan211 May 24, 2024

jayzhan211 May 24, 2024

alamb May 25, 2024

jayzhan211 May 25, 2024

alamb left a comment

alamb May 25, 2024

alamb May 25, 2024

alamb May 25, 2024

alamb May 25, 2024

alamb May 25, 2024

jayzhan211 May 25, 2024

alamb commented May 26, 2024


		/// MEDIAN aggregate expression. If using the non-distinct variation, then this uses a

Move Median to functions-aggregate and Introduce Numeric signature #10644

Move Median to functions-aggregate and Introduce Numeric signature #10644

Conversation

jayzhan211 commented May 24, 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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 26, 2024

Move Median to `functions-aggregate` and Introduce Numeric signature #10644

Move Median to `functions-aggregate` and Introduce Numeric signature #10644