sql: populate pg_catalog.pg_aggregate table #48126

abhishek20123g · 2020-04-28T20:24:25Z

Fixes #44616

This commit added virtual Schema and populated pg_catalog.pg_aggregate table,
along with its respective testcases.

Release justification: low-risk functionality addition.

Release note (sql change): This PR adds virtual Schema and populate
pg_catalog.pg_aggregate table.

blathers-crl · 2020-04-28T20:24:30Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

@rafiss (commented on sql: populate pg_catalog.pg_aggregate table #44616)
@yuzefovich (author of sql: populate pg_catalog.pg_aggregate table #44616)
@solongordon (commented on sql: populate pg_catalog.pg_aggregate table #44616)
@jordanlewis (commented on sql: populate pg_catalog.pg_aggregate table #44616)

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2020-04-28T20:24:32Z

This change is

abhishek20123g · 2020-04-28T20:44:26Z

Small Overview:

As far as I understand aggregated function in crdb is created differently from psql as we don't uses CREATE AGGREGATE therefore I think aggtransfn,
aggtransfn, aggcombinefn aggserialfn, aggdeserialfn and .. should be
populated as NULL.
I think we could populate aggtransspace by somehow using

cockroach/pkg/sql/sem/tree/aggregate_funcs.go

Line 43 in 03e5567

Size() int64

but I am not sure should we populate it with it.
The logic used for populating aggsortop is non-dynamic in nature though I had cross-checked with psql it is compatible with it. As I was not able to figure out how could be determined sort operator for given aggregate function, therefore, I had to hard code it. If there is more proper way to do this I will update as per you review.

blathers-crl · 2020-04-28T20:51:55Z

❌ The GitHub CI (Cockroach) build has failed on 7e73427f.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

yuzefovich · 2020-04-28T21:55:22Z

@rohany I think you'd be the best person to review this

jordanlewis

Thanks for the submission! I'm fine with leaving aggtransspace as null.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abhishek20123g, @jordanlewis, @rohany, and @solongordon)

pkg/sql/pg_catalog.go, line 2988 at r1 (raw file):

						returnType := tree.NewDOid(tree.DInt(oid.T_bool))
						switch name {
						case "max", "bit_or":

bit_or and bit_and don't have sort ops, but bool_and and bool_or do.

pkg/sql/pg_catalog.go, line 2996 at r1 (raw file):

					err := addRow(
						h.BuiltinOid(name, &overload).AsRegProc(name), // aggfnoid
						tree.NewDString("n"),                          // aggkind

I'm not sure exactly why, but I think this needs to vary - rank, percent_rank, cume_dist, and dense_rank must be h, and all of the percentile ones need to be o.

pkg/sql/pg_catalog.go, line 2997 at r1 (raw file):

						h.BuiltinOid(name, &overload).AsRegProc(name), // aggfnoid
						tree.NewDString("n"),                          // aggkind
						tree.NewDInt(0),                               // aggnumdirectargs

Use tree.DZero

pkg/sql/pg_catalog.go, line 2998 at r1 (raw file):

						tree.NewDString("n"),                          // aggkind
						tree.NewDInt(0),                               // aggnumdirectargs
						tree.DNull,                                    // aggtransfn

That ones that say (zero if none) in the docs should be DZero, not null, I think. Refer to this: https://www.postgresql.org/docs/12/catalog-pg-aggregate.html

pkg/sql/pg_catalog.go, line 3006 at r1 (raw file):

						tree.DNull,                                    // aggminvtransfn
						tree.DNull,                                    // aggmfinalfn
						tree.DNull,                                    // aggfinalextra

This and the next should be DFalse, not null.

blathers-crl · 2020-04-29T15:37:39Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

abhishek20123g · 2020-04-29T16:23:53Z

Thanks for the review @jordanlewis

Made Changes as you suggested 😊

Updated from tree.DNull to tree.DBoolFalse.
bit_or and bit_and don't have sort ops, but bool_and and bool_or do.

My bad I had read it wrong, Corrected it.
Updated aggkind and aggnumdirectargs as per the function name rank, percentile_ ...
For aggtransfn and other made them look as they were in psql, when no functions is present.

regprocForZeroOid := oidZero.AsRegProc("-")

abhishek=# select 0::regproc;
 regproc 
---------
 -
(1 row)

Not sure how these changes breaking testcase in testdata/logic_test/ormsthough I had updated them.
There are some inconsistency between
https://www.postgresql.org/docs/12/catalog-pg-aggregate.html and
https://www.postgresql.org/docs/9.6/catalog-pg-aggregate.html
Though I had followed 9.6 as other schemas are also following it too.

jordanlewis

Looking good!

jordanlewis · 2020-05-02T19:14:45Z

pkg/sql/sqlbase/constants.go

@@ -142,6 +142,7 @@ const (
 	PgCatalogPreparedStatementsTableID
 	PgCatalogPreparedXactsTableID
 	PgCatalogProcTableID
+	PgCatalogAggregateTableID


Please put this in alphabetical order with the others

Sure @jordanlewis
along with that, I have updated PgCatalogID as it was also not in order.

Sorry, but that one should stay where it was, at the top of the PGCatalog definitions, to follow the convention in the file.

Sorry, my bad I should have asked 😊
Corrected

jordanlewis · 2020-05-03T15:42:30Z

pkg/sql/logictest/testdata/logic_test/pg_catalog

+oid  oprname  aggsortop
+
+# Check whether correct operator's oid is set for min, bool_and and every.
+query OTO colnames


These tests look good but I don't quite understand why they don't return any values. A better test would probably produce the oid for min, bool_and and every. same goes for the above.

Sure,
made chnages

Fixes cockroachdb#44616 This commit added virtual Schema and populated pg_catalog.pg_aggregate table, along with its respective testcases. Release justification: low-risk functionality addition. Release note (sql change): This PR adds virtual Schema and populate pg_catalog.pg_aggregate table.

jordanlewis

Thank you @abhishek20123g!

bors r+

craig · 2020-05-03T20:48:21Z

Build succeeded

GitHub CI (Cockroach)

yuzefovich

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abhishek20123g, @jordanlewis, @rohany, and @solongordon)

pkg/sql/pg_catalog.go, line 2997 at r3 (raw file):

							// Cases to determine aggregate kind.
							case "rank", "percent_rank", "cume_dict", "dense_rank":

s/cume_dict/cume_dist/g.

abhishek20123g · 2020-05-04T04:30:32Z

Sorry, @yuzefovich,
I am unable to understand the issue you are pointing at can you please give me briefer at it.
Thanks

yuzefovich · 2020-05-04T14:34:07Z

I meant that there is a typo in the code, cume_dict - it should be cume_dist, and the syntax "s/foo/bar/g" means replace "foo" with "bar".

This PR corrects the typo introduced by following PR cockroachdb#48126 that is update reference of builtin function from `cume_dict` to `cume_dist`. Release note (sql change): None

abhishek20123g · 2020-05-04T16:14:43Z

I meant that there is a typo in the code, cume_dict - it should be cume_dist, and the syntax "s/foo/bar/g" means replace "foo" with "bar".

@yuzefovich thank you for identifying the typo.
I have created a separate PR #48351 as you suggested to correct the typo.

This PR corrects the typo introduced by following PR cockroachdb#48126 that is update reference of builtin function from `cume_dict` to `cume_dist`. Release note: None

48351: sql: remove typo in pg_catalog.pg_aggregate table population. r=yuzefovich a=abhishek20123g This PR corrects the typo introduced by following PR #48126 that is update reference of builtin function from `cume_dict` to `cume_dist`. Release note: None Co-authored-by: abhishek20123g <abhishek20123g@gmail.com>

blathers-crl bot added the X-blathers-triaged blathers was able to find an owner label Apr 28, 2020

blathers-crl bot requested review from jordanlewis, rafiss, solongordon and yuzefovich April 28, 2020 20:24

yuzefovich requested review from rohany and removed request for rafiss and yuzefovich April 28, 2020 21:55

jordanlewis reviewed Apr 29, 2020

View reviewed changes

abhishek20123g force-pushed the sql-populate-pg_catalog.pg_aggregate-table branch from 7e73427 to d33c19f Compare April 29, 2020 15:37

abhishek20123g force-pushed the sql-populate-pg_catalog.pg_aggregate-table branch from d33c19f to adf60ea Compare April 29, 2020 16:49

abhishek20123g requested a review from jordanlewis May 2, 2020 17:19

jordanlewis reviewed May 3, 2020

View reviewed changes

abhishek20123g force-pushed the sql-populate-pg_catalog.pg_aggregate-table branch from adf60ea to 4bcb121 Compare May 3, 2020 16:08

abhishek20123g force-pushed the sql-populate-pg_catalog.pg_aggregate-table branch from 4bcb121 to 4047d45 Compare May 3, 2020 16:36

jordanlewis approved these changes May 3, 2020

View reviewed changes

craig bot merged commit 96f8f33 into cockroachdb:master May 3, 2020

yuzefovich reviewed May 4, 2020

View reviewed changes

abhishek20123g mentioned this pull request May 4, 2020

sql: remove typo in pg_catalog.pg_aggregate table population. #48351

Merged

abhishek20123g deleted the sql-populate-pg_catalog.pg_aggregate-table branch May 6, 2020 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: populate pg_catalog.pg_aggregate table #48126

sql: populate pg_catalog.pg_aggregate table #48126

abhishek20123g commented Apr 28, 2020

blathers-crl bot commented Apr 28, 2020

cockroach-teamcity commented Apr 28, 2020

abhishek20123g commented Apr 28, 2020

blathers-crl bot commented Apr 28, 2020

yuzefovich commented Apr 28, 2020

jordanlewis left a comment

blathers-crl bot commented Apr 29, 2020

abhishek20123g commented Apr 29, 2020

jordanlewis left a comment

jordanlewis May 2, 2020

abhishek20123g May 3, 2020

jordanlewis May 3, 2020

abhishek20123g May 3, 2020

jordanlewis May 3, 2020

abhishek20123g May 3, 2020 •

edited

Loading

jordanlewis left a comment

craig bot commented May 3, 2020

yuzefovich left a comment

abhishek20123g commented May 4, 2020 •

edited

Loading

yuzefovich commented May 4, 2020

abhishek20123g commented May 4, 2020

sql: populate pg_catalog.pg_aggregate table #48126

sql: populate pg_catalog.pg_aggregate table #48126

Conversation

abhishek20123g commented Apr 28, 2020

blathers-crl bot commented Apr 28, 2020

cockroach-teamcity commented Apr 28, 2020

abhishek20123g commented Apr 28, 2020

blathers-crl bot commented Apr 28, 2020

yuzefovich commented Apr 28, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Apr 29, 2020

abhishek20123g commented Apr 29, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

jordanlewis May 2, 2020

Choose a reason for hiding this comment

abhishek20123g May 3, 2020

Choose a reason for hiding this comment

jordanlewis May 3, 2020

Choose a reason for hiding this comment

abhishek20123g May 3, 2020

Choose a reason for hiding this comment

jordanlewis May 3, 2020

Choose a reason for hiding this comment

abhishek20123g May 3, 2020 • edited Loading

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

craig bot commented May 3, 2020

Build succeeded

yuzefovich left a comment

Choose a reason for hiding this comment

abhishek20123g commented May 4, 2020 • edited Loading

yuzefovich commented May 4, 2020

abhishek20123g commented May 4, 2020

abhishek20123g May 3, 2020 •

edited

Loading

abhishek20123g commented May 4, 2020 •

edited

Loading