opt: add support for aggregate FILTER #34077

justinj · 2019-01-17T03:19:33Z

This commit introduces support for FILTERing individual aggregates in a
group by.

One unintuitive change is that if we have a COUNT(*) with a FILTER, we
need to synthesize an input for the aggregation to be over, because
otherwise we have no input to hang the AggFilter off of.

Thus, we convert
COUNT(*) FILTER (WHERE foo)
to
COUNT(true) FILTER (WHERE foo).

This isn't a problem for DISTINCT because COUNT(DISTINCT *) is not valid.

Release note (sql change): FILTER expressions are now supported by the
cost-based optimizer.

cockroach-teamcity · 2019-01-17T03:19:40Z

This change is

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 602 at r1 (raw file):

----
group           ·            ·
 │              aggregate 0  count_rows() FILTER (WHERE k > 5)

How do we get count_rows here if we're building it as count(true)? I was expecting some special code in execbuilder that converts it back but can't see any.

justinj

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 602 at r1 (raw file):

Previously, RaduBerinde wrote…

How do we get count_rows here if we're building it as count(true)? I was expecting some special code in execbuilder that converts it back but can't see any.

Ah, I forgot to regenerate these tests, fixed. Do you think that conversion in execbuilder is important?

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 602 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Ah, I forgot to regenerate these tests, fixed. Do you think that conversion in execbuilder is important?

Nah, this is not a very common case. Plus the main benefit would be to not render the true column and that would be hard to do in the execbuilder.

rytaft

Reviewed 9 of 10 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 2 of 0 LGTMs obtained

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 587 at r2 (raw file):

·               filter       v > 10          ·             ·

# TODO(radu): FILTER not yet supported.

[nit] remove this TODO

pkg/sql/opt/optbuilder/scope.go, line 956 at r2 (raw file):

		}

		s.builder.semaCtx.Properties.Restore(oldProps)

should we be concerned about not restoring props in case of the error above?

pkg/sql/opt/optbuilder/scope.go, line 999 at r2 (raw file):

			if f.Filter != nil {
				// If we have a COUNT(*) with a FILTER, we need to synthesize an input for the aggregation
				// to be over, because otherwise we have no input to hang the AggFilter off of.

[nit] comments should be <= 80 columns

pkg/sql/opt/optbuilder/testdata/aggregate, line 2643 at r2 (raw file):

      └── sum [type=decimal]
           └── agg-filter [type=decimal]
                ├── variable: d [type=decimal]

It's a bit hard to distinguish the input from the filter... maybe it's worth adding labels for input/filter?

andy-kimball

Reviewable status: complete! 2 of 0 LGTMs obtained

pkg/sql/opt/exec/execbuilder/relational_builder.go, line 642 at r2 (raw file):

			child := item.Agg.Child(0)

			if aggFilter, ok := child.(*memo.AggFilterExpr); ok {

Since it appears we're assuming there's just one AggFilterExpr, and it sits above any AggDistinctExpr, can you add some checking to the Memo.checkExpr function? We already have some checks there for the distinct case.

justinj

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @andy-kimball and @rytaft)

pkg/sql/opt/exec/execbuilder/relational_builder.go, line 642 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Since it appears we're assuming there's just one AggFilterExpr, and it sits above any AggDistinctExpr, can you add some checking to the Memo.checkExpr function? We already have some checks there for the distinct case.

Done.

pkg/sql/opt/exec/execbuilder/testdata/aggregate, line 587 at r2 (raw file):

Previously, rytaft wrote…

[nit] remove this TODO

Done.

pkg/sql/opt/optbuilder/scope.go, line 956 at r2 (raw file):

Previously, rytaft wrote…

should we be concerned about not restoring props in case of the error above?

I wouldn't think so, since we bail if that happens, and throw everything away, right?

pkg/sql/opt/optbuilder/scope.go, line 999 at r2 (raw file):

Previously, rytaft wrote…

[nit] comments should be <= 80 columns

Done.

pkg/sql/opt/optbuilder/testdata/aggregate, line 2643 at r2 (raw file):

Previously, rytaft wrote…

It's a bit hard to distinguish the input from the filter... maybe it's worth adding labels for input/filter?

Good idea—done.

rytaft

Reviewed 5 of 5 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @andy-kimball)

pkg/sql/opt/optbuilder/scope.go, line 956 at r2 (raw file):

Previously, justinj (Justin Jaffray) wrote…

I wouldn't think so, since we bail if that happens, and throw everything away, right?

Yea, I think you're right

RaduBerinde · 2019-01-19T19:43:42Z

pkg/sql/opt/optbuilder/scope.go, line 956 at r2 (raw file):

Previously, rytaft wrote…

Yea, I think you're right

The semaCtx is passed externally, and it's probably being reused (https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/plan_opt.go#L232). I think we should restore.

This commit introduces support for FILTERing individual aggregates in a group by. One unintuitive change is that if we have a COUNT(*) with a FILTER, we need to synthesize an input for the aggregation to be over, because otherwise we have no input to hang the AggFilter off of. Thus, we convert COUNT(*) FILTER (WHERE foo) to COUNT(true) FILTER (WHERE foo). This isn't a problem for DISTINCT because COUNT(DISTINCT *) is not valid. Release note (sql change): FILTER expressions are now supported by the cost-based optimizer.

justinj · 2019-01-22T18:10:23Z

Ok, fixed the restoring. TFTRs!

bors r+

34077: opt: add support for aggregate FILTER r=justinj a=justinj This commit introduces support for FILTERing individual aggregates in a group by. One unintuitive change is that if we have a COUNT(*) with a FILTER, we need to synthesize an input for the aggregation to be over, because otherwise we have no input to hang the AggFilter off of. Thus, we convert COUNT(*) FILTER (WHERE foo) to COUNT(true) FILTER (WHERE foo). This isn't a problem for DISTINCT because COUNT(DISTINCT *) is not valid. Release note (sql change): FILTER expressions are now supported by the cost-based optimizer. Co-authored-by: Justin Jaffray <[email protected]>

craig · 2019-01-22T18:45:46Z

Build succeeded

GitHub CI (Cockroach)

A bit more work to address #3998. Summary of changes: - Edit the 'Supported statements' section of the ['Cost-based Optimizer'][1] page to add: - Add FILTER per cockroachdb/cockroach#34077 - Add DELETE per cockroachdb/cockroach#34522 - Add `INSERT .. ON CONFLICT` variants per cockroachdb/cockroach#33339 [1]: http://www.cockroachlabs.com/docs/v2.2/cost-based-optimizer.html

A bit more work to address #3998. Summary of changes: - Edit the 'Supported statements' section of the ['Cost-based Optimizer'][1] page as follows: - Add FILTER per cockroachdb/cockroach#34077 - Add DELETE per cockroachdb/cockroach#34522 - Add `INSERT .. ON CONFLICT` variants per cockroachdb/cockroach#33339 - Remove `experimental_optimizer_updates` cluster setting (can't find a commit for this, but I don't see it in `SHOW ALL` output on my local build of yesterday's `master`, version number is `v2.2.0-alpha.20181217-1096-gd104dcee69-dirty`. [1]: http://www.cockroachlabs.com/docs/v2.2/cost-based-optimizer.html

A bit more work to address #3998. Summary of changes: - Edit the 'Supported statements' section of the ['Cost-based Optimizer'][1] page as follows: - Add DELETE per cockroachdb/cockroach#34522 - Add `INSERT .. ON CONFLICT` variants per cockroachdb/cockroach#33339 - Add FILTER clause on aggregate functions per cockroachdb/cockroach#34077 - Remove `experimental_optimizer_updates` cluster setting (can't find a commit for this, but I don't see it in `SHOW ALL` output on my local build of yesterday's `master`, version number is `v2.2.0-alpha.20181217-1096-gd104dcee69-dirty`. [1]: http://www.cockroachlabs.com/docs/v2.2/cost-based-optimizer.html

A bit more work to address #3998. Summary of changes: - Edit the 'Supported statements' section of the ['Cost-based Optimizer'][1] page as follows: - Add DELETE per cockroachdb/cockroach#34522 - Add `INSERT .. ON CONFLICT` variants per cockroachdb/cockroach#33339 - Add `SELECT`, `VALUES`, and `UNION` statements that do not include window functions - Add FILTER clause on aggregate functions per cockroachdb/cockroach#34077 - Remove `experimental_optimizer_updates` cluster setting [1]: http://www.cockroachlabs.com/docs/v2.2/cost-based-optimizer.html

justinj requested review from rytaft, RaduBerinde, andy-kimball and a team January 17, 2019 03:19

justinj requested a review from a team as a code owner January 17, 2019 03:19

justinj requested a review from a team January 17, 2019 03:19

justinj force-pushed the agg-filter branch from 0b2fd1c to 548e509 Compare January 17, 2019 03:24

RaduBerinde reviewed Jan 17, 2019

View reviewed changes

justinj force-pushed the agg-filter branch from 548e509 to 1ac1539 Compare January 17, 2019 18:31

justinj commented Jan 17, 2019

View reviewed changes

RaduBerinde approved these changes Jan 17, 2019

View reviewed changes

rytaft approved these changes Jan 17, 2019

View reviewed changes

andy-kimball reviewed Jan 17, 2019

View reviewed changes

justinj force-pushed the agg-filter branch from 1ac1539 to 14b9b0d Compare January 18, 2019 22:00

justinj commented Jan 18, 2019

View reviewed changes

rytaft approved these changes Jan 19, 2019

View reviewed changes

justinj force-pushed the agg-filter branch from 14b9b0d to b674076 Compare January 20, 2019 18:01

justinj force-pushed the agg-filter branch from b674076 to ec3a451 Compare January 22, 2019 15:51

justinj requested a review from a team January 22, 2019 15:51

craig bot merged commit ec3a451 into cockroachdb:master Jan 22, 2019

rmloveland mentioned this pull request Feb 7, 2019

Update CBO docs to list more supported statements cockroachdb/docs#4340

Merged

jseldess mentioned this pull request Feb 12, 2019

opt: add support for aggregate FILTER cockroachdb/docs#4365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt: add support for aggregate FILTER #34077

opt: add support for aggregate FILTER #34077

justinj commented Jan 17, 2019

cockroach-teamcity commented Jan 17, 2019

RaduBerinde left a comment

justinj left a comment

RaduBerinde left a comment

rytaft left a comment

andy-kimball left a comment

justinj left a comment

rytaft left a comment

RaduBerinde commented Jan 19, 2019

justinj commented Jan 22, 2019

craig bot commented Jan 22, 2019

opt: add support for aggregate FILTER #34077

opt: add support for aggregate FILTER #34077

Conversation

justinj commented Jan 17, 2019

cockroach-teamcity commented Jan 17, 2019

RaduBerinde left a comment

Choose a reason for hiding this comment

justinj left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

andy-kimball left a comment

Choose a reason for hiding this comment

justinj left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

RaduBerinde commented Jan 19, 2019

justinj commented Jan 22, 2019

craig bot commented Jan 22, 2019

Build succeeded