Infer count() aggregation is not null by findepi · Pull Request #11256 · apache/datafusion

findepi · 2024-07-03T16:34:31Z

count([DISTINCT [expr]]) aggregate function never returns null. Infer non-nullness of such aggregate expression. This allows elimination of the HAVING filter for a query such as

SELECT ... count(*) AS c
FROM ...
GROUP BY ...
HAVING c IS NOT NULL

findepi · 2024-07-03T19:06:15Z

Added unit and integration tests. Should be ready to review.

datafusion/optimizer/tests/optimizer_integration.rs

findepi · 2024-07-03T20:31:10Z

@alamb PTAL

alamb · 2024-07-03T21:26:23Z

Thank you very much @findepi -- for both this PR and your other recent contributions. I will indeed review this PR, though I may not get to it until tomorrow or Friday

jonahgao · 2024-07-04T14:26:05Z

I think there might be a simpler way to achieve this.

We can declare the count aggregation expr as non-nullable like this, and then the simplify_expressions rule and the eliminate_filter rule will handle subsequent processing.

datafusion/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

Lines 1450 to 1454 in 6e63748

    
           Expr::IsNotNull(expr) | Expr::IsNotUnknown(expr) 
        
               if !info.nullable(&expr)? => 
        
           { 
        
               Transformed::yes(lit(true)) 
        
           }

findepi · 2024-07-04T19:07:33Z

@jonahgao thanks!
I think we may later choose to add a similar optimizer to the one that was proposed here for the sake of filter expressions. Let's not add one if not needed today.

findepi · 2024-07-04T19:26:01Z

datafusion/expr/src/expr_schema.rs

                match func_def {
+                    // TODO: min/max over non-nullable input should be recognized as non-nullable
                    AggregateFunctionDefinition::BuiltIn(fun) => fun.nullable(),
                    // TODO: UDF should be able to customize nullability


To address this TODO we could extend the pattern here

datafusion/datafusion/expr/src/aggregate_function.rs

Lines 84 to 88 in 4bc3228

pub fn return_type(

&self,

input_expr_types: &[DataType],

input_expr_nullable: &[bool],

) -> Result<DataType> {

ie now we have return_type(input_types, nullability) -> return_type
we could have return_type(input_types, nullability) -> (return_type, nullability)

this is an API for builtin functions, we would need similar API for UDAFs

datafusion/datafusion/expr/src/udaf.rs

Line 330 in 5f02c8a

fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;

Obviously, this would be a breaking change: both method signature and return type would change.
We can avoid the breaking change by making nullability a separate method, but it would be good to first determine the ideal end-state, and only then think how to get there.

copied into #11274

datafusion/expr/src/expr_schema.rs

jonahgao

Looks good to me. Thanks @findepi . Other questions could be resolved through subsequent PRs.

`count([DISTINCT [expr]])` aggregate function never returns null. Infer non-nullness of such aggregate expression. This allows elimination of the HAVING filter for a query such as SELECT ... count(*) AS c FROM ... GROUP BY ... HAVING c IS NOT NULL

findepi · 2024-07-05T20:23:44Z

thanks @jonahgao for your review! updated.

* Fix test function name typo * Improve formatting * Infer count() aggregation is not null `count([DISTINCT [expr]])` aggregate function never returns null. Infer non-nullness of such aggregate expression. This allows elimination of the HAVING filter for a query such as SELECT ... count(*) AS c FROM ... GROUP BY ... HAVING c IS NOT NULL

In apache#11256 it started inferring, for logical plans only, that count() aggregation does not return nulls. However, it did not introduce the counterpart inference for physical plans. Somehow, this discrepancy did not get detected by DF tests, but we run into it with, e.g. `sdf run -q "SELECT count(*) FROM (VALUES (3, 'c')) as T(c, x) GROUP BY c "` The fix here is ad hoc. We should keep an eye on apache#11274 where the goal seems to make UDAFs to communicate nullability of their result. If done right, that should have affect both logical and physical plans.

github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Jul 3, 2024

findepi force-pushed the findepi/infer-count-aggregation-is-not-null-64b31c branch from f346b75 to 2cba7df Compare July 3, 2024 16:35

findepi marked this pull request as draft July 3, 2024 16:35

This comment was marked as resolved.

Sign in to view

findepi force-pushed the findepi/infer-count-aggregation-is-not-null-64b31c branch from 2cba7df to 37019a5 Compare July 3, 2024 17:49

findepi marked this pull request as ready for review July 3, 2024 17:49

findepi force-pushed the findepi/infer-count-aggregation-is-not-null-64b31c branch 3 times, most recently from f9dadfe to fe8fdef Compare July 3, 2024 19:06

findepi force-pushed the findepi/infer-count-aggregation-is-not-null-64b31c branch from fe8fdef to d839797 Compare July 3, 2024 19:08

findepi commented Jul 3, 2024

View reviewed changes

datafusion/optimizer/tests/optimizer_integration.rs Show resolved Hide resolved

findepi force-pushed the findepi/infer-count-aggregation-is-not-null-64b31c branch from d839797 to 0352eda Compare July 3, 2024 19:28

findepi force-pushed the findepi/infer-count-aggregation-is-not-null-64b31c branch from 0352eda to ba27f93 Compare July 4, 2024 19:07

github-actions bot added logical-expr Logical plan and expressions and removed sqllogictest SQL Logic Tests (.slt) labels Jul 4, 2024

findepi commented Jul 4, 2024

View reviewed changes

Fix test function name typo

12b1db6

findepi force-pushed the findepi/infer-count-aggregation-is-not-null-64b31c branch from ba27f93 to 24f4ff1 Compare July 4, 2024 19:38

findepi mentioned this pull request Jul 4, 2024

Decide whether we want to standardize which assert_eq! parameter ("left" or "right") should be used for "expected" value #11270

Closed

Improve formatting

f7ceed3

findepi force-pushed the findepi/infer-count-aggregation-is-not-null-64b31c branch from 24f4ff1 to 53b34ae Compare July 4, 2024 19:55

github-actions bot added the core Core DataFusion crate label Jul 4, 2024

jonahgao reviewed Jul 5, 2024

View reviewed changes

datafusion/expr/src/expr_schema.rs Outdated Show resolved Hide resolved

jayzhan211 mentioned this pull request Jul 5, 2024

Customizable Nullability for UDAF #11274

Closed

jonahgao approved these changes Jul 5, 2024

View reviewed changes

Infer count() aggregation is not null

1a7d541

`count([DISTINCT [expr]])` aggregate function never returns null. Infer non-nullness of such aggregate expression. This allows elimination of the HAVING filter for a query such as SELECT ... count(*) AS c FROM ... GROUP BY ... HAVING c IS NOT NULL

findepi force-pushed the findepi/infer-count-aggregation-is-not-null-64b31c branch from 53b34ae to 1a7d541 Compare July 5, 2024 20:15

jonahgao merged commit 13cb65e into apache:main Jul 6, 2024

findepi deleted the findepi/infer-count-aggregation-is-not-null-64b31c branch July 6, 2024 15:17

findepi added a commit to findepi/datafusion that referenced this pull request Oct 23, 2024

fixup! Physical-plan counterpart for DF apache#11256.

e46a004

findepi added a commit to sdf-labs/datafusion that referenced this pull request Oct 23, 2024

fixup! Physical-plan counterpart for DF apache#11256.

7a7fa5f

findepi added a commit to sdf-labs/datafusion that referenced this pull request Oct 23, 2024

fixup! fixup! Physical-plan counterpart for DF apache#11256.

e576c96

findepi added a commit to sdf-labs/datafusion that referenced this pull request Oct 23, 2024

fixup! fixup! Physical-plan counterpart for DF apache#11256.

eceab18

findepi added a commit to sdf-labs/datafusion that referenced this pull request Oct 23, 2024

fixup! Physical-plan counterpart for DF apache#11256.

0679be2

findepi mentioned this pull request Oct 23, 2024

Fix tests on 41 branch and temporarily disable those unfixable sdf-labs/datafusion#71

Merged

findepi added a commit to sdf-labs/datafusion that referenced this pull request Oct 24, 2024

fixup! Physical-plan counterpart for DF apache#11256.

43cd9df

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Infer count() aggregation is not null#11256

Infer count() aggregation is not null#11256
jonahgao merged 3 commits intoapache:mainfrom
findepi:findepi/infer-count-aggregation-is-not-null-64b31c

findepi commented Jul 3, 2024 •

edited

Loading

Uh oh!

This comment was marked as resolved.

findepi commented Jul 3, 2024

Uh oh!

Uh oh!

findepi commented Jul 3, 2024

Uh oh!

alamb commented Jul 3, 2024

Uh oh!

jonahgao commented Jul 4, 2024 •

edited

Loading

Uh oh!

findepi commented Jul 4, 2024

Uh oh!

findepi Jul 4, 2024

Uh oh!

findepi Jul 5, 2024

Uh oh!

Uh oh!

jonahgao left a comment

Uh oh!

findepi commented Jul 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	pub fn return_type(
	&self,
	input_expr_types: &[DataType],
	input_expr_nullable: &[bool],
	) -> Result<DataType> {

Comments

Conversation

findepi commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

findepi commented Jul 3, 2024

Uh oh!

Uh oh!

findepi commented Jul 3, 2024

Uh oh!

alamb commented Jul 3, 2024

Uh oh!

jonahgao commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Jul 4, 2024

Uh oh!

findepi Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

findepi Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahgao left a comment

Choose a reason for hiding this comment

Uh oh!

findepi commented Jul 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

findepi commented Jul 3, 2024 •

edited

Loading

jonahgao commented Jul 4, 2024 •

edited

Loading