-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 IEC standard abbreviations (GiB, TiB, etc) #6742
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated change looks good to me! Learned a bit new things about IEC standard also
core/dbt/utils.py
Outdated
@@ -454,7 +454,7 @@ def __get__(self, obj, objtype): | |||
|
|||
|
|||
def format_bytes(num_bytes): | |||
for unit in ["Bytes", "KB", "MB", "GB", "TB", "PB"]: | |||
for unit in ["Bytes", "KiB", "MiB", "GiB", "TiB", "PiB"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we actually use this method anywhere in dbt-core
. It's a holdover from when it was added, for the sole benefit of dbt-bigquery
, and we forgot to remove it during the Great Adapter Split-Apart of 2021.
What do you think about just removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about just removing it?
Removed!
I did a quick cursory check for both format_bytes
and format_rows_number
in the dbt Labs adapters using a GitHub search like this. DIdn't see any references other than in dbt-bigquery, which we know about.
I didn't check any non-dbt Labs adapters, although our expectation is that it is not being used. If it did happen to be used by some adapter out there, they'd be on their own to resolve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm aussi
Les remerciements @ChenyuLInx and @jtcohen6 Merging now. |
resolves #6741
equivalent to dbt-labs/dbt-bigquery#482
Description
When using multiples of 1024, the IEC standard abbreviations are "GiB", "TiB", "PiB" instead of "GB", "TB", "PB" (which are for multiples of 1000).
More info
Today I Learned (TIL),
format_bytes
(andformat_rows_number
) are defined in dbt-core but redefined in dbt-bigquery and only used in that adapter. So the dbt-core version is unused to best I can tell.Context for the duplication
Removing them from dbt-core was mentioned here and here as part of the discussion for #48.
It feels to me like one of the following should have happened along with dbt-labs/dbt-bigquery#48:
format_bytes
andformat_rows_number
from dbt-core; orAs it stands, the implementations in dbt-core are unused (to the best that we can tell), but would probably need to be maintained to preserve backwards compatibility in case some unknown adapter is using them.
Proposal
Going forward, I'd propose that we choose one (and only one) of the following:
Checklist
changie new
to create a changelog entry