Add Coalesce function by msathis · Pull Request #1969 · apache/datafusion

msathis · 2022-03-09T12:56:10Z

Which issue does this PR close?

Closes #1890.

Rationale for this change

To support coalesce function in SQL

What changes are included in this PR?

Basic coalesce support. ~~Now everything is treated as string, will have to work to support other data types properly.~~

Are there any user-facing changes?

Users can use coalesce function in SQL now

msathis · 2022-03-15T17:33:06Z

@andygrove @xudong963 Please take a look & review when you find time.

Dandandan · 2022-03-15T19:05:08Z

+        for column_value in args {
+            match column_value {
+                ColumnarValue::Array(array_ref) => {
+                    if array_ref.is_valid(i) {


This implementation can likely be optimized (I think it should be >10x faster) by not converting values to ScalarValue and using is_valid, matching per value, but operating directly on Arrays. That would also allow optimizations like skipping running the coalesce althogether if all items in the array already are non null, or skipping handling the array if every value is null, etc.

This will involve some more work / code though, but maybe a comment that there are some big optimizations possible could be useful.

The current implementation also could likely already be optimized a bit by moving the for loop to the innermost place instead of the outermost place and/or implementing one of the rules like mentioned.

yjshen · 2022-03-16T15:15:51Z

@msathis Could you please also add this to Ballista, as #2008 did?

msathis · 2022-03-16T16:28:20Z

@yjshen Yup. Thanks for pointing out. I'll add it to Ballista with the optimization @Dandandan suggested

alamb · 2022-03-21T19:05:31Z

@yjshen Yup. Thanks for pointing out. I'll add it to Ballista with the optimization @Dandandan suggested

@msathis do you plan to do this as a follow on PR or this PR?

In other words is this PR ready for review again or are you still working on it?

liukun4515 · 2022-03-24T01:09:38Z

        BuiltinScalarFunction::Chr => Ok(DataType::Utf8),
+        BuiltinScalarFunction::Coalesce => {
+            // COALESCE has multiple args and they might get coerced, get a preview of this
+            let coerced_types = data_types(input_expr_types, &signature(fun));


Is there any doc or reference to guide for the common result type or the coercion type?

liukun4515 · 2022-03-24T01:18:13Z

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


It's better to add other case with agg function from #2067 (comment)

select COALESCE(sum(c3),0) from table

All the value of c3 is null.
@msathis

Makes sense. i'll update the test case!

you can try this case

select COALESCE(sum(c3),0) from table group by xxx

msathis · 2022-03-24T09:47:47Z

R ready for review again or are you still working on it?

Hi @alamb , Sorry, i got busy with some office work. I'll try to revise the PR in the next couple of days. I was planning to address all the performance optimisations @Dandandan suggested.

alamb · 2022-03-24T11:04:32Z

Thank you @msathis -- makes total sense. Let us know if you need any help!

msathis · 2022-03-31T10:19:49Z

+                }
+                ColumnarValue::Scalar(scalar) => {
+                    if !scalar.is_null() {
+                        // TODO: Figure out how to set value at index


Hi @alamb, I need your help here. I couldn't figure out how to set value to particular index of arrow::Array

I think you may want to call scalar. to_array_of_size(size) here to create the appropriate array:

https://github.com/alamb/arrow-datafusion/blob/41b4e49/datafusion/common/src/scalar.rs#L1075-L1076

Thanks @alamb I think that worked.

liukun4515 · 2022-04-02T02:56:20Z

@msathis If the pr is ready to review, please at me.
Thanks.

alamb

Thank you @msathis -- this is a great PR. I think it could be merged as is, though I have some small comments / suggestions.

I also updated this PR description as it said "everything was treated as a string" which I do not think applies any longer

alamb · 2022-04-02T10:49:23Z

+        "| coalesce(test.c1,test.c2) |",
+        "+---------------------------+",
+        "| 0                         |",
+        "| 1                         |",


alamb · 2022-04-02T10:49:45Z

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


this test seems to be the same as coalesce_plan -- is that intentional?

its an overlook, you're right. i'll remove coalesce_plan

alamb · 2022-04-02T10:49:53Z

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


alamb · 2022-04-02T10:51:43Z

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


stylistically this might be better if it were right next to coalesce_result given its similarity.

For that matter, since it uses the same data setup, it might be cool to combine the two tests (and run two different queries in them)

alamb · 2022-04-02T10:52:33Z

+        "+---------------------------------------------+",
+    ];
+    assert_batches_eq!(expected, &actual);
+    Ok(())


nice testing 👍

alamb · 2022-04-02T10:59:39Z

I will wait for @liukun4515 to review as well prior to merging

msathis · 2022-04-02T17:16:58Z

@alamb Updated the PR to address the review comments

msathis · 2022-04-02T17:21:14Z

@msathis If the pr is ready to review, please at me. Thanks.

@liukun4515 Your review will be great! I think the PR is ready. My knowledge is limited, i feel more optimisation is possible.

alamb · 2022-04-03T11:02:36Z

Thanks @msathis -- I think this is looking great

alamb · 2022-04-05T19:44:43Z

@liukun4515 would you still like to review this prior to merging?

yjshen

LGTM, Thanks @msathis!

liukun4515

LGTM

* feat: coalesce function. WIP * fix: add license header * fix: support all primitive data types * fix: add test case and remove commented code * fix: refactor code and support coalesce from ballista * fix: support scalar values in coalesce * fix: clippy * fix: clippy by removing unneeded mut * fix: clippy by removing unneeded mut * fix: address review comments Co-authored-by: Sathis Kumar <sathis.kumar@udemy.com>

feat: coalesce function. WIP

0266a1e

github-actions bot added the datafusion label Mar 9, 2022

fix: add license header

2706b46

msathis mentioned this pull request Mar 10, 2022

Add support for SQL coalesce function #1890

Closed

msathis added 2 commits March 14, 2022 23:12

fix: support all primitive data types

544a87e

fix: add test case and remove commented code

9ba9a65

msathis marked this pull request as ready for review March 15, 2022 09:45

msathis changed the title ~~Coalesce function - WIP~~ Coalesce function Mar 15, 2022

Dandandan reviewed Mar 15, 2022

View reviewed changes

doki23 mentioned this pull request Mar 16, 2022

Support more ScalarFunction in Ballista #2008

Merged

yjshen added the api change Changes the API exposed to users of the crate label Mar 16, 2022

xudong963 added the enhancement New feature or request label Mar 17, 2022

Dandandan mentioned this pull request Mar 23, 2022

Support sum0 as a built-in agg function #2067

Closed

liukun4515 reviewed Mar 24, 2022

View reviewed changes

msathis added 2 commits March 30, 2022 11:49

Merge remote-tracking branch 'upstream/master' into coalesce_fn

e09224c

fix: refactor code and support coalesce from ballista

527df45

github-actions bot added the ballista label Mar 31, 2022

msathis commented Mar 31, 2022

View reviewed changes

msathis added 2 commits April 1, 2022 13:12

fix: support scalar values in coalesce

34c8e2b

fix: clippy

742e91c

msathis added 2 commits April 2, 2022 11:09

fix: clippy by removing unneeded mut

77f7ba7

fix: clippy by removing unneeded mut

ea3aa56

alamb approved these changes Apr 2, 2022

View reviewed changes

fix: address review comments

348048c

alamb approved these changes Apr 3, 2022

View reviewed changes

alamb changed the title ~~Coalesce function~~ Add Coalesce function Apr 3, 2022

yjshen approved these changes Apr 6, 2022

View reviewed changes

liukun4515 approved these changes Apr 6, 2022

View reviewed changes

yjshen merged commit b890190 into apache:master Apr 6, 2022

Conversation

msathis commented Mar 9, 2022 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

msathis commented Mar 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjshen commented Mar 16, 2022

Uh oh!

msathis commented Mar 16, 2022

Uh oh!

alamb commented Mar 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liukun4515 Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msathis commented Mar 24, 2022

Uh oh!

alamb commented Mar 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liukun4515 commented Apr 2, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb commented Apr 2, 2022

Uh oh!

msathis commented Apr 2, 2022

Uh oh!

msathis commented Apr 2, 2022

Uh oh!

alamb commented Apr 3, 2022

Uh oh!

alamb commented Apr 5, 2022

Uh oh!

yjshen left a comment

Choose a reason for hiding this comment

Uh oh!

liukun4515 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

msathis commented Mar 9, 2022 •

edited by alamb

Loading

liukun4515 Mar 24, 2022 •

edited

Loading