[built-in function] add `greatest(T,...)` and `least(T,...)` SQL functions #6527

jimexist · 2023-06-02T07:53:02Z

Which issue does this PR close?

Closes Add greatest(T,...) and least(T,...) SQL functions #6531

Rationale for this change

add greatest(T,..) and least(T,..) variadic functions as per SQL 2023.

What changes are included in this PR?

add greatest(T,..) and least(T,..) variadic functions as per SQL 2023.
- when no args are supplied the functions err
- when both scalar and arrays are supplied then scalars are converted to arrays before max/min
- only boolean, string, and numeric types are supported for now, no dates, etc.
- arrow zip and ord kernels are used
change variadic equal function signature to take a vec of allowed types, only types within that types can be used

Are these changes tested?

unit tests
integration tests using tokio and based on sql

Are there any user-facing changes?

jimexist · 2023-06-02T13:44:26Z

datafusion/physical-expr/src/comparison_expressions.rs

+        )))
+    } else {
+        Err(DataFusionError::NotImplemented(format!(
+            "{:?} expressions are not implemented for arrays yet as we need to update arrow kernels",


@alamb is there already a max/min kernel in arrow?

maybe track in this issue:

compute::ord kernel for getting min and max of two scalar/array values arrow-rs#4347

There is min like https://docs.rs/arrow/latest/arrow/compute/fn.min.html but I think it works only for primitive arrays

Perhaps you can use the existing MinAccumulator / Max Accumulators?

https://docs.rs/datafusion/latest/datafusion/physical_plan/expressions/struct.MinAccumulator.html

datafusion/core/tests/sql/expr.rs

2010YOUY01

Nice work! Learned a lot from that 👍🏼

But there seem some problems with the current signature definitions
https://github.com/apache/arrow-datafusion/blob/815413c4a4b473f996ccaa7deb650653430a5aba/datafusion/expr/src/signature.rs#L41-L50
This seems to be the first function that implemented variadic_equal signature, the current implementation said it's
arbitrary number of arguments of an arbitrary but equal type
but the arguments should also be from some common type list for this case (greatest()/least()).

So it's better to change TypeSignature to:

pub enum TypeSignature {
    /// arbitrary number of arguments of an common type out of a list of valid types
    // A function such as `concat` is `Variadic(vec![DataType::Utf8, DataType::LargeUtf8])`
    Variadic(Vec<DataType>),
    /// arbitrary number of arguments of an common type out of a list of valid types, all arguments have the same type
    VariadicEqual(Vec<DataType>),
    /// arbitrary number of arguments of an arbitrary but equal type
    // A function such as `array` is `VariadicEqual`
    // The first argument decides the type used for coercion
    VariadicEqualAny,
    /// arbitrary number of arguments with arbitrary types
    VariadicAny,
    ...

And then the planner can better catch invalid function calls like this one

DataFusion CLI v25.0.0
❯ select least(interval '1 day', interval '2 day', interval '3 day');
thread 'main' panicked at 'Unsupported data type for comparison: Interval(MonthDayNano)', /Users/yongting/Desktop/code/arrow-datafusion/datafusion/physical-expr/src/comparison_expressions.rs:74:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

2010YOUY01 · 2023-06-03T21:15:55Z

datafusion/expr/src/function.rs

@@ -376,6 +381,9 @@ pub fn signature(fun: &BuiltinScalarFunction) -> Signature {
        BuiltinScalarFunction::Chr | BuiltinScalarFunction::ToHex => {
            Signature::uniform(1, vec![DataType::Int64], fun.volatility())
        }
+        BuiltinScalarFunction::Greatest | BuiltinScalarFunction::Least => {
+            Signature::variadic_equal(fun.volatility())


Should be Signature::variadic_equal(SUPPORTED_COMPARISON_TYPES, fun.volatility())

datafusion/expr/src/type_coercion/functions.rs

alamb

Welcome back 👋 @jimexist ! It is great to see you again and thank you for your contribution.

I am sorry I wrote a comment about using Min and Max accumulators previously but I forgot to click submit.

cc @izveigor as I think you have been working to add additional support for array computations

alamb · 2023-06-02T15:49:18Z

datafusion/physical-expr/src/comparison_expressions.rs

+        )))
+    } else {
+        Err(DataFusionError::NotImplemented(format!(
+            "{:?} expressions are not implemented for arrays yet as we need to update arrow kernels",


There is min like https://docs.rs/arrow/latest/arrow/compute/fn.min.html but I think it works only for primitive arrays

Perhaps you can use the existing MinAccumulator / Max Accumulators?

https://docs.rs/datafusion/latest/datafusion/physical_plan/expressions/struct.MinAccumulator.html

alamb · 2023-06-05T19:06:21Z

datafusion/expr/src/signature.rs

    // A function such as `array` is `VariadicEqual`
    // The first argument decides the type used for coercion
-    VariadicEqual,
+    VariadicEqual(Vec<DataType>),


If the types are all equal, what is the purpose of storing a Vec of them? or maybe the Vec is the list of valid types?

It also looks like the code special cases when there are no types specified to mean "any type is allowed" -- can we please explicitly mention that in the docs as well? Or maybe add a new variant (VariadicEqualSpecific?)

alamb · 2023-06-05T19:08:50Z

datafusion/expr/src/type_coercion/functions.rs

+        let is_error = get_valid_types(
+            &signature,
+            &[DataType::Int32, DataType::Boolean, DataType::Int32],
+        )
+        .is_err();
+        assert!(is_error);


Another way to test for error is doing:

Suggested change

let is_error = get_valid_types(

&signature,

&[DataType::Int32, DataType::Boolean, DataType::Int32],

)

.is_err();

assert!(is_error);

let is_error = get_valid_types(

&signature,

&[DataType::Int32, DataType::Boolean, DataType::Int32],

)

.uwrap_err();

Which is both more concise, but I think also prints out what the OK value is when the result is not an Error

alamb · 2023-06-05T19:09:57Z

datafusion/physical-expr/src/comparison_expressions.rs

+}
+
+/// Evaluate a greatest or least function
+fn compare(op: ComparisonOperator, args: &[ColumnarValue]) -> Result<ColumnarValue> {


I think this functionality is basically the same as the Min/Max accumulators (e.g. https://docs.rs/datafusion/latest/datafusion/physical_plan/expressions/struct.MinAccumulator.html)

alamb · 2023-06-05T19:11:44Z

datafusion/core/tests/sql/expr.rs

+}
+
+#[tokio::test]
+async fn test_comparison_func_array_scalar_expression() -> Result<()> {


Can we please write this test using sqllogictest instead of a new rs test? I think you'll find it quite nice for sql tests and it is easier to maintain and extend.

https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests#cookbook-adding-tests

2010YOUY01

Just checked the implementation of Variadic, it's actually the VariadicEqual we are trying to implement:
If a function is given a signature least(Variadic[Int32/Int64], it will try if can coerce to any of least(Int32, Int32, ...) or least(Int64, Int64, ...)
https://github.com/apache/arrow-datafusion/blob/a7970ebf6ef5181fe78de5db63dcf05760db5ace/datafusion/expr/src/type_coercion/functions.rs#L66-L69

But its name and doc are a bit misleading, which makes me ignore that previously and tried to use a new variant, maybe we can also make it more obvious.

2010YOUY01 · 2023-06-05T19:46:03Z

datafusion/expr/src/function.rs

@@ -376,6 +381,12 @@ pub fn signature(fun: &BuiltinScalarFunction) -> Signature {
        BuiltinScalarFunction::Chr | BuiltinScalarFunction::ToHex => {
            Signature::uniform(1, vec![DataType::Int64], fun.volatility())
        }
+        BuiltinScalarFunction::Greatest | BuiltinScalarFunction::Least => {
+            Signature::variadic_equal(


Tried Signature::variadic( and it just work as expected

❯ select least(1,2,3); +-----------------------------------+ | least(Int64(1),Int64(2),Int64(3)) | +-----------------------------------+ | 1 | +-----------------------------------+ 1 row in set. Query took 0.077 seconds. ❯ select least(1,2.0,3); +-------------------------------------+ | least(Int64(1),Float64(2),Int64(3)) | +-------------------------------------+ | 1.0 | +-------------------------------------+ 1 row in set. Query took 0.009 seconds. ❯ select least(interval '1 day', interval '2 day', interval '3 day'); +-----------------------------------------------------------------------------------------------------------------------------------------------+ | least(IntervalMonthDayNano("18446744073709551616"),IntervalMonthDayNano("36893488147419103232"),IntervalMonthDayNano("55340232221128654848")) | +-----------------------------------------------------------------------------------------------------------------------------------------------+ | 0 years 0 mons 1 days 0 hours 0 mins 0.000000000 secs | +-----------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set. Query took 0.008 seconds.

alamb · 2023-06-09T13:37:45Z

Marking as draft as this PR has feedback and is awaiting a response (I am trying to keep the list of PRs needing review clear) -- please mark as ready for review when ready

jimexist · 2023-06-12T07:24:05Z

Welcome back 👋 @jimexist ! It is great to see you again and thank you for your contribution.

I am sorry I wrote a comment about using Min and Max accumulators previously but I forgot to click submit.

cc @izveigor as I think you have been working to add additional support for array computations

thank you @alamb my bandwidth recently is very limited, so I am going to pause this PR for a bit, if anyone wants to take over feel free to. I can also do a merge as is (after rebase fix) and create issues as follow ups?

alamb · 2023-06-12T17:59:01Z

thank you @alamb my bandwidth recently is very limited, so I am going to pause this PR for a bit, if anyone wants to take over feel free to. I can also do a merge as is (after rebase fix) and create issues as follow ups?

Whatever you prefer -- I defer to your judgement

github-actions · 2024-05-07T01:46:45Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

github-actions bot added core Core datafusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions labels Jun 2, 2023

jimexist force-pushed the add-greatest-least branch from 5214393 to b6f9f35 Compare June 2, 2023 13:42

jimexist commented Jun 2, 2023

View reviewed changes

jimexist changed the title ~~[built-in function] add greatest and least~~ [built-in function] add greatest() and least() SQL functions Jun 2, 2023

comphead reviewed Jun 2, 2023

View reviewed changes

datafusion/core/tests/sql/expr.rs Show resolved Hide resolved

jimexist force-pushed the add-greatest-least branch from 4a2c338 to 538e085 Compare June 3, 2023 13:30

2010YOUY01 reviewed Jun 3, 2023

View reviewed changes

jimexist force-pushed the add-greatest-least branch from 85bda83 to 30b575a Compare June 4, 2023 02:20

jimexist commented Jun 4, 2023

View reviewed changes

datafusion/expr/src/type_coercion/functions.rs Outdated Show resolved Hide resolved

jimexist force-pushed the add-greatest-least branch 2 times, most recently from 1b4e6c8 to 74fb1fa Compare June 4, 2023 02:36

jimexist changed the title ~~[built-in function] add greatest() and least() SQL functions~~ [built-in function] add greatest(T,...) and least(T,...) SQL functions Jun 4, 2023

[built-in function] add greatest and least

15bc806

jimexist force-pushed the add-greatest-least branch from 8f68126 to 15bc806 Compare June 4, 2023 05:24

jimexist requested review from comphead and 2010YOUY01 June 5, 2023 00:14

alamb reviewed Jun 5, 2023

View reviewed changes

2010YOUY01 reviewed Jun 5, 2023

View reviewed changes

alamb marked this pull request as draft June 9, 2023 13:37

github-actions bot added the Stale label May 7, 2024

github-actions bot closed this May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[built-in function] add `greatest(T,...)` and `least(T,...)` SQL functions #6527

[built-in function] add `greatest(T,...)` and `least(T,...)` SQL functions #6527

jimexist commented Jun 2, 2023 •

edited

jimexist Jun 2, 2023

jimexist Jun 2, 2023

alamb Jun 2, 2023

2010YOUY01 left a comment

2010YOUY01 Jun 3, 2023

alamb left a comment

alamb Jun 2, 2023

alamb Jun 5, 2023

alamb Jun 5, 2023

alamb Jun 5, 2023

alamb Jun 5, 2023

2010YOUY01 left a comment

2010YOUY01 Jun 5, 2023

alamb commented Jun 9, 2023

jimexist commented Jun 12, 2023

alamb commented Jun 12, 2023

github-actions bot commented May 7, 2024

[built-in function] add greatest(T,...) and least(T,...) SQL functions #6527

[built-in function] add greatest(T,...) and least(T,...) SQL functions #6527

Conversation

jimexist commented Jun 2, 2023 • edited

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

2010YOUY01 left a 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

2010YOUY01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jun 9, 2023

jimexist commented Jun 12, 2023

alamb commented Jun 12, 2023

github-actions bot commented May 7, 2024

[built-in function] add `greatest(T,...)` and `least(T,...)` SQL functions #6527

[built-in function] add `greatest(T,...)` and `least(T,...)` SQL functions #6527

jimexist commented Jun 2, 2023 •

edited