sql: make SPLIT AT output be consistent with SHOW_RANGES#55543
sql: make SPLIT AT output be consistent with SHOW_RANGES#55543craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
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 was unable to automatically find a reviewer. You can try CCing one of the following members:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
|
This change is |
neeral
left a comment
There was a problem hiding this comment.
Manual testing:
Before:
root@:26257/defaultdb> alter table t split at values (2);
key | pretty | split_enforced_until
---------------+---------------+-----------------------------------
\274\211\212 | /Table/52/1/2 | 2262-04-11 23:47:16.854776+00:00
(1 row)
After:
root@:26257/defaultdb> alter table t split at values (2);
key | pretty | split_enforced_until
---------------+--------+-----------------------------------
\274\211\212 | /2 | 2262-04-11 23:47:16.854776+00:00
(1 row)
cc: @RaduBerinde could you please review or suggest someone to review this
Reviewable status:
complete! 0 of 0 LGTMs obtained
|
cc: @RaduBerinde @ajwerner for review (or suggest someone to review) |
ajwerner
left a comment
There was a problem hiding this comment.
It would be great to add a logictest, probably in pkg/sql/logictest/testdata/alter_table. Add a test and
On the whole I'm good with this once it gets a test. I wonder if ultimately it'd be better if we had the appropriate tuple of values but I suppose that's neither here nor there and cockroach's ability to deal with tuple types is currently limited.
Reviewable status:
b36f1eb to
5141e11
Compare
|
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. |
5141e11 to
3f7b1d9
Compare
neeral
left a comment
There was a problem hiding this comment.
Thank you for taking a look @ajwerner. I have added a logic test for this.
PTAL
Reviewable status:
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/alter_table, line 1421 at r1 (raw file):
Quoted 21 lines of code…
# Test formatting of keys in output of SPLIT AT query TTTI colnames,rowsort SELECT start_key, end_key, replicas, lease_holder FROM [SHOW RANGES FROM TABLE t] ---- start_key end_key replicas lease_holder NULL NULL {1} 1 query TTT colnames ALTER TABLE t SPLIT AT VALUES (1), (10) ---- key pretty split_enforced_until [213 137 137] /1 2262-04-11 23:47:16.854776 +0000 +0000 [213 137 146] /10 2262-04-11 23:47:16.854776 +0000 +0000 query TTTI colnames,rowsort SELECT start_key, end_key, replicas, lease_holder FROM [SHOW RANGES FROM TABLE t] ---- start_key end_key replicas lease_holder NULL /1 {1} 1 /1 /10 {1} 1 /10 NULL {1} 1 statement ok DROP TABLE t
Bad news, we don't support SHOW RANGES in all of our test configs, namely not in 3node-tenant. Can you create a new logictest file (split_at?) for this and put # LogicTest: !3node-tenant at the top. It should look just like
The output of the SPLIT AT command returns keys in the form `/Table/52/1/1` whereas the output of SHOW RANGES omits the prefix, of table id and index id, and displays keys as `/1`. This is a little confusing and making them consistent would make it easier to visually compare. This patch strips the prefix from the keys in the output of SPLIT AT. Release note (sql change): pretty column of SPLIT AT output was changed by stripping prefix. This makes it consistent with output from SHOW RANGES.
3f7b1d9 to
3ea77b0
Compare
neeral
left a comment
There was a problem hiding this comment.
PTAL
Reviewable status:
pkg/sql/logictest/testdata/logic_test/alter_table, line 1421 at r1 (raw file):
Previously, ajwerner wrote…
# Test formatting of keys in output of SPLIT AT query TTTI colnames,rowsort SELECT start_key, end_key, replicas, lease_holder FROM [SHOW RANGES FROM TABLE t] ---- start_key end_key replicas lease_holder NULL NULL {1} 1 query TTT colnames ALTER TABLE t SPLIT AT VALUES (1), (10) ---- key pretty split_enforced_until [213 137 137] /1 2262-04-11 23:47:16.854776 +0000 +0000 [213 137 146] /10 2262-04-11 23:47:16.854776 +0000 +0000 query TTTI colnames,rowsort SELECT start_key, end_key, replicas, lease_holder FROM [SHOW RANGES FROM TABLE t] ---- start_key end_key replicas lease_holder NULL /1 {1} 1 /1 /10 {1} 1 /10 NULL {1} 1 statement ok DROP TABLE tBad news, we don't support
SHOW RANGESin all of our test configs, namely not in3node-tenant. Can you create a new logictest file (split_at?) for this and put# LogicTest: !3node-tenantat the top. It should look just like
Ah - strange it didn't fail when I ran make testlogic locally. I've created a new file split_at and reverted changes to alter_table
ajwerner
left a comment
There was a problem hiding this comment.
bors r=ajwerner
Reviewable status:
|
Build succeeded: |
This fixes #24740
The output of the SPLIT AT command returns keys in the form
/Table/52/1/1whereas the output of SHOW RANGES omits theprefix, of table id and index id, and displays keys as
/1.This is a little confusing and making them consistent would
make it easier to visually compare.
This patch strips the prefix from the keys in the output of
SPLIT AT.
Release note (sql change): pretty column of SPLIT AT output was changed
by stripping prefix. This makes it consistent with output from SHOW
RANGES.