sql: add the getdatabaseencoding() builtin function #45129

abhishek20123g · 2020-02-15T01:55:54Z

Resolves #41771.

This commit adds builtin function, getdatabaseencoding(),
and the unit-test case for it.

Release note (sql change): This PR is introduced to add builtin
function, getdatabaseencoding(), which returns the current encoding
name used by the database.

cockroach-teamcity · 2020-02-15T01:56:03Z

This change is

claassistantio · 2020-02-15T01:56:19Z

All committers have signed the CLA.

rohany

Thanks for the contribution! This needs a test before it can land. You should add a logic test for it (perhaps in pkg/sql/logictest/testdata/logic_test/pg_builtins.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abhishek20123g and @otan)

pkg/sql/sem/builtins/builtins.go, line 3073 at r1 (raw file):

			ReturnType: tree.FixedReturnType(types.String),
			Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
				rows1, err1 := ctx.InternalExecutor.QueryRow(

nit: i'd name these just rows, err.

pkg/sql/sem/builtins/builtins.go, line 3081 at r1 (raw file):

				encodingID, ok := rows1[0].(*tree.DInt)
				if !ok {
					return tree.DNull, errors.New("failed to type assert Datum to DInt")

this should return nil rather than DNull. Additionally, I'd make this an errors.AssertionFailure with more detail. In this case we already know that we couldn't perform the type assertion, but we should have more context on what exactly we expected vs what we found.

pkg/sql/sem/builtins/builtins.go, line 3083 at r1 (raw file):

					return tree.DNull, errors.New("failed to type assert Datum to DInt")
				}
				if err1 != nil {

You need to check if the error is not nil before accessing the result of the query.

pkg/sql/sem/builtins/builtins.go, line 3084 at r1 (raw file):

				}
				if err1 != nil {
					return tree.DNull, err1

same, this should be nil.

pkg/sql/sem/builtins/builtins.go, line 3088 at r1 (raw file):

				// Using pg_encoding_to_char() to convert an encoding ID to an encoding name.
				rows2, err2 := ctx.InternalExecutor.QueryRow(

Why not just call pg_encoding_to_char in your first query?

abhishek20123g · 2020-02-16T02:48:07Z

Hi @rohany
Thanks for suggesting changes, changes had been made as requested.
I have combined two separate queries for evaluating encoding_id and converting encoding_id to encoding name into one query (which makes more sense).
I am returning nil instead of DNull.
Refactored row1, err1 to row, err, respectively.

wrt Adding test-cases for getdatabaseencoding(),
I had added test-case in pkg/sql/logictest/testdata/logic_test/builtin_function, I am not sure about whether i should add unit test-case in this file or in other file. I have decided to add a test-case in this file because getdatabasencoding() is a builtin function, and most of the functions related to it are also present in this file.
And I was only able to add test-case corresponding to UTF8 because perhaps CockroachDB currently only supports UTF8 encoding.

pkg/sql/sem/builtins/builtins.go

rohany · 2020-02-16T04:59:54Z

The test looks good! We do only support UTF8 right now.

Some more nits:

Can you squash these commits into a single commit?
Can you rename the commit to "sql: add the getdatabaseencoding() builtin function"
Please use the commit template within cockroach to write a commit message. Importantly, your commit needs a release note. The instructions for this are in the commit template.

abhishek20123g · 2020-02-16T05:18:29Z

Absolutely, I will squash all the commit into one, and make them look as recommended.

rohany · 2020-02-16T15:50:50Z

pkg/sql/sem/builtins/builtins.go

+			Types:      tree.ArgTypes{},
+			ReturnType: tree.FixedReturnType(types.String),
+			Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
+				// computing encoding name bypassing encoding_id which is


Can you change this comment to

"Compute encoding name by calling pg_encoding_to_char on the encoding from pg_database."

All comments should be complete sentences!

rohany · 2020-02-16T15:51:14Z

Aside from my nit about the comment, this looks good! Once you fix that I will merge the PR.

abhishek20123g · 2020-02-16T18:20:55Z

Hi @rohany,
I have changed comment as recommended.

And thank you for guiding me, at various points in this PR.
For my future PR's, I will try to make them easy to merge.

rohany · 2020-02-16T22:45:35Z

Can you rebase this onto master? It looks like you're running into some sort of test flake.

otan · 2020-02-16T23:48:35Z

pkg/sql/sem/builtins/builtins.go

+				row, err := ctx.InternalExecutor.QueryRow(
+					ctx.Ctx(), "getdatabaseencoding",
+					ctx.Txn,
+					"SELECT pg_encoding_to_char(encoding) "+


i think this should go in pg_builtins.go, and we should probably try re-using

cockroach/pkg/sql/sem/builtins/pg_builtins.go

Line 567 in bb14400

if args[0].Compare(ctx, DatEncodingUTFId) == 0 {

rather than trying to call a nested database query.

Sure, I will move it to pg_builtins.go
and you mean rather calling pg_encoding_to_char inside the query, I should use the same logic in pg_encoding_to_char to convert encodingID to encoding name.

can we do it by creating some kind of external helper function which converts encodingID to the encoding name which is being called by both, pg_encoding_to_char and getdatabaseencoding?
because otherwise, we would have update logics inside both functions for adding support to another encoding type.

can we do it by creating some kind of external helper function which converts encodingID to the encoding name which is being called by both, pg_encoding_to_char and getdatabaseencoding?

yes that would be my suggestion :)

sure, I will update.

pkg/sql/logictest/testdata/logic_test/builtin_function

abhishek20123g · 2020-02-17T07:31:05Z

hi @otan,
I have moved getdatabaseencoding() form bulitins.go to pg_builtins.go .
Created a helper function called getEncodingNameForEncodingID.
Updated the defination of pg_encoding_to_char and getdatabaseencoding .

Do I have to rebase changes to master as suggested by rohany ?

otan · 2020-02-17T08:23:18Z

pkg/sql/sem/builtins/pg_builtins.go

+				row, err := ctx.InternalExecutor.QueryRow(
+					ctx.Ctx(), "getdatabaseencoding",
+					ctx.Txn,
+					"SELECT encoding FROM pg_database WHERE datname = $1", ctx.SessionData.Database)


sorry bit slow to realise this - looks like we're always returning UTF-8 for this anyway. We populate pg_database with a virtual schema, and we always return the same value of UTF-8 for this query anyway:

cockroach/pkg/sql/pg_catalog.go

Line 1084 in bb14400

builtins.DatEncodingUTFId, // encoding

Perhaps we should also just return UTF-8 here as well. Make a comment on the virtual schema that if we ever change the value of that, we should change it here and vice versa.

Sorry for the round-aboutness with getting this out there.

(note: i would generally try avoid calling another query inside here unless you had to, which led me down this path to finding this :] ideally, if it's configurable, we'd have a function that lets us figure this out without shelling out to another query)

So, rather than using InternalExecutor to execute the query you are recommending me to
just return UTF-8 and add a comment to virtual schema to that if we change value there we update it here too.

like use in pg_client_encoding in pg_builtins.go

ajwerner · 2020-02-17T16:09:58Z

Flaked on #44944.

Resolves cockroachdb#41771. This commit adds builtin function, getdatabaseencoding(), and the unit-test case for it. Release note (sql change): This PR is introduced to add builtin function, getdatabaseencoding(), which returns the current encoding name used by the database.

otan

looks good to me - i made a minor jiggle with the comments for you.

thanks for contributing and hope to see more!

otan · 2020-02-18T08:06:35Z

bors r=otan

45129: sql: add the getdatabaseencoding() builtin function r=otan a=abhishek20123g Resolves #41771. This commit adds builtin function, getdatabaseencoding(), and the unit-test case for it. Release note (sql change): This PR is introduced to add builtin function, getdatabaseencoding(), which returns the current encoding name used by the database. Co-authored-by: abhishek20123g <abhishek20123g@gmail.com>

i have taken over!

craig · 2020-02-18T08:45:24Z

Build succeeded

GitHub CI (Cockroach)

abhishek20123g · 2020-02-18T10:45:36Z

thanks, @rohany, and @otan for helping me out

Can you recommend to me some issues where I could start working?
thanks

rohany · 2020-02-18T19:51:43Z

Our good first issue page has a ton of issues that you could take a look at!

abhishek20123g requested a review from otan February 15, 2020 12:23

rohany previously requested changes Feb 15, 2020

View reviewed changes

abhishek20123g force-pushed the sql-add-getdatabaseencoding-function branch from 78733d0 to ed32ba7 Compare February 16, 2020 01:59

rohany reviewed Feb 16, 2020

View reviewed changes

pkg/sql/sem/builtins/builtins.go Outdated Show resolved Hide resolved

abhishek20123g force-pushed the sql-add-getdatabaseencoding-function branch from 8251bfe to e17cd68 Compare February 16, 2020 06:50

rohany reviewed Feb 16, 2020

View reviewed changes

abhishek20123g force-pushed the sql-add-getdatabaseencoding-function branch from e17cd68 to 00e87fa Compare February 16, 2020 18:12

otan reviewed Feb 16, 2020

View reviewed changes

pkg/sql/logictest/testdata/logic_test/builtin_function Show resolved Hide resolved

abhishek20123g force-pushed the sql-add-getdatabaseencoding-function branch 2 times, most recently from 8895468 to 9850f90 Compare February 17, 2020 07:22

abhishek20123g force-pushed the sql-add-getdatabaseencoding-function branch from 9850f90 to 7986c87 Compare February 17, 2020 08:16

otan reviewed Feb 17, 2020

View reviewed changes

abhishek20123g force-pushed the sql-add-getdatabaseencoding-function branch from 7986c87 to 400afb1 Compare February 17, 2020 10:30

abhishek20123g closed this Feb 17, 2020

abhishek20123g force-pushed the sql-add-getdatabaseencoding-function branch from 400afb1 to 3c2dcbc Compare February 17, 2020 12:29

abhishek20123g reopened this Feb 17, 2020

otan changed the title ~~sql: Adding getdatabaseencoding() function, with respect to #issue 41771~~ sql: add the getdatabaseencoding() builtin function Feb 18, 2020

otan force-pushed the sql-add-getdatabaseencoding-function branch from 400afb1 to ecb7ffc Compare February 18, 2020 08:05

otan approved these changes Feb 18, 2020

View reviewed changes

otan requested a review from rohany February 18, 2020 08:06

craig bot merged commit ecb7ffc into cockroachdb:master Feb 18, 2020

jseldess mentioned this pull request Mar 2, 2020

sql: add the getdatabaseencoding() builtin function cockroachdb/docs#6734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add the getdatabaseencoding() builtin function #45129

sql: add the getdatabaseencoding() builtin function #45129

abhishek20123g commented Feb 15, 2020 •

edited by rohany

Loading

cockroach-teamcity commented Feb 15, 2020

claassistantio commented Feb 15, 2020 •

edited

Loading

rohany left a comment

abhishek20123g commented Feb 16, 2020 •

edited

Loading

rohany commented Feb 16, 2020

abhishek20123g commented Feb 16, 2020

rohany Feb 16, 2020

rohany commented Feb 16, 2020

abhishek20123g commented Feb 16, 2020

rohany commented Feb 16, 2020

otan Feb 16, 2020

abhishek20123g Feb 17, 2020

otan Feb 17, 2020

abhishek20123g Feb 17, 2020

abhishek20123g commented Feb 17, 2020

otan Feb 17, 2020 •

edited

Loading

otan Feb 17, 2020

abhishek20123g Feb 17, 2020

ajwerner commented Feb 17, 2020

otan left a comment

otan commented Feb 18, 2020

craig bot commented Feb 18, 2020

abhishek20123g commented Feb 18, 2020

rohany commented Feb 18, 2020

sql: add the getdatabaseencoding() builtin function #45129

sql: add the getdatabaseencoding() builtin function #45129

Conversation

abhishek20123g commented Feb 15, 2020 • edited by rohany Loading

cockroach-teamcity commented Feb 15, 2020

claassistantio commented Feb 15, 2020 • edited Loading

rohany left a comment

Choose a reason for hiding this comment

abhishek20123g commented Feb 16, 2020 • edited Loading

rohany commented Feb 16, 2020

abhishek20123g commented Feb 16, 2020

rohany Feb 16, 2020

Choose a reason for hiding this comment

rohany commented Feb 16, 2020

abhishek20123g commented Feb 16, 2020

rohany commented Feb 16, 2020

otan Feb 16, 2020

Choose a reason for hiding this comment

abhishek20123g Feb 17, 2020

Choose a reason for hiding this comment

otan Feb 17, 2020

Choose a reason for hiding this comment

abhishek20123g Feb 17, 2020

Choose a reason for hiding this comment

abhishek20123g commented Feb 17, 2020

otan Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

otan Feb 17, 2020

Choose a reason for hiding this comment

abhishek20123g Feb 17, 2020

Choose a reason for hiding this comment

ajwerner commented Feb 17, 2020

otan left a comment

Choose a reason for hiding this comment

otan commented Feb 18, 2020

craig bot commented Feb 18, 2020

Build succeeded

abhishek20123g commented Feb 18, 2020

rohany commented Feb 18, 2020

abhishek20123g commented Feb 15, 2020 •

edited by rohany

Loading

claassistantio commented Feb 15, 2020 •

edited

Loading

abhishek20123g commented Feb 16, 2020 •

edited

Loading

otan Feb 17, 2020 •

edited

Loading