You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/24 11:39:34 UTC
[GitHub] [arrow-rs] mbrobbel opened a new pull request, #3177: Move decimal constants from `arrow-data` to `arrow-schema` crate
mbrobbel opened a new pull request, #3177:
URL: https://github.com/apache/arrow-rs/pull/3177
# Rationale for this change
When building something with `arrow-schema` it's useful to have these constants without having to depend on `arrow-data`.
# What changes are included in this PR?
This change moves the following decimal related constants from the `arrow-data` to the `arrow-schema` crate:
- `MAX_DECIMAL_FOR_EACH_PRECISION`
- `MIN_DECIMAL_FOR_EACH_PRECISION`
- `DECIMAL128_MAX_PRECISION`
- `DECIMAL128_MAX_SCALE`
- `DECIMAL256_MAX_PRECISION`
- `DECIMAL256_MAX_SCALE`
- `DECIMAL_DEFAULT_SCALE`
# Are there any user-facing changes?
Yes, this is a breaking change. It requires updating `use` statements in dependent crates. We could `pub use` these constants from the `arrow-data` crate to prevent this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3177: Move decimal constants from `arrow-data` to `arrow-schema` crate
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3177:
URL: https://github.com/apache/arrow-rs/pull/3177#discussion_r1031520794
##########
arrow-schema/src/datatype.rs:
##########
@@ -412,6 +412,108 @@ impl DataType {
}
}
+/// `MAX_DECIMAL_FOR_EACH_PRECISION[p]` holds the maximum `i128` value that can
+/// be stored in [DataType::Decimal128] value of precision `p`
+pub const MAX_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [
Review Comment:
I'm not sure about moving these, as the decimal256 versions are in terms of i256 which is defined in arrow-buffer. Perhaps we could just move the schema constants?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3177: Move decimal constants from `arrow-data` to `arrow-schema` crate
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3177:
URL: https://github.com/apache/arrow-rs/pull/3177#discussion_r1031538122
##########
arrow/src/row/mod.rs:
##########
@@ -1004,6 +1004,7 @@ unsafe fn decode_column(
mod tests {
use std::sync::Arc;
+ use arrow_schema::{DECIMAL128_MAX_PRECISION, DECIMAL256_MAX_PRECISION};
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #3177: Move decimal constants from `arrow-data` to `arrow-schema` crate
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #3177:
URL: https://github.com/apache/arrow-rs/pull/3177#issuecomment-1326757055
Benchmark runs are scheduled for baseline = 3e2d39ed89ba786dcb88ddbb2a73253cdf680903 and contender = 22deab00696f4fac2a587c59bf9b0ce2c2ec3ac6. 22deab00696f4fac2a587c59bf9b0ce2c2ec3ac6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/76e5dce60fb64884a8735bc1d25ab760...367e63519cbd4060bfea6682a3e12a07/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/35f1214d14e74fd787f92003cbd6c318...78e53119223248fe9166153b0c4b34cf/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/607e2629e03548e0b83d40eb989d2ba1...e4201d3221ab41b99577db11d60f3bd0/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/002e7365daa44daf935a4030f2b21e0e...4f29b08d93fd47da8365252de819ed25/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3177: Move decimal constants from `arrow-data` to `arrow-schema` crate
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3177:
URL: https://github.com/apache/arrow-rs/pull/3177#discussion_r1031522227
##########
arrow-data/src/decimal.rs:
##########
@@ -638,108 +641,6 @@ pub(crate) const MIN_DECIMAL_BYTES_FOR_LARGER_EACH_PRECISION: [i256; 76] = [
]),
];
-/// `MAX_DECIMAL_FOR_EACH_PRECISION[p]` holds the maximum `i128` value
-/// that can be stored in [arrow_schema::DataType::Decimal128] value of precision `p`
-pub const MAX_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [
- 9,
- 99,
- 999,
- 9999,
- 99999,
- 999999,
- 9999999,
- 99999999,
- 999999999,
- 9999999999,
- 99999999999,
- 999999999999,
- 9999999999999,
- 99999999999999,
- 999999999999999,
- 9999999999999999,
- 99999999999999999,
- 999999999999999999,
- 9999999999999999999,
- 99999999999999999999,
- 999999999999999999999,
- 9999999999999999999999,
- 99999999999999999999999,
- 999999999999999999999999,
- 9999999999999999999999999,
- 99999999999999999999999999,
- 999999999999999999999999999,
- 9999999999999999999999999999,
- 99999999999999999999999999999,
- 999999999999999999999999999999,
- 9999999999999999999999999999999,
- 99999999999999999999999999999999,
- 999999999999999999999999999999999,
- 9999999999999999999999999999999999,
- 99999999999999999999999999999999999,
- 999999999999999999999999999999999999,
- 9999999999999999999999999999999999999,
- 99999999999999999999999999999999999999,
-];
-
-/// `MIN_DECIMAL_FOR_EACH_PRECISION[p]` holds the minimum `i128` value
-/// that can be stored in a [arrow_schema::DataType::Decimal128] value of precision `p`
-pub const MIN_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [
- -9,
- -99,
- -999,
- -9999,
- -99999,
- -999999,
- -9999999,
- -99999999,
- -999999999,
- -9999999999,
- -99999999999,
- -999999999999,
- -9999999999999,
- -99999999999999,
- -999999999999999,
- -9999999999999999,
- -99999999999999999,
- -999999999999999999,
- -9999999999999999999,
- -99999999999999999999,
- -999999999999999999999,
- -9999999999999999999999,
- -99999999999999999999999,
- -999999999999999999999999,
- -9999999999999999999999999,
- -99999999999999999999999999,
- -999999999999999999999999999,
- -9999999999999999999999999999,
- -99999999999999999999999999999,
- -999999999999999999999999999999,
- -9999999999999999999999999999999,
- -99999999999999999999999999999999,
- -999999999999999999999999999999999,
- -9999999999999999999999999999999999,
- -99999999999999999999999999999999999,
- -999999999999999999999999999999999999,
- -9999999999999999999999999999999999999,
- -99999999999999999999999999999999999999,
-];
-
-/// The maximum precision for [arrow_schema::DataType::Decimal128] values
-pub const DECIMAL128_MAX_PRECISION: u8 = 38;
Review Comment:
Could we add `pub use` to avoid this being a breaking change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #3177: Move decimal constants from `arrow-data` to `arrow-schema` crate
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #3177:
URL: https://github.com/apache/arrow-rs/pull/3177
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3177: Move decimal constants from `arrow-data` to `arrow-schema` crate
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3177:
URL: https://github.com/apache/arrow-rs/pull/3177#discussion_r1031520794
##########
arrow-schema/src/datatype.rs:
##########
@@ -412,6 +412,108 @@ impl DataType {
}
}
+/// `MAX_DECIMAL_FOR_EACH_PRECISION[p]` holds the maximum `i128` value that can
+/// be stored in [DataType::Decimal128] value of precision `p`
+pub const MAX_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [
Review Comment:
I'm not sure about moving these, as the decimal256 versions are in terms of i256 which is defined in arrow-buffer, and so they are left behind. Perhaps we could just move the schema constants?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org