Skip to content

Make CLI automatically switch from aligned to vertical format#12208

Merged
kokosing merged 1 commit intotrinodb:masterfrom
nineinchnick:cli-format-auto
Feb 20, 2023
Merged

Make CLI automatically switch from aligned to vertical format#12208
kokosing merged 1 commit intotrinodb:masterfrom
nineinchnick:cli-format-auto

Conversation

@nineinchnick
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick commented May 2, 2022

Description

Make CLI automatically switch from aligned to vertical format when the output wouldn't fit the current terminal's width. Currently, users can terminate the query with \G, but this has a few drawbacks:

  • users need to run the query twice if they notice it's too wide
  • a table that's too wide will be opened in the pager and won't remain on the screen when writing a follow-up query - users might want to refer to the values from the first query, so they'd need to interrupt writing the second one
  • users have to notice the output is too wide - check out the output of SHOW FUNCTIONS where the second column is so wide you might not notice there are more
  • it's kinda hard to type - backslash is unusual and G has to be upper-case

This PR introduces a new AUTO output format that automatically switches from ALIGNED to VERTICAL if the aligned table is wider than the current terminal. But it doesn't make the auto format the default, for backward compatibility. Users need to enable it by using --output-format-interactive=AUTO or by adding it to the config file.

Is this change a fix, improvement, new feature, refactoring, or other?

improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

cli

How would you describe this change to a non-technical end user or system administrator?

Add a new AUTO output format to the CLI client, that switches from ALIGNED to VERTICAL if the output wouldn't fit the current terminal.

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# CLI
* Add a new `AUTO` output format, that switches from `ALIGNED` to `VERTICAL` if the output wouldn't fit the current terminal.  ({issue}`issuenumber`)

@nineinchnick
Copy link
Copy Markdown
Member Author

This is the equivalent of \x auto in psql. Setting the env var is the equivalent of adding \x auto to ~/.psqlrc.

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature and implementation seems good to me.

The env var seems overkill to me. What use-case did you have in mind?

@nineinchnick
Copy link
Copy Markdown
Member Author

The feature and implementation seems good to me.

The env var seems overkill to me. What use-case did you have in mind?

It's required so that users (well, I mostly ;-) can have it turned on by default, without changing the current default output (aligned). There's no RC file like in PostgreSQL. Without this, it defeats the purpose of it being automatic if I'd have to remember to turn it on on every invocation.

@hashhar
Copy link
Copy Markdown
Member

hashhar commented May 10, 2022

Let's discuss about having a config file then, it seems like a good change IMO since it'll make it much easier to support things like not having to enter lengthy arguments for Kerberos or self-signed tls auth etc. as well instead of just being specific to this single feature.

cc: @electrum @findepi WDYT about a config file to read some defaults for configs from? Would make using the CLI a lot easier.

@nineinchnick
Copy link
Copy Markdown
Member Author

@hashhar would it be enough to read a properties file from the user's home dir? https://picocli.info/#_propertiesdefaultprovider

@nineinchnick nineinchnick force-pushed the cli-format-auto branch 3 times, most recently from ceff83a to e457c2a Compare May 11, 2022 10:06
@nineinchnick nineinchnick requested review from electrum, findepi, hashhar and mosabua and removed request for electrum and mosabua May 11, 2022 10:06
@nineinchnick
Copy link
Copy Markdown
Member Author

All done, I added reading a config file as the first commit. I also added two new options for the pager and history file, so they also can be configured via the file, not only as environmental variables, for consistency.

The second commit adds the new AUTO format but also allows to configure the output format in the interactive mode. @mosabua I requested a review from you because I made doc changes but let's first wait to get the code changes approved.

@nineinchnick nineinchnick force-pushed the cli-format-auto branch 2 times, most recently from 837dab7 to 3c43e40 Compare May 12, 2022 10:03
@nineinchnick
Copy link
Copy Markdown
Member Author

@hashhar I removed the new env var for output format but changing the default in the config file means it'll apply to both interactive and batch modes. I'm thinking about adding yet another flag for interactive mode, but --output-format-interactive is a bit too long. Maybe a boolean --auto-expand to be used instead of adding a new output format?

@nineinchnick nineinchnick force-pushed the cli-format-auto branch 3 times, most recently from 3d31785 to 474914f Compare May 17, 2022 08:26
@nineinchnick
Copy link
Copy Markdown
Member Author

nineinchnick commented May 22, 2022

@hashhar @electrum @findepi this is ready for review.

@nineinchnick nineinchnick requested a review from mosabua May 30, 2022 09:36
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs look good now.

@nineinchnick
Copy link
Copy Markdown
Member Author

@dain maybe you'd like to review this one?

@mosabua
Copy link
Copy Markdown
Member

mosabua commented Sep 7, 2022

Could we get this reviewed by whoever necessary and merged?

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Sep 22, 2022

@electrum can you PTAL?

@nineinchnick
Copy link
Copy Markdown
Member Author

@electrum @dain PTAL. Are features like this worth adding to the CLI? If not, I don't mind closing this PR.

@mosabua
Copy link
Copy Markdown
Member

mosabua commented Oct 26, 2022

I think this is a good feature to add.

@nineinchnick
Copy link
Copy Markdown
Member Author

@kokosing WDYT?

Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change. I think it is also safe to merge as it does not change any existing functionality, but adds new one.

@nineinchnick nineinchnick force-pushed the cli-format-auto branch 2 times, most recently from 2395382 to 69ee34e Compare February 16, 2023 13:13
@nineinchnick
Copy link
Copy Markdown
Member Author

@kokosing this is ready to go, PTAL.

@aakashnand
Copy link
Copy Markdown
Member

@nineinchnick I also think this is useful. I have faced this problem where there are too many columns, until now I have been using \G but this flag now looks promising. Good Job 👍🏼

@kokosing kokosing merged commit 59e27e7 into trinodb:master Feb 20, 2023
@kokosing
Copy link
Copy Markdown
Member

Thanks

@nineinchnick nineinchnick deleted the cli-format-auto branch February 20, 2023 12:26
@github-actions github-actions bot added this to the 408 milestone Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

6 participants