Implement IGNORE NULLS for FIRST_VALUE by huaxingao · Pull Request #9411 · apache/datafusion

huaxingao · 2024-03-01T04:38:09Z

Which issue does this PR close?

Closes #.
Related #9055

Rationale for this change

Spark has ignore null in First_value, so we want to support this option too.
The logic for ignore null in Last_value is similar. I will have a separate PR for ignore null in Last_value

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

mustafasrepo · 2024-03-01T06:17:39Z

datafusion/physical-expr/src/aggregate/first_last.rs

-        Ok((!indices.is_empty()).then_some(indices.value(0) as _))
+
+        if self.is_ignore_null {
+            let indices = lexsort_to_indices(&sort_columns, Some(value.len()))?;


instead you can use

Suggested change

let indices = lexsort_to_indices(&sort_columns, Some(value.len()))?;

let indices = lexsort_to_indices(&sort_columns, None)?;

to be more explicit about we are sorting whole array

Fixed. Thanks!

mustafasrepo · 2024-03-01T06:20:21Z

datafusion/physical-expr/src/aggregate/first_last.rs

+        )
+    }
+
+    pub fn with_ignore_null(


Instead of this API, maybe we can use builder API as below

pub fn with_ignore_null(mut self, ignore_null: bool) -> Self{ self.ignore_null = ignore_null; self }

However, this purely stylistic. Feel free to proceed as you wish

Changed. Thanks for the suggestion!

mustafasrepo

Left some minor comments. Other than these this PR is LGTM!. Thanks @huaxingao for this PR.

viirya · 2024-03-01T06:54:20Z

datafusion/physical-expr/src/aggregate/first_last.rs

    ordering_req: LexOrdering,
    // Stores whether incoming data already satisfies the ordering requirement.
    requirement_satisfied: bool,
+    is_ignore_null: bool,


It might be good to add a comment like above parameters.

Added. Thanks

viirya · 2024-03-01T07:05:38Z

datafusion/physical-expr/src/aggregate/first_last.rs

+                // If ignoring nulls, find the first non-null value.
+                for i in 0..value.len() {
+                    if !value.is_null(i) {
+                        return Ok((!value.is_empty()).then_some(i));


Once you enter this loop, I think value is not empty.

Fixed. Thanks

viirya · 2024-03-01T07:07:27Z

datafusion/physical-expr/src/aggregate/first_last.rs

+            // If ignoring nulls, find the first non-null value.
+            for index in indices.iter().flatten() {
+                if !value.is_null(index as usize) {
+                    return Ok((!value.is_empty()).then_some(index as usize));


viirya

Hmm, I saw you implement it in physical expression FirstValue. But do you add ignore null parameter to logical one and pass it through logical to physical? I.e., does a query with first value + ignore null work now? Can you add an e2e test to sqllogictests?

comphead · 2024-03-01T16:38:06Z

datafusion/physical-expr/src/aggregate/first_last.rs

    expr: Arc<dyn PhysicalExpr>,
    ordering_req: LexOrdering,
    requirement_satisfied: bool,
+    is_ignore_null: bool,


maybe we can use ignore_nulls: bool, it is consistent with SQL keywords and also window functions that in parallel implementing IGNORE NULLS .

Would be also nice to have a comments in struct about this field

Fixed. Thanks

comphead

Thanks @huaxingao
Would be that possible to add tests to aggregate.slt?

comphead · 2024-03-01T16:50:05Z

Hmm, I saw you implement it in physical expression FirstValue. But do you add ignore null parameter to logical one and pass it through logical to physical? I.e., does a query with first value + ignore null work now? Can you add an e2e test to sqllogictests?

It should start with logical plan, from #9221 the null treatment is allowed for window functions now https://github.com/apache/arrow-datafusion/blob/main/datafusion/sql/src/expr/function.rs#L61

For agg I suppose we need to do the same

alamb

Thank you for the contribution @huaxingao -- this looks good to me. I had some style suggestions but nothing that would prevent the PR from merging in my mind

alamb · 2024-03-03T15:20:30Z

datafusion/core/src/physical_planner.rs

            args,
            filter,
            order_by,
+            ..


As as style thing I have found it helpful to not use a .. match all, and instead explicitly ignore each field like

Suggested change

..

null_treatment: _,

With this treatment, if/when someone adds a new field to AggregateFunction in the future, the compiler will tell them all the places that may need to be changed.

Fixed. Thanks for the suggestion!

alamb · 2024-03-03T15:21:25Z

datafusion/physical-expr/src/aggregate/first_last.rs

    expr: Arc<dyn PhysicalExpr>,
    ordering_req: LexOrdering,
    requirement_satisfied: bool,
+    ignore_null: bool,


a very minor nit is that some parts of this PR use ignore_null and some parts use ignore_nulls (with an s). It might be nice to make them uniform

I have change all of them to ignore_nulls. Thanks for catching this!

alamb · 2024-03-03T15:22:15Z

datafusion/substrait/src/logical_plan/producer.rs

 ) -> Result<Measure> {
    match expr {
-        Expr::AggregateFunction(expr::AggregateFunction { func_def, args, distinct, filter, order_by }) => {
+        Expr::AggregateFunction(expr::AggregateFunction { func_def, args, distinct, filter, order_by, .. }) => {


same comment here about matching all vs explicit field matches

Fixed. Thanks!

viirya · 2024-03-03T18:04:50Z

datafusion/expr/src/expr.rs

    pub filter: Option<Box<Expr>>,
    /// Optional ordering
    pub order_by: Option<Vec<Expr>>,
+    pub null_treatment: Option<NullTreatment>,


Maybe add a comment as other parameters did.

Added. Thanks

viirya · 2024-03-03T18:05:22Z

datafusion/expr/src/expr.rs

            }) => {
                fmt_function(f, func_def.name(), *distinct, args, true)?;
+                if let Some(nt) = null_treatment {
+                    write!(f, "{}", nt)?;


Suggested change

write!(f, "{}", nt)?;

write!(f, " {}", nt)?;

Changed. Thanks

viirya · 2024-03-03T18:05:34Z

datafusion/expr/src/expr.rs

                info += &format!(" ORDER BY [{}]", expr_vec_fmt!(order_by));
            };
+            if let Some(nt) = null_treatment {
+                info += &format!("{}", nt);


Suggested change

info += &format!("{}", nt);

info += &format!(" {}", nt);

Changed. Thanks

viirya · 2024-03-03T18:07:51Z

datafusion/physical-expr/src/aggregate/first_last.rs

        }
    }

+    pub fn ignore_null(mut self, ignore_null: bool) -> Self {


For a builder style API, a common API name pattern should be (e.g., the existing with_requirement_satisfied):

Suggested change

pub fn ignore_null(mut self, ignore_null: bool) -> Self {

pub fn with_ignore_null(mut self, ignore_null: bool) -> Self {

Changed. Thanks!

viirya

Looks good to me. I have a few minor comments. Thanks @huaxingao.

… in the output

huaxingao · 2024-03-04T00:12:56Z

Test failed:

External error: query result mismatch:
[SQL] EXPLAIN SELECT DISTINCT i FROM t1000;
[Diff] (-expected|+actual)
    logical_plan
    Aggregate: groupBy=[[t1000.i]], aggr=[[]]
    --TableScan: t1000 projection=[i]
    physical_plan
    AggregateExec: mode=FinalPartitioned, gby=[i@0 as i], aggr=[]
    --CoalesceBatchesExec: target_batch_size=8192
    ----RepartitionExec: partitioning=Hash([i@0], 4), input_partitions=4
    ------AggregateExec: mode=Partial, gby=[i@0 as i], aggr=[]
-   --------MemoryExec: partitions=4, --------MemoryExec: partitions=4, partition_sizes=[1, 1, 2, 1]
+   --------MemoryExec: partitions=4, partition_sizes=[1, 2, 1, 1]
at test_files/limit.slt:392

partition_sizes=[1, 2, 1, 1] should also be correct, right? I got partition_sizes=[1, 1, 2, 1] when running the test on my local.

huaxingao · 2024-03-04T00:18:05Z

datafusion/sqllogictest/test_files/group_by.slt

  s.ts >= e.ts
 GROUP BY s.sn, s.zip_code, s.country, s.ts, s.currency
-ORDER BY s.sn
+ORDER BY s.sn, s.zip_code


The expected result is

0 GRC 0 2022-01-01T06:00:00 EUR 30 1 FRA 1 2022-01-01T08:00:00 EUR 50 1 TUR 2 2022-01-01T11:30:00 TRY 75 1 FRA 3 2022-01-02T12:00:00 EUR 200 0 GRC 4 2022-01-03T10:00:00 EUR 80 1 TUR 4 2022-01-03T10:00:00 TRY 100

but my test has

0 GRC 0 2022-01-01T06:00:00 EUR 30 1 FRA 1 2022-01-01T08:00:00 EUR 50 1 TUR 2 2022-01-01T11:30:00 TRY 75 1 FRA 3 2022-01-02T12:00:00 EUR 200 1 TUR 4 2022-01-03T10:00:00 TRY 100 0 GRC 4 2022-01-03T10:00:00 EUR 80

I think the above is also legal. I added one more column in ORDER BY to make the returned order deterministic.

alamb · 2024-03-04T06:57:55Z

~~I believe the CI failures were due to a logical conflict on main, fixed in #9444~~

~~So merging up from main will likely fix them~~

Update: I think this is unrelated -- I filed #9450 to track

alamb · 2024-03-04T12:01:11Z

Closing / reopening to trigger CI

alamb

Hi @huaxingao -- I am trying to clear the PR backlog to keep the code in this repo moving. I noticed this PR had conflicts due to #8891, so I took the liberty of merging up from main to resolve the conflicts (and hopefully get this PR ready to merge)

Thanks again for the contribution and I am sorry this one has not yet merged.

huaxingao · 2024-03-05T22:45:56Z

Thanks @alamb

alamb · 2024-03-05T23:00:54Z

🚀

huaxingao · 2024-03-05T23:10:11Z

Thank you all very much for helping me with this PR! @alamb @comphead @mustafasrepo @viirya

github-actions bot added the physical-expr Changes to the physical-expr crates label Mar 1, 2024

mustafasrepo reviewed Mar 1, 2024

View reviewed changes

mustafasrepo approved these changes Mar 1, 2024

View reviewed changes

viirya reviewed Mar 1, 2024

View reviewed changes

comphead reviewed Mar 1, 2024

View reviewed changes

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate substrait Changes to the substrait crate labels Mar 2, 2024

Huaxin Gao added 5 commits March 2, 2024 09:51

Implement IGNORE NULLS for FIRST_VALUE

463b189

fix style

0be9115

fix clippy error

0f9a5a1

fix clippy error

6dee063

address comments

d061f06

huaxingao force-pushed the ignore_null_first branch 3 times, most recently from 9cc5ce3 to 366e7ef Compare March 2, 2024 19:11

fix error

b3f8952

huaxingao force-pushed the ignore_null_first branch from 3e84596 to b3f8952 Compare March 2, 2024 19:24

add test to aggregate.slt

226ce01

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

alamb approved these changes Mar 3, 2024

View reviewed changes

viirya reviewed Mar 3, 2024

View reviewed changes

viirya approved these changes Mar 3, 2024

View reviewed changes

viirya reviewed Mar 3, 2024

View reviewed changes

Huaxin Gao added 3 commits March 3, 2024 11:00

address comments

b8c98da

Trigger Build

7eca782

Add one additional column in order by to ensure a deterministic order…

68be32a

… in the output

huaxingao commented Mar 4, 2024

View reviewed changes

alamb closed this Mar 4, 2024

alamb reopened this Mar 4, 2024

This was referenced Mar 4, 2024

Intermittent limit.slt failure #9450

Closed

test: port strpos test in physical_expr/src/functions to sqllogictest #9439

Merged

Merge remote-tracking branch 'apache/main' into ignore_null_first

75be556

alamb reviewed Mar 5, 2024

View reviewed changes

alamb merged commit 3aba67e into apache:main Mar 5, 2024

huaxingao deleted the ignore_null_first branch March 5, 2024 23:10

huaxingao mentioned this pull request Mar 25, 2024

Implement IGNORE NULLS for LAST_VALUE #9801

Merged

	let indices = lexsort_to_indices(&sort_columns, Some(value.len()))?;
	let indices = lexsort_to_indices(&sort_columns, None)?;

	pub fn ignore_null(mut self, ignore_null: bool) -> Self {
	pub fn with_ignore_null(mut self, ignore_null: bool) -> Self {

Conversation

huaxingao commented Mar 1, 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?

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!

mustafasrepo 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!

viirya 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!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

comphead commented Mar 1, 2024

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!

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!

Choose a reason for hiding this comment

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

huaxingao commented Mar 4, 2024

Uh oh!

Choose a reason for hiding this comment

alamb commented Mar 4, 2024 •

edited

Loading