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/08/09 05:29:35 UTC
[GitHub] [arrow-rs] HaoYang670 opened a new pull request, #2383: Rewrite `Decimal` and `DecimalArray` `using const_generic`
HaoYang670 opened a new pull request, #2383:
URL: https://github.com/apache/arrow-rs/pull/2383
Signed-off-by: remzi <13...@gmail.com>
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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] liukun4515 commented on pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1209059510
> I assume that this PR will not introduce performance regression. But we need more test because weird things often happen.
Thanks for your concern about performance, that is why I submit these two pr about decimal optimization. https://github.com/apache/arrow-rs/pull/2360 https://github.com/apache/arrow-rs/pull/2357
In our service, there are many columns with decimal data type.
--
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 pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1209209172
So we don't actually have any decimal benchmarks that I can find...
--
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940960239
##########
arrow/src/util/decimal.rs:
##########
@@ -18,21 +18,51 @@
//! Decimal related utils
use crate::datatypes::{
- DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE, DECIMAL256_MAX_PRECISION,
- DECIMAL256_MAX_SCALE,
+ DataType, DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE, DECIMAL256_MAX_PRECISION,
+ DECIMAL256_MAX_SCALE, DECIMAL_DEFAULT_SCALE,
};
use crate::error::{ArrowError, Result};
use num::bigint::BigInt;
use num::Signed;
use std::cmp::{min, Ordering};
-pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
- /// The bit-width of the internal representation.
- const BIT_WIDTH: usize;
- /// The maximum precision.
- const MAX_PRECISION: usize;
- /// The maximum scale.
- const MAX_SCALE: usize;
+#[derive(Debug)]
+pub struct BasicDecimal<const BYTE_WIDTH: usize> {
+ precision: usize,
+ scale: usize,
+ value: [u8; BYTE_WIDTH],
+}
+
+impl<const BYTE_WIDTH: usize> BasicDecimal<BYTE_WIDTH> {
+ #[allow(clippy::type_complexity)]
+ const _MAX_PRECISION_SCALE_CONSTRUCTOR_DEFAULT_TYPE: (
Review Comment:
As this is not pub, I think `MAX_PRECISION_SCALE_CONSTRUCTOR_DEFAULT_TYPE` might be okay. A `_` prefix seems redundant.
--
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 #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1209215318
Benchmark runs are scheduled for baseline = 56f7904ef89ea02b49e6b346d2456d79085f7502 and contender = 77c814cfc66845192c1e0be6ae8b0de6a1a0d5f0. 77c814cfc66845192c1e0be6ae8b0de6a1a0d5f0 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/3ff93b59cbd24b97a3f5cb66b7c95a6f...9c65a5d00dd6496b8b4df9c0d9fbd547/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/16e22b13f7404ded844e76c8c9a34752...4b449743e99045c6b893612219e75719/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/8e0dd37322cd49d7bd656ed7564b1277...7009f274fe724b4a84077bf4005bcfff/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a116c29fbb5b445db5519ef1d43fd3d6...8a3f6b81c66d4ccb9f31734577614fd6/)
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940953395
##########
arrow/src/array/array_decimal.rs:
##########
@@ -71,44 +71,47 @@ use crate::util::decimal::{BasicDecimal, Decimal128, Decimal256};
/// assert_eq!(6, decimal_array.scale());
/// ```
///
-pub struct Decimal128Array {
- data: ArrayData,
- value_data: RawPtrBox<u8>,
- precision: usize,
- scale: usize,
-}
+pub type Decimal128Array = BasicDecimalArray<16>;
-pub struct Decimal256Array {
- data: ArrayData,
- value_data: RawPtrBox<u8>,
- precision: usize,
- scale: usize,
-}
+pub type Decimal256Array = BasicDecimalArray<32>;
mod private_decimal {
pub trait DecimalArrayPrivate {
fn raw_value_data_ptr(&self) -> *const u8;
}
}
Review Comment:
Do we still need this trait?
--
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] alamb commented on pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1209273679
It wasn't 100% clear to me -- but @liukun4515 this PR may have improved decimal validation performance. Perhaps you can run your benchmarks again now to see if things have improved.
--
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940952692
##########
arrow/src/array/array_decimal.rs:
##########
@@ -287,116 +325,86 @@ pub trait BasicDecimalArray<T: BasicDecimal, U: From<ArrayData>>:
// decreased
self.validate_decimal_precision(precision)?;
- let data_type = if Self::VALUE_LENGTH == 16 {
- DataType::Decimal128(self.precision(), self.scale())
- } else {
- DataType::Decimal256(self.precision(), self.scale())
- };
+ let data_type = Self::TYPE_CONSTRUCTOR(self.precision, self.scale);
assert_eq!(self.data().data_type(), &data_type);
// safety: self.data is valid DataType::Decimal as checked above
- let new_data_type = if Self::VALUE_LENGTH == 16 {
- DataType::Decimal128(precision, scale)
- } else {
- DataType::Decimal256(precision, scale)
- };
+ let new_data_type = Self::TYPE_CONSTRUCTOR(precision, scale);
Ok(self.data().clone().with_data_type(new_data_type).into())
}
+}
+impl Decimal256Array {
/// Validates decimal values in this array can be properly interpreted
/// with the specified precision.
- fn validate_decimal_precision(&self, precision: usize) -> Result<()>;
-}
-
-impl BasicDecimalArray<Decimal128, Decimal128Array> for Decimal128Array {
- const VALUE_LENGTH: i32 = 16;
- const DEFAULT_TYPE: DataType =
- DataType::Decimal128(DECIMAL128_MAX_PRECISION, DECIMAL_DEFAULT_SCALE);
- const MAX_PRECISION: usize = DECIMAL128_MAX_PRECISION;
- const MAX_SCALE: usize = DECIMAL128_MAX_SCALE;
-
- fn data(&self) -> &ArrayData {
- &self.data
- }
-
- fn precision(&self) -> usize {
- self.precision
- }
-
- fn scale(&self) -> usize {
- self.scale
- }
-
- fn validate_decimal_precision(&self, precision: usize) -> Result<()> {
+ pub fn validate_decimal_precision(&self, precision: usize) -> Result<()> {
if precision < self.precision {
for v in self.iter().flatten() {
- validate_decimal_precision(v.as_i128(), precision)?;
+ validate_decimal256_precision(&v.to_string(), precision)?;
}
}
Ok(())
}
-}
-
-impl BasicDecimalArray<Decimal256, Decimal256Array> for Decimal256Array {
- const VALUE_LENGTH: i32 = 32;
- const DEFAULT_TYPE: DataType =
- DataType::Decimal256(DECIMAL256_MAX_PRECISION, DECIMAL_DEFAULT_SCALE);
- const MAX_PRECISION: usize = DECIMAL256_MAX_PRECISION;
- const MAX_SCALE: usize = DECIMAL256_MAX_SCALE;
- fn data(&self) -> &ArrayData {
- &self.data
- }
+ /// Returns a Decimal array with the same data as self, with the
+ /// specified precision.
+ ///
+ /// Returns an Error if:
+ /// 1. `precision` is larger than [`Self::MAX_PRECISION`]
+ /// 2. `scale` is larger than [`Self::MAX_SCALE`];
+ /// 3. `scale` is > `precision`
+ pub fn with_precision_and_scale(self, precision: usize, scale: usize) -> Result<Self>
+ where
+ Self: Sized,
+ {
+ if precision > Self::MAX_PRECISION {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "precision {} is greater than max {}",
+ precision,
+ Self::MAX_PRECISION
+ )));
+ }
+ if scale > Self::MAX_SCALE {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "scale {} is greater than max {}",
+ scale,
+ Self::MAX_SCALE
+ )));
+ }
+ if scale > precision {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "scale {} is greater than precision {}",
+ scale, precision
+ )));
+ }
- fn precision(&self) -> usize {
- self.precision
- }
+ // Ensure that all values are within the requested
+ // precision. For performance, only check if the precision is
+ // decreased
+ self.validate_decimal_precision(precision)?;
- fn scale(&self) -> usize {
- self.scale
- }
+ let data_type = Self::TYPE_CONSTRUCTOR(self.precision, self.scale);
+ assert_eq!(self.data().data_type(), &data_type);
- fn validate_decimal_precision(&self, precision: usize) -> Result<()> {
- if precision < self.precision {
- for v in self.iter().flatten() {
- validate_decimal256_precision(&v.to_string(), precision)?;
- }
- }
- Ok(())
- }
-}
+ // safety: self.data is valid DataType::Decimal as checked above
+ let new_data_type = Self::TYPE_CONSTRUCTOR(precision, scale);
-impl Decimal128Array {
- /// Creates a [Decimal128Array] with default precision and scale,
- /// based on an iterator of `i128` values without nulls
- pub fn from_iter_values<I: IntoIterator<Item = i128>>(iter: I) -> Self {
- let val_buf: Buffer = iter.into_iter().collect();
- let data = unsafe {
- ArrayData::new_unchecked(
- Self::default_type(),
- val_buf.len() / std::mem::size_of::<i128>(),
- None,
- None,
- 0,
- vec![val_buf],
- vec![],
- )
- };
- Decimal128Array::from(data)
+ Ok(self.data().clone().with_data_type(new_data_type).into())
}
}
-impl From<ArrayData> for Decimal128Array {
+impl<const BYTE_WIDTH: usize> From<ArrayData> for BasicDecimalArray<BYTE_WIDTH> {
fn from(data: ArrayData) -> Self {
assert_eq!(
data.buffers().len(),
1,
- "Decimal128Array data should contain 1 buffer only (values)"
+ "DecimalArray data should contain 1 buffer only (values)"
Review Comment:
We may show `Decimal128Array` or `Decimal256Array` based on byte width.
--
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 #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r941070788
##########
arrow/src/array/array_decimal.rs:
##########
@@ -242,22 +245,57 @@ pub trait BasicDecimalArray<T: BasicDecimal, U: From<ArrayData>>:
.offset(list_offset);
let array_data = unsafe { builder.build_unchecked() };
- U::from(array_data)
+ Self::from(array_data)
}
/// The default precision and scale used when not specified.
- fn default_type() -> DataType {
+ pub const fn default_type() -> DataType {
Self::DEFAULT_TYPE
}
+ fn raw_value_data_ptr(&self) -> *const u8 {
+ self.value_data.as_ptr()
+ }
+}
+
+impl Decimal128Array {
+ /// Creates a [Decimal128Array] with default precision and scale,
+ /// based on an iterator of `i128` values without nulls
+ pub fn from_iter_values<I: IntoIterator<Item = i128>>(iter: I) -> Self {
Review Comment:
For the record this method is unsound, but was unsound before. There is a broader issue here
--
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] viirya commented on pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1209604195
Opened a minor PR #2389 to clean up some comments not addressed yet.
--
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] HaoYang670 commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940967054
##########
arrow/src/util/decimal.rs:
##########
@@ -119,15 +165,44 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
}
}
+impl<const BYTE_WIDTH: usize> PartialOrd for BasicDecimal<BYTE_WIDTH> {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Review Comment:
you can see constant values as a kind of type (although they are not 100% same).
So different const values are different types.
--
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] HaoYang670 commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940960852
##########
arrow/src/array/array_decimal.rs:
##########
@@ -71,44 +71,47 @@ use crate::util::decimal::{BasicDecimal, Decimal128, Decimal256};
/// assert_eq!(6, decimal_array.scale());
/// ```
///
-pub struct Decimal128Array {
- data: ArrayData,
- value_data: RawPtrBox<u8>,
- precision: usize,
- scale: usize,
-}
+pub type Decimal128Array = BasicDecimalArray<16>;
-pub struct Decimal256Array {
- data: ArrayData,
- value_data: RawPtrBox<u8>,
- precision: usize,
- scale: usize,
-}
+pub type Decimal256Array = BasicDecimalArray<32>;
mod private_decimal {
pub trait DecimalArrayPrivate {
fn raw_value_data_ptr(&self) -> *const u8;
}
}
-pub trait BasicDecimalArray<T: BasicDecimal, U: From<ArrayData>>:
- private_decimal::DecimalArrayPrivate
-{
- const VALUE_LENGTH: i32;
- const DEFAULT_TYPE: DataType;
- const MAX_PRECISION: usize;
- const MAX_SCALE: usize;
+pub struct BasicDecimalArray<const BYTE_WIDTH: usize> {
Review Comment:
We could also seal the `byte_width` in this way:
https://users.rust-lang.org/t/how-to-seal-the-const-generic/77947/2
--
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940955661
##########
arrow/src/array/array_decimal.rs:
##########
@@ -71,44 +71,47 @@ use crate::util::decimal::{BasicDecimal, Decimal128, Decimal256};
/// assert_eq!(6, decimal_array.scale());
/// ```
///
-pub struct Decimal128Array {
- data: ArrayData,
- value_data: RawPtrBox<u8>,
- precision: usize,
- scale: usize,
-}
+pub type Decimal128Array = BasicDecimalArray<16>;
-pub struct Decimal256Array {
- data: ArrayData,
- value_data: RawPtrBox<u8>,
- precision: usize,
- scale: usize,
-}
+pub type Decimal256Array = BasicDecimalArray<32>;
mod private_decimal {
pub trait DecimalArrayPrivate {
fn raw_value_data_ptr(&self) -> *const u8;
}
}
-pub trait BasicDecimalArray<T: BasicDecimal, U: From<ArrayData>>:
- private_decimal::DecimalArrayPrivate
-{
- const VALUE_LENGTH: i32;
- const DEFAULT_TYPE: DataType;
- const MAX_PRECISION: usize;
- const MAX_SCALE: usize;
+pub struct BasicDecimalArray<const BYTE_WIDTH: usize> {
+ data: ArrayData,
+ value_data: RawPtrBox<u8>,
+ precision: usize,
+ scale: usize,
+}
+
+impl<const BYTE_WIDTH: usize> BasicDecimalArray<BYTE_WIDTH> {
+ pub const VALUE_LENGTH: i32 = BYTE_WIDTH as i32;
+ pub const DEFAULT_TYPE: DataType = BasicDecimal::<BYTE_WIDTH>::DEFAULT_TYPE;
+ pub const MAX_PRECISION: usize = BasicDecimal::<BYTE_WIDTH>::MAX_PRECISION;
+ pub const MAX_SCALE: usize = BasicDecimal::<BYTE_WIDTH>::MAX_SCALE;
+ pub const TYPE_CONSTRUCTOR: fn(usize, usize) -> DataType =
Review Comment:
Can TYPE_CONSTRUCTOR be non-public?
--
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940954693
##########
arrow/src/array/array_decimal.rs:
##########
@@ -71,44 +71,47 @@ use crate::util::decimal::{BasicDecimal, Decimal128, Decimal256};
/// assert_eq!(6, decimal_array.scale());
/// ```
///
-pub struct Decimal128Array {
- data: ArrayData,
- value_data: RawPtrBox<u8>,
- precision: usize,
- scale: usize,
-}
+pub type Decimal128Array = BasicDecimalArray<16>;
-pub struct Decimal256Array {
- data: ArrayData,
- value_data: RawPtrBox<u8>,
- precision: usize,
- scale: usize,
-}
+pub type Decimal256Array = BasicDecimalArray<32>;
mod private_decimal {
pub trait DecimalArrayPrivate {
fn raw_value_data_ptr(&self) -> *const u8;
}
}
-pub trait BasicDecimalArray<T: BasicDecimal, U: From<ArrayData>>:
- private_decimal::DecimalArrayPrivate
-{
- const VALUE_LENGTH: i32;
- const DEFAULT_TYPE: DataType;
- const MAX_PRECISION: usize;
- const MAX_SCALE: usize;
+pub struct BasicDecimalArray<const BYTE_WIDTH: usize> {
Review Comment:
We still have no idea how to constrain byte width, right?
--
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] HaoYang670 commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940957037
##########
arrow/src/util/decimal.rs:
##########
@@ -18,21 +18,51 @@
//! Decimal related utils
use crate::datatypes::{
- DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE, DECIMAL256_MAX_PRECISION,
- DECIMAL256_MAX_SCALE,
+ DataType, DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE, DECIMAL256_MAX_PRECISION,
+ DECIMAL256_MAX_SCALE, DECIMAL_DEFAULT_SCALE,
};
use crate::error::{ArrowError, Result};
use num::bigint::BigInt;
use num::Signed;
use std::cmp::{min, Ordering};
-pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
- /// The bit-width of the internal representation.
- const BIT_WIDTH: usize;
- /// The maximum precision.
- const MAX_PRECISION: usize;
- /// The maximum scale.
- const MAX_SCALE: usize;
+#[derive(Debug)]
+pub struct BasicDecimal<const BYTE_WIDTH: usize> {
+ precision: usize,
+ scale: usize,
+ value: [u8; BYTE_WIDTH],
+}
+
+impl<const BYTE_WIDTH: usize> BasicDecimal<BYTE_WIDTH> {
+ #[allow(clippy::type_complexity)]
+ const _MAX_PRECISION_SCALE_CONSTRUCTOR_DEFAULT_TYPE: (
+ usize,
+ usize,
+ fn(usize, usize) -> DataType,
+ DataType,
+ ) = match BYTE_WIDTH {
+ 16 => (
+ DECIMAL128_MAX_PRECISION,
+ DECIMAL128_MAX_SCALE,
+ DataType::Decimal128,
+ DataType::Decimal128(DECIMAL128_MAX_PRECISION, DECIMAL_DEFAULT_SCALE),
+ ),
+ 32 => (
+ DECIMAL256_MAX_PRECISION,
+ DECIMAL256_MAX_SCALE,
+ DataType::Decimal256,
+ DataType::Decimal256(DECIMAL256_MAX_PRECISION, DECIMAL_DEFAULT_SCALE),
+ ),
+ _ => panic!("invalid byte width"),
Review Comment:
We could constrain the byte width here. When compile the constant items, the compiler will give error if byte width != 16 or 32.
@viirya
--
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] HaoYang670 commented on pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1208964360
Seems like there are some unrelated errors in `cargo doc`
--
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940964181
##########
arrow/src/util/decimal.rs:
##########
@@ -119,15 +165,44 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
}
}
+impl<const BYTE_WIDTH: usize> PartialOrd for BasicDecimal<BYTE_WIDTH> {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Review Comment:
Okay, I see. Was not sure if `Self` includes the generic parameter.
--
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 #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r941002927
##########
arrow/src/util/decimal.rs:
##########
@@ -41,7 +71,11 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
/// Safety:
/// This method doesn't validate if the decimal value represented by the bytes
/// can be fitted into the specified precision.
- fn try_new_from_bytes(precision: usize, scale: usize, bytes: &[u8]) -> Result<Self>
+ pub fn try_new_from_bytes(
+ precision: usize,
+ scale: usize,
+ bytes: &[u8],
Review Comment:
I think this would help the compiler elude bounds checks, and so might be worth doing
##########
arrow/src/util/decimal.rs:
##########
@@ -41,7 +71,11 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
/// Safety:
/// This method doesn't validate if the decimal value represented by the bytes
/// can be fitted into the specified precision.
- fn try_new_from_bytes(precision: usize, scale: usize, bytes: &[u8]) -> Result<Self>
+ pub fn try_new_from_bytes(
+ precision: usize,
+ scale: usize,
+ bytes: &[u8],
Review Comment:
I think this would help the compiler elide bounds checks, and so might be worth doing
--
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] HaoYang670 commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r941121798
##########
arrow/src/util/decimal.rs:
##########
@@ -41,7 +71,11 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
/// Safety:
/// This method doesn't validate if the decimal value represented by the bytes
/// can be fitted into the specified precision.
- fn try_new_from_bytes(precision: usize, scale: usize, bytes: &[u8]) -> Result<Self>
+ pub fn try_new_from_bytes(
+ precision: usize,
+ scale: usize,
+ bytes: &[u8],
Review Comment:
Updated.
--
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 pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1209211972
I'm going to get this in as I think it is a valuable cleanup, and we can continue to iterate on performance in subsequent PRs
--
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940966794
##########
arrow/src/util/decimal.rs:
##########
@@ -18,21 +18,51 @@
//! Decimal related utils
use crate::datatypes::{
- DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE, DECIMAL256_MAX_PRECISION,
- DECIMAL256_MAX_SCALE,
+ DataType, DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE, DECIMAL256_MAX_PRECISION,
+ DECIMAL256_MAX_SCALE, DECIMAL_DEFAULT_SCALE,
};
use crate::error::{ArrowError, Result};
use num::bigint::BigInt;
use num::Signed;
use std::cmp::{min, Ordering};
-pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
- /// The bit-width of the internal representation.
- const BIT_WIDTH: usize;
- /// The maximum precision.
- const MAX_PRECISION: usize;
- /// The maximum scale.
- const MAX_SCALE: usize;
+#[derive(Debug)]
+pub struct BasicDecimal<const BYTE_WIDTH: usize> {
+ precision: usize,
+ scale: usize,
+ value: [u8; BYTE_WIDTH],
+}
+
+impl<const BYTE_WIDTH: usize> BasicDecimal<BYTE_WIDTH> {
+ #[allow(clippy::type_complexity)]
+ const _MAX_PRECISION_SCALE_CONSTRUCTOR_DEFAULT_TYPE: (
+ usize,
+ usize,
+ fn(usize, usize) -> DataType,
+ DataType,
+ ) = match BYTE_WIDTH {
+ 16 => (
+ DECIMAL128_MAX_PRECISION,
+ DECIMAL128_MAX_SCALE,
+ DataType::Decimal128,
+ DataType::Decimal128(DECIMAL128_MAX_PRECISION, DECIMAL_DEFAULT_SCALE),
+ ),
+ 32 => (
+ DECIMAL256_MAX_PRECISION,
+ DECIMAL256_MAX_SCALE,
+ DataType::Decimal256,
+ DataType::Decimal256(DECIMAL256_MAX_PRECISION, DECIMAL_DEFAULT_SCALE),
+ ),
+ _ => panic!("invalid byte width"),
Review Comment:
Maybe mention what is valid byte width in the message.
--
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940966348
##########
arrow/src/util/decimal.rs:
##########
@@ -18,21 +18,51 @@
//! Decimal related utils
use crate::datatypes::{
- DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE, DECIMAL256_MAX_PRECISION,
- DECIMAL256_MAX_SCALE,
+ DataType, DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE, DECIMAL256_MAX_PRECISION,
+ DECIMAL256_MAX_SCALE, DECIMAL_DEFAULT_SCALE,
};
use crate::error::{ArrowError, Result};
use num::bigint::BigInt;
use num::Signed;
use std::cmp::{min, Ordering};
-pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
- /// The bit-width of the internal representation.
- const BIT_WIDTH: usize;
- /// The maximum precision.
- const MAX_PRECISION: usize;
- /// The maximum scale.
- const MAX_SCALE: usize;
+#[derive(Debug)]
+pub struct BasicDecimal<const BYTE_WIDTH: usize> {
+ precision: usize,
+ scale: usize,
+ value: [u8; BYTE_WIDTH],
+}
+
+impl<const BYTE_WIDTH: usize> BasicDecimal<BYTE_WIDTH> {
+ #[allow(clippy::type_complexity)]
+ const _MAX_PRECISION_SCALE_CONSTRUCTOR_DEFAULT_TYPE: (
+ usize,
+ usize,
+ fn(usize, usize) -> DataType,
+ DataType,
+ ) = match BYTE_WIDTH {
+ 16 => (
+ DECIMAL128_MAX_PRECISION,
+ DECIMAL128_MAX_SCALE,
+ DataType::Decimal128,
+ DataType::Decimal128(DECIMAL128_MAX_PRECISION, DECIMAL_DEFAULT_SCALE),
+ ),
+ 32 => (
+ DECIMAL256_MAX_PRECISION,
+ DECIMAL256_MAX_SCALE,
+ DataType::Decimal256,
+ DataType::Decimal256(DECIMAL256_MAX_PRECISION, DECIMAL_DEFAULT_SCALE),
+ ),
+ _ => panic!("invalid byte width"),
Review Comment:
Looks okay.
--
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] HaoYang670 commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940961653
##########
arrow/src/util/decimal.rs:
##########
@@ -119,15 +165,44 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
}
}
+impl<const BYTE_WIDTH: usize> PartialOrd for BasicDecimal<BYTE_WIDTH> {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Review Comment:
Nope. If other has different byte width, then it will has a different type from `Self`.
--
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 #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383
--
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 #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940957463
##########
arrow/src/util/decimal.rs:
##########
@@ -41,7 +71,11 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
/// Safety:
/// This method doesn't validate if the decimal value represented by the bytes
/// can be fitted into the specified precision.
- fn try_new_from_bytes(precision: usize, scale: usize, bytes: &[u8]) -> Result<Self>
+ pub fn try_new_from_bytes(
+ precision: usize,
+ scale: usize,
+ bytes: &[u8],
Review Comment:
Could we make these methods take fixed length slices?
--
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940958113
##########
arrow/src/util/decimal.rs:
##########
@@ -119,15 +165,44 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
}
}
+impl<const BYTE_WIDTH: usize> PartialOrd for BasicDecimal<BYTE_WIDTH> {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Review Comment:
Hmm, can `other` be a `BasicDecimal` with different byte width?
--
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] viirya commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940971475
##########
arrow/src/util/decimal.rs:
##########
@@ -41,7 +71,11 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
/// Safety:
/// This method doesn't validate if the decimal value represented by the bytes
/// can be fitted into the specified precision.
- fn try_new_from_bytes(precision: usize, scale: usize, bytes: &[u8]) -> Result<Self>
+ pub fn try_new_from_bytes(
+ precision: usize,
+ scale: usize,
+ bytes: &[u8],
Review Comment:
I think it could be `&[u8; BYTE_WIDTH]`.
--
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] viirya commented on pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1209602704
I guess one point from @tustvold is that using fixed length slices when constructing decimals would help the compiler elide bounds checks, the performance gain might come from it.
--
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] HaoYang670 commented on a diff in pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#discussion_r940972006
##########
arrow/src/util/decimal.rs:
##########
@@ -119,15 +165,44 @@ pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
}
}
+impl<const BYTE_WIDTH: usize> PartialOrd for BasicDecimal<BYTE_WIDTH> {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Review Comment:
For compiler, the difference between `value` and `type` is that `value` cannot be known at compile time, they can only be known at runtime (by hardware).
However, `type` is known at compile time. But `type` only exists at compile time.
For `const value`, the thing goes to be interesting. Because
1. constant value can be known by compiler at compile time
2. constant value will exist at both compile time and runtime.
That why in Rust `const_generic` is possible.
@viirya
--
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] HaoYang670 commented on pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1209052375
I assume that this PR will not introduce performance regression. But we need more test because weird things often happen.
--
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] HaoYang670 commented on pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1209219101
Thank you for your review @viirya @tustvold
--
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] HaoYang670 commented on pull request #2383: Rewrite `Decimal` and `DecimalArray` using `const_generic`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2383:
URL: https://github.com/apache/arrow-rs/pull/2383#issuecomment-1210042034
> I guess one point from @tustvold is that using fixed length slices when constructing decimals would help the compiler elide bounds checks, the performance gain might come from it.
Yes, I found we have discussion about bound checking in #2360.
--
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