sql: add voting_replicas, non_voting_replicas columns to SHOW RANGES by ecwall · Pull Request #93513 · cockroachdb/cockroach

ecwall · 2022-12-13T15:57:44Z

Some of the multitenant admin functions accept VOTERS, NONVOTERS as input.

Add voting_replicas, non_voting_replicas columns to SHOW RANGE(S) to make working with the admin functions easier.

Release note (sql change): Add voting_replicas, non_voting_replicas columns to output of SHOW RANGE(S) statements.

cockroach-teamcity · 2022-12-13T15:57:57Z

This change is

arulajmani

Assuming the powers who decide what columns are added to SHOW RANGES are happy with this, the change itself looks sane.

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, @knz, and @rafiss)

pkg/sql/show_ranges_test.go line 148 at r1 (raw file):

}

func TestShowRangesColumns(t *testing.T) {

Could we write this using a LogicTest instead?

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @knz, and @rafiss)

pkg/sql/show_ranges_test.go line 148 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Could we write this using a LogicTest instead?

Done.

arulajmani

Reviewed 1 of 2 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, @knz, and @rafiss)

pkg/sql/logictest/logic.go line 3410 at r5 (raw file):

				for i, v := range vals {
					colT := query.colTypes[i]
					// Ignore column - useful for non-deterministic output

Do we need this? Could we not instead construct the query such that the column isn't included?

pkg/sql/sqltestutils/sql_test_utils.go line 129 at r5 (raw file):

}

func ArrayStringToSlice(t *testing.T, array string, message ...string) []string {

nit: comment

pkg/sql/logictest/testdata/logic_test/ranges line 155 at r5 (raw file):

ALTER INDEX d.c@c_i_idx SPLIT AT VALUES (0)

# hex encode start_key, end_key so that text is UTF-8 compatible

What's the motivation behind this?

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @knz, and @rafiss)

pkg/sql/logictest/logic.go line 3410 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Do we need this? Could we not instead construct the query such that the column isn't included?

I think it is useful to have the tests I added in this PR fail if columns are added/removed to indicate that the existing test case should be updated instead of adding something new.

pkg/sql/sqltestutils/sql_test_utils.go line 129 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: comment

Added.

Code quote:

ArrayStringToSlice

pkg/sql/logictest/testdata/logic_test/ranges line 155 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

What's the motivation behind this?

git (including GitHub) can't diff this file because it has non-UTF-8 characters in it (these don't render in reviewable, but you can see them in GoLand). The problem still exists in this PR because the "before" file still contains these characters, but it will be fixed going forward.

diff --git a/pkg/sql/logictest/testdata/logic_test/ranges b/pkg/sql/logictest/testdata/logic_test/ranges
index b1b1342c71..9b4fb03bbd 100644
Binary files a/pkg/sql/logictest/testdata/logic_test/ranges and b/pkg/sql/logictest/testdata/logic_test/ranges differ

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @knz, and @rafiss)

pkg/sql/logictest/testdata/logic_test/ranges line 155 at r5 (raw file):

Previously, ecwall (Evan Wall) wrote…

git (including GitHub) can't diff this file because it has non-UTF-8 characters in it (these don't render in reviewable, but you can see them in GoLand). The problem still exists in this PR because the "before" file still contains these characters, but it will be fixed going forward.
diff --git a/pkg/sql/logictest/testdata/logic_test/ranges b/pkg/sql/logictest/testdata/logic_test/ranges
index b1b1342c71..9b4fb03bbd 100644
Binary files a/pkg/sql/logictest/testdata/logic_test/ranges and b/pkg/sql/logictest/testdata/logic_test/ranges differ

arulajmani

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, @knz, and @rafiss)

pkg/sql/logictest/logic.go line 3410 at r5 (raw file):

Previously, ecwall (Evan Wall) wrote…

I think it is useful to have the tests I added in this PR fail if columns are added/removed to indicate that the existing test case should be updated instead of adding something new.

Even then, if all you want to do is verify column names, couldn't you achieve the same thing by doing something like:

SELECT * FROM [SHOW RANGES FROM TABLE tbl;] LIMIT 0;

pkg/sql/logictest/testdata/logic_test/ranges line 155 at r5 (raw file):

Previously, ecwall (Evan Wall) wrote…

I see, thanks for making the change! Consider saying the same in the comment, given it's not completely obvious why we're doing this.

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @knz, and @rafiss)

pkg/sql/logictest/logic.go line 3410 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Even then, if all you want to do is verify column names, couldn't you achieve the same thing by doing something like:
SELECT * FROM [SHOW RANGES FROM TABLE tbl;] LIMIT 0;

I thought it would be useful to have a row also to make sure it is populated.

For some reason SHOW RANGES FROM DATABASE does not show any rows even if a table is added to it so I left it as is. There is a comment above that it is populated async so I don't think it should be verified here.

pkg/sql/logictest/testdata/logic_test/ranges line 155 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I see, thanks for making the change! Consider saying the same in the comment, given it's not completely obvious why we're doing this.

Good idea, added.

arulajmani

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, @knz, and @rafiss)

pkg/sql/logictest/logic.go line 3410 at r5 (raw file):

For some reason SHOW RANGES FROM DATABASE does not show any rows even if a table is added to it so I left it as is.

Isn't that because there's no table inside the database?

There is a comment above that it is populated async so I don't think it should be verified here.

Yeah, given this, I'm not sure it's worth adding this new "_" symbol to the logic test grammar. It doesn't seem generally valuable, given one could just decide to omit columns they're not interested in fetching.

For our particular test case, all we're deterministically testing is the column names, which we can do using a LIMIT 0. If all we care about is this command returns rows, then we should just do a count(*).

If you feel strongly about adding this '_' symbol however, I won't stand in your way. Just add a comment about it up top in logic.go where the other symbols are described.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @rafiss)

pkg/sql/logictest/logic.go line 3410 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

For some reason SHOW RANGES FROM DATABASE does not show any rows even if a table is added to it so I left it as is.

Isn't that because there's no table inside the database?

There is a comment above that it is populated async so I don't think it should be verified here.

Yeah, given this, I'm not sure it's worth adding this new "_" symbol to the logic test grammar. It doesn't seem generally valuable, given one could just decide to omit columns they're not interested in fetching.

For our particular test case, all we're deterministically testing is the column names, which we can do using a LIMIT 0. If all we care about is this command returns rows, then we should just do a count(*).

If you feel strongly about adding this '_' symbol however, I won't stand in your way. Just add a comment about it up top in logic.go where the other symbols are described.

SHOW RANGES FROM DATABASE is broken currently. I'm working on it.

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @rafiss)

pkg/sql/logictest/logic.go line 3410 at r5 (raw file):

Isn't that because there's no table inside the database?

Even after I added a table it did not have any records until I added a split to the table. SHOW RANGES FROM TABLE shows a record for a table even without a split.

If you feel strongly about adding this '_' symbol however, I won't stand in your way. Just add a comment about it up top in logic.go where the other symbols are described.

Comment added.

Fixes #93508 Some of the multitenant admin functions accept VOTERS, NONVOTERS as input. Add voting_replicas, non_voting_replicas columns to SHOW RANGE(S) to make working with the admin functions easier. Release note (sql change): Add voting_replicas, non_voting_replicas columns to output of SHOW RANGE(S) statements.

craig · 2022-12-15T22:21:54Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-12-16T01:32:17Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-12-16T01:47:52Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

ecwall · 2022-12-16T03:37:29Z

bors r=arulajmani

craig · 2022-12-16T03:37:30Z

Already running a review

craig · 2022-12-16T04:51:36Z

Build succeeded:

Bazel Essential CI (Cockroach)

ecwall requested review from ajwerner, arulajmani, knz and rafiss December 13, 2022 15:57

arulajmani reviewed Dec 13, 2022

View reviewed changes

ecwall commented Dec 13, 2022

View reviewed changes

arulajmani reviewed Dec 14, 2022

View reviewed changes

ecwall commented Dec 14, 2022

View reviewed changes

arulajmani reviewed Dec 14, 2022

View reviewed changes

ecwall commented Dec 14, 2022

View reviewed changes

arulajmani approved these changes Dec 15, 2022

View reviewed changes

knz reviewed Dec 15, 2022

View reviewed changes

ecwall commented Dec 15, 2022

View reviewed changes

craig bot merged commit a72ea76 into cockroachdb:master Dec 16, 2022

cockroach-teamcity mentioned this pull request Dec 16, 2022

PR #93513 - sql: add voting_replicas, non_voting_replicas columns to SHOW RANGES cockroachdb/docs#15845

Closed

mgartner mentioned this pull request Jan 30, 2023

pkg/sql/logictest/tests/5node/5node_test: TestLogic_ranges failed #96136

Closed

Conversation

ecwall commented Dec 13, 2022

Uh oh!

cockroach-teamcity commented Dec 13, 2022

Uh oh!

arulajmani left a comment

Choose a reason for hiding this comment

Uh oh!

ecwall left a comment

Choose a reason for hiding this comment

Uh oh!

arulajmani left a comment

Choose a reason for hiding this comment

Uh oh!

ecwall left a comment

Choose a reason for hiding this comment

Uh oh!

ecwall left a comment

Choose a reason for hiding this comment

Uh oh!

arulajmani left a comment

Choose a reason for hiding this comment

Uh oh!

ecwall left a comment

Choose a reason for hiding this comment

Uh oh!

arulajmani left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

ecwall left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Dec 15, 2022

Uh oh!

craig bot commented Dec 16, 2022

Uh oh!

craig bot commented Dec 16, 2022

Uh oh!

ecwall commented Dec 16, 2022

Uh oh!

craig bot commented Dec 16, 2022

Uh oh!

craig bot commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants