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

Use correct abbreviations for data quantities #13095

Closed
wants to merge 0 commits into from

Conversation

metadaddy
Copy link
Member

Description

The Trino CLI currently reports data size incorrectly, using the legacy, 1,024-based, values for K, M, G etc rather than the modern 1,000-based values.

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

Fix

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?

This change corrects the display of data sizes in the CLI to use 1,000-based units.

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) 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.
( ) Release notes entries required with the following suggested text:

@cla-bot
Copy link

cla-bot bot commented Jul 6, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

@dain
Copy link
Member

dain commented Jul 6, 2022

If you search the issue and PR history this has been discussed several time before. At this point we can not change this behavior.

@dain dain closed this Jul 6, 2022
@metadaddy
Copy link
Member Author

Hi @dain - I spent a good 30 mins looking, but I couldn't see any previous issues or PRs on this topic. In any case, it's your project.

Is there any value in the tests I wrote for the CLI FormatUtils class? I'd be happy to modify the relevant test to fit the current behavior and submit them in a separate PR.

@dain
Copy link
Member

dain commented Jul 6, 2022

It seems that @martint may be ok with this for the CLI

@dain dain reopened this Jul 6, 2022
@metadaddy
Copy link
Member Author

Cool - I'll do the CLA thing.

@findepi
Copy link
Member

findepi commented Jul 19, 2022

Fixing the CLI is correct in isolation but will introduce inconsistency with Trino built-in function parse_data_size (https://trino.io/docs/current/functions/conversion.html#parse_data_size) which we're not planning on changing (per #11160 (review)).

Wonder how we proceed here.
cc @nineinchnick @electrum

@electrum
Copy link
Member

I think it would be better to keep the base-2 units and simply add the i to the unit designation. This preserves the existing behavior and compatibility with base-2 units used everywhere else.

@metadaddy
Copy link
Member Author

Oops - I submitted this PR from my master branch, then I needed to sync back for another change. I'll submit another PR with a different branch and reference this one.

@metadaddy
Copy link
Member Author

Replaced by #14859

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.

Trino CLI reports data with wrong units
4 participants