Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: make SPLIT AT output be consistent with SHOW EXPERIMENTAL_RANGES #24740

Closed
rmloveland opened this issue Apr 12, 2018 · 7 comments · Fixed by #55543
Closed

sql: make SPLIT AT output be consistent with SHOW EXPERIMENTAL_RANGES #24740

rmloveland opened this issue Apr 12, 2018 · 7 comments · Fixed by #55543
Labels
A-sql-execution Relating to SQL execution. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.

Comments

@rmloveland
Copy link
Collaborator

This is a feature request.

On version 2.0.0, the pretty-printed key output format differs between SHOW TESTING_RANGES and SPLIT AT as shown below.

This is a bit confusing when you are trying to learn the key output format so you can understand what is happening under the hood when you execute these statements.

The feature request is for SPLIT AT to use the same "short form" as SHOW TESTING_RANGES to make it easier to visually compare the outputs.

(The table t below is taken from the SQL tests for the SHOW TESTING_RANGES PR.)


    > SELECT version();
    +-----------------------------------------------------------------------------------+
    |                                      version                                      |
    +-----------------------------------------------------------------------------------+
    | CockroachDB CCL v2.0.0 (x86_64-apple-darwin13, built 2018/04/03 20:52:33, go1.10) |
    +-----------------------------------------------------------------------------------+
    (1 row)

    > SHOW CREATE TABLE t;
    +-------+------------------------------------------------------------+
    | Table |                        CreateTable                         |
    +-------+------------------------------------------------------------+
    | t     | CREATE TABLE t (                                          +|
    |       |         k1 INT NOT NULL,                                  +|
    |       |         k2 INT NOT NULL,                                  +|
    |       |         v INT NULL,                                       +|
    |       |         w INT NULL,                                       +|
    |       |         CONSTRAINT "primary" PRIMARY KEY (k1 ASC, k2 ASC),+|
    |       |         INDEX idx (v ASC, w ASC),                         +|
    |       |         FAMILY "primary" (k1, k2, v, w)                   +|
    |       | )                                                          |
    +-------+------------------------------------------------------------+
    (1 row)

    > ALTER TABLE t SPLIT AT VALUES (1), (10);
    +----------+----------------+
    |   key    |     pretty     |
    +----------+----------------+
    | \xbc8989 | /Table/52/1/1  |
    | \xbc8992 | /Table/52/1/10 |
    +----------+----------------+
    (2 rows)

    root=> SHOW TESTING_RANGES FROM TABLE t;
    +-----------+---------+----------+----------+--------------+
    | Start Key | End Key | Range ID | Replicas | Lease Holder |
    +-----------+---------+----------+----------+--------------+
    |           | /1      |       33 | {1,3,4}  |            3 |
    | /1        | /10     |       34 | {1,3,5}  |            3 |
    | /10       |         |       35 | {1,3,5}  |            3 |
    +-----------+---------+----------+----------+--------------+
    (3 rows)
@rmloveland
Copy link
Collaborator Author

cc @RaduBerinde and @knz

@RaduBerinde
Copy link
Member

I don't see a problem with removing the /Table/<id>/<idx> prefix.
In addition, I am not sure if the key column is useful at all. If it is used internally, we could hide it by default.

@RaduBerinde RaduBerinde changed the title Pretty-printed key output differs between SHOW TESTING_RANGES and SPLIT AT sql: make SPLIT AT output be consistent with SHOW EXPERIMENTAL_RANGES Apr 12, 2018
@knz knz added S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-execution Relating to SQL execution. labels May 14, 2018
@knz knz added the E-easy Easy issue to tackle, requires little or no CockroachDB experience label May 14, 2018
@rasouli
Copy link

rasouli commented Jun 22, 2020

Greetings!

I am new here and I am very interested to start contributing by picking up a good first issue like this.
I was wondering if it is ok to start working on this and any clue you can give me to start hacking with.

Thanks 😃

@rohany
Copy link
Contributor

rohany commented Jun 25, 2020

Hi @rasouli, go ahead! I'd first look at how SHOW RANGES is implemented (pkg/sql/delegate/show_ranges.go), and then the code that drives ALTER TABLE SPLIT AT in pkg/sql/split.go.

@ajwerner
Copy link
Contributor

In order to make this change you'll need to change the set of columns returned from the Values() method on the splitNode in split.go. You'll also need to update the result columns from that node. The set of columns returned from the split node is:

var AlterTableSplitColumns = ResultColumns{

@neeral
Copy link
Contributor

neeral commented Oct 14, 2020

@rasouli are you working on this? If not, I'd like to pick it up.

@rasouli
Copy link

rasouli commented Oct 14, 2020

@rasouli are you working on this? If not, I'd like to pick it up.

Hi neeral,
please go ahead, unfortunately I have been very busy since then.
best of luck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-execution Relating to SQL execution. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants