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 2020/11/18 03:18:33 UTC
[GitHub] [arrow] GregBowyer opened a new pull request #8698: Remove rust specialization
GregBowyer opened a new pull request #8698:
URL: https://github.com/apache/arrow/pull/8698
This is a very initial attempt at removing the specialization features from the Rust Parquet implementation.
The specialisation is too complex to be covered by `min_specialization` and requires a bit of reworking in the crate.
Right now the code dispatches in sub-traits and methods on the Parquet type, and uses a combination of trait abuse, macros and transmutes to eliminate the feature.
I have broken this up into several commits ranging from the simplest removals (which could probably be taken fairly easily) to the most ugly and complex.
I am not stoked on the `transmute` abuse, and I think another take (or follow up) should be taken to remove as many as possible in the code.
The general trait for `DataType::T` has been made a private sealed trait to make it impossible to implement external to the Parquet crate, this is intentional as I dont think many of the public interfaces are sensible for end users to be able to implement.
# TODO:
[ ] Purge other transmutes if possible
[ ] Refine and rationalise the `unimplemented!` implementations
[ ] Performance test?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-729388560
https://issues.apache.org/jira/browse/ARROW-10636
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534652635
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -229,6 +235,82 @@ impl fmt::Display for ByteArray {
}
}
+/// Wrapper type for performance reasons, this represents `FIXED_LEN_BYTE_ARRAY` but in all other
+/// considerations behaves the same as `ByteArray`
+#[repr(transparent)]
+#[derive(Clone, Debug, Default)]
+pub struct FixedLenByteArray(ByteArray);
+
+impl PartialEq for FixedLenByteArray {
+ fn eq(&self, other: &FixedLenByteArray) -> bool {
+ self.0.eq(&other.0)
+ }
+}
+
+impl PartialEq<ByteArray> for FixedLenByteArray {
+ fn eq(&self, other: &ByteArray) -> bool {
+ self.0.eq(other)
+ }
+}
+
+impl PartialEq<FixedLenByteArray> for ByteArray {
+ fn eq(&self, other: &FixedLenByteArray) -> bool {
+ self.eq(&other.0)
+ }
+}
+
+impl fmt::Display for FixedLenByteArray {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ self.0.fmt(f)
+ }
+}
+
+impl PartialOrd for FixedLenByteArray {
+ fn partial_cmp(&self, other: &FixedLenByteArray) -> Option<Ordering> {
+ self.0.partial_cmp(&other.0)
+ }
+}
+
+impl PartialOrd<FixedLenByteArray> for ByteArray {
+ fn partial_cmp(&self, other: &FixedLenByteArray) -> Option<Ordering> {
+ self.partial_cmp(&other.0)
+ }
+}
+
+impl PartialOrd<ByteArray> for FixedLenByteArray {
+ fn partial_cmp(&self, other: &ByteArray) -> Option<Ordering> {
+ self.0.partial_cmp(other)
+ }
+}
+
+use std::ops::{Deref, DerefMut};
Review comment:
NIT: This should move to the top with the rest of the use statements.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb closed pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #8698:
URL: https://github.com/apache/arrow/pull/8698
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534651119
##########
File path: .github/workflows/rust.yml
##########
@@ -50,7 +50,10 @@ jobs:
strategy:
fail-fast: false
matrix:
- rust: [nightly-2020-11-24]
Review comment:
I am ok with that, I am fighting the CI anyhow and any assistance is welcome.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534655181
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -583,32 +740,198 @@ pub(crate) mod private {
impl_from_raw!(f64, self => { Err(general_err!("Type cannot be converted to i64")) });
impl ParquetValueType for super::Int96 {
- fn encoded(&self) -> EncodedValue<'_> {
- EncodedValue::Bytes { data: self.as_bytes() }
+ #[inline]
+ fn encode<W: std::io::Write>(values: &[Self], writer: &mut W, _: &mut BitWriter) -> Result<()> {
+ for value in values {
+ let raw = unsafe {
+ std::slice::from_raw_parts(value.data() as *const [u32] as *const u8, 12)
+ };
+ writer.write_all(raw)?;
+ }
+ Ok(())
}
+ #[inline]
+ fn set_data(decoder: &mut PlainDecoderDetails, data: ByteBufferPtr, num_values: usize) {
+ decoder.data.replace(data);
+ decoder.start = 0;
+ decoder.num_values = num_values;
+ }
+
+ #[inline]
+ fn decode(buffer: &mut [Self], decoder: &mut PlainDecoderDetails) -> Result<usize> {
+ // TODO - Remove the duplication between this and the general slice method
+ let data = decoder.data.as_ref().expect("set_data should have been called");
+ let num_values = std::cmp::min(buffer.len(), decoder.num_values);
+ let bytes_left = data.len() - decoder.start;
+ let bytes_to_decode = 12 * num_values;
+
+ if bytes_left < bytes_to_decode {
+ return Err(eof_err!("Not enough bytes to decode"));
+ }
+
+ let data_range = data.range(decoder.start, bytes_to_decode);
+ let bytes: &[u8] = data_range.data();
+ decoder.start += bytes_to_decode;
+
+ let mut pos = 0; // position in byte array
+ for i in 0..num_values {
+ let elem0 = byteorder::LittleEndian::read_u32(&bytes[pos..pos + 4]);
+ let elem1 = byteorder::LittleEndian::read_u32(&bytes[pos + 4..pos + 8]);
+ let elem2 = byteorder::LittleEndian::read_u32(&bytes[pos + 8..pos + 12]);
+
+ buffer[i]
+ .as_mut_any()
+ .downcast_mut::<Self>()
+ .unwrap()
+ .set_data(elem0, elem1, elem2);
+
+ pos += 12;
+ }
+ decoder.num_values -= num_values;
+
+ Ok(num_values)
+ }
+
+ #[inline]
fn as_any(&self) -> &dyn std::any::Any {
self
}
+ #[inline]
fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
self
}
}
+ // TODO - Why does macro importing fail?
+ /// Reads `$size` of bytes from `$src`, and reinterprets them as type `$ty`, in
+ /// little-endian order. `$ty` must implement the `Default` trait. Otherwise this won't
+ /// compile.
+ /// This is copied and modified from byteorder crate.
+ macro_rules! read_num_bytes {
+ ($ty:ty, $size:expr, $src:expr) => {{
+ assert!($size <= $src.len());
+ let mut buffer = <$ty as $crate::util::bit_util::FromBytes>::Buffer::default();
+ buffer.as_mut()[..$size].copy_from_slice(&$src[..$size]);
+ <$ty>::from_ne_bytes(buffer)
+ }};
+ }
+
impl ParquetValueType for super::ByteArray {
- fn encoded(&self) -> EncodedValue<'_> {
- EncodedValue::Bytes { data: self.data() }
+ #[inline]
+ fn encode<W: std::io::Write>(values: &[Self], writer: &mut W, _: &mut BitWriter) -> Result<()> {
+ for value in values {
+ let len: u32 = value.len().try_into().unwrap();
+ writer.write_all(&len.to_ne_bytes())?;
+ let raw = value.data();
+ writer.write_all(raw)?;
+ }
+ Ok(())
+ }
+
+ #[inline]
+ fn set_data(decoder: &mut PlainDecoderDetails, data: ByteBufferPtr, num_values: usize) {
+ decoder.data.replace(data);
+ decoder.start = 0;
+ decoder.num_values = num_values;
+ }
+
+ #[inline]
+ fn decode(buffer: &mut [Self], decoder: &mut PlainDecoderDetails) -> Result<usize> {
+ let data = decoder.data.as_mut().expect("set_data should have been called");
+ let num_values = std::cmp::min(buffer.len(), decoder.num_values);
+ for i in 0..num_values {
+ let len: usize = read_num_bytes!(u32, 4, data.start_from(decoder.start).as_ref()) as usize;
+ decoder.start += std::mem::size_of::<u32>();
+
+ if data.len() < decoder.start + len {
+ return Err(eof_err!("Not enough bytes to decode"));
+ }
+
+ let val: &mut Self = buffer[i]
+ .as_mut_any()
+ .downcast_mut()
+ .unwrap();
+
+ val.set_data(data.range(decoder.start, len));
+ decoder.start += len;
+ }
+ decoder.num_values -= num_values;
+
+ Ok(num_values)
+ }
+
+ #[inline]
+ fn dict_encoding_size(&self) -> (usize, usize) {
+ (std::mem::size_of::<u32>(), self.len())
+ }
+
+ #[inline]
+ fn as_any(&self) -> &dyn std::any::Any {
+ self
}
+ #[inline]
+ fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
+ self
+ }
+ }
+
+ impl ParquetValueType for super::FixedLenByteArray {
Review comment:
The lifted `FixedLenByteArray` becomes used here to restore (actually make _better_) the performance of encoding and decoding.
The logic is I think different enough to warrant some duplication.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-744630179
I suggest merging this PR as is and then putting in the CI change as a follow on PR (this one is already large and long outstanding). Just let us know @GregBowyer
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8698: Remove rust specialization
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-729363125
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-736938052
I have been working on this w.r.t performance, I think I have most parts performing better than the original. I am running off clean benchmarks right now to validate.
There are a few ancillary tweaks I have made (I will call out in the PR review) that gain some additional performance (I spotted some pipeline hazards in `perf`) but nothing crazy
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534654706
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -583,32 +740,198 @@ pub(crate) mod private {
impl_from_raw!(f64, self => { Err(general_err!("Type cannot be converted to i64")) });
impl ParquetValueType for super::Int96 {
- fn encoded(&self) -> EncodedValue<'_> {
- EncodedValue::Bytes { data: self.as_bytes() }
+ #[inline]
+ fn encode<W: std::io::Write>(values: &[Self], writer: &mut W, _: &mut BitWriter) -> Result<()> {
+ for value in values {
+ let raw = unsafe {
+ std::slice::from_raw_parts(value.data() as *const [u32] as *const u8, 12)
+ };
+ writer.write_all(raw)?;
+ }
+ Ok(())
}
+ #[inline]
+ fn set_data(decoder: &mut PlainDecoderDetails, data: ByteBufferPtr, num_values: usize) {
+ decoder.data.replace(data);
+ decoder.start = 0;
+ decoder.num_values = num_values;
+ }
+
+ #[inline]
+ fn decode(buffer: &mut [Self], decoder: &mut PlainDecoderDetails) -> Result<usize> {
+ // TODO - Remove the duplication between this and the general slice method
+ let data = decoder.data.as_ref().expect("set_data should have been called");
Review comment:
Performance notes:
This code is lifted from elsewhere so its useful to state out loud what change was made.
Other than a small amount of shuffling the code about for general style, A lot of the pre-existing code has a pattern of the form
```rust
assert!(self.some_option.is_some())
let some_option = self.some_option.unwrap()
```
This unfortunately is not spotted by the compiler and seems to generate multiple branches that just consume CPU cycles.
Since the validations are useful, in this (and many places) the logic is folded into doing an unwrap (with a little more error messaging to state what invariant is being tested for)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-741989525
I am tempted to take a look at the compressors in a follow up pull request.
I will try to get the review comments dealt with today
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r526207035
##########
File path: rust/parquet/src/column/writer.rs
##########
@@ -955,81 +963,31 @@ impl<T: DataType> ColumnWriterImpl<T> {
/// Trait to define default encoding for types, including whether or not the type
/// supports dictionary encoding.
trait EncodingWriteSupport {
- /// Returns encoding for a column when no other encoding is provided in writer
- /// properties.
- fn fallback_encoding(props: &WriterProperties) -> Encoding;
-
/// Returns true if dictionary is supported for column writer, false otherwise.
fn has_dictionary_support(props: &WriterProperties) -> bool;
}
-// Basic implementation, always falls back to PLAIN and supports dictionary.
-impl<T: DataType> EncodingWriteSupport for ColumnWriterImpl<T> {
- default fn fallback_encoding(_props: &WriterProperties) -> Encoding {
- Encoding::PLAIN
- }
-
- default fn has_dictionary_support(_props: &WriterProperties) -> bool {
- true
- }
-}
-
-impl EncodingWriteSupport for ColumnWriterImpl<BoolType> {
- fn fallback_encoding(props: &WriterProperties) -> Encoding {
- match props.writer_version() {
- WriterVersion::PARQUET_1_0 => Encoding::PLAIN,
- WriterVersion::PARQUET_2_0 => Encoding::RLE,
- }
- }
-
- // Boolean column does not support dictionary encoding and should fall back to
- // whatever fallback encoding is defined.
- fn has_dictionary_support(_props: &WriterProperties) -> bool {
- false
- }
-}
-
-impl EncodingWriteSupport for ColumnWriterImpl<Int32Type> {
- fn fallback_encoding(props: &WriterProperties) -> Encoding {
- match props.writer_version() {
- WriterVersion::PARQUET_1_0 => Encoding::PLAIN,
- WriterVersion::PARQUET_2_0 => Encoding::DELTA_BINARY_PACKED,
- }
+/// Returns encoding for a column when no other encoding is provided in writer properties.
+fn fallback_encoding(kind: Type, props: &WriterProperties) -> Encoding {
+ match (kind, props.writer_version()) {
+ (Type::BOOLEAN, WriterVersion::PARQUET_2_0) => Encoding::RLE,
+ (Type::INT32, WriterVersion::PARQUET_2_0) => Encoding::DELTA_BINARY_PACKED,
+ (Type::INT64, WriterVersion::PARQUET_2_0) => Encoding::DELTA_BINARY_PACKED,
+ (Type::BYTE_ARRAY, WriterVersion::PARQUET_2_0) => Encoding::DELTA_BYTE_ARRAY,
+ (Type::FIXED_LEN_BYTE_ARRAY, WriterVersion::PARQUET_2_0) => Encoding::DELTA_BYTE_ARRAY,
+ _ => Encoding::PLAIN,
}
}
-impl EncodingWriteSupport for ColumnWriterImpl<Int64Type> {
- fn fallback_encoding(props: &WriterProperties) -> Encoding {
- match props.writer_version() {
- WriterVersion::PARQUET_1_0 => Encoding::PLAIN,
- WriterVersion::PARQUET_2_0 => Encoding::DELTA_BINARY_PACKED,
- }
- }
-}
-
-impl EncodingWriteSupport for ColumnWriterImpl<ByteArrayType> {
- fn fallback_encoding(props: &WriterProperties) -> Encoding {
- match props.writer_version() {
- WriterVersion::PARQUET_1_0 => Encoding::PLAIN,
- WriterVersion::PARQUET_2_0 => Encoding::DELTA_BYTE_ARRAY,
- }
- }
-}
-
-impl EncodingWriteSupport for ColumnWriterImpl<FixedLenByteArrayType> {
- fn fallback_encoding(props: &WriterProperties) -> Encoding {
- match props.writer_version() {
- WriterVersion::PARQUET_1_0 => Encoding::PLAIN,
- WriterVersion::PARQUET_2_0 => Encoding::DELTA_BYTE_ARRAY,
- }
- }
-
- fn has_dictionary_support(props: &WriterProperties) -> bool {
- match props.writer_version() {
- // Dictionary encoding was not enabled in PARQUET 1.0
- WriterVersion::PARQUET_1_0 => false,
- WriterVersion::PARQUET_2_0 => true,
- }
+/// Returns true if dictionary is supported for column writer, false otherwise.
Review comment:
I carefully checked the logic in this file and I believe it matches the original without the need for specialization. π
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -424,16 +460,165 @@ impl AsBytes for str {
}
}
-/// Contains the Parquet physical type information as well as the Rust primitive type
-/// presentation.
-pub trait DataType: 'static {
- type T: std::cmp::PartialEq
+pub(crate) mod private {
+ use super::{AsBytes, Result, ParquetError};
+
+ pub type BitIndex = u64;
+
+ #[derive(Copy, Clone)]
+ pub enum EncodedValue<'a> {
+ /// The value can be encoded from the following bytes
+ Bytes {
+ data: &'a [u8]
+ },
+ /// The value encodes as a specific set bit at a given index
+ Bits{
+ index: BitIndex
+ },
+ }
+
+ /// Sealed trait to start to remove specialisation from implementations
+ ///
+ /// This is done to force the associated value type to be unimplementable outside of this
+ /// crate, and thus hint to the type system (and end user) traits are public for the contract
+ /// and not for extension.
+ pub trait ParquetValueType : std::cmp::PartialEq
+ std::fmt::Debug
+ + std::fmt::Display
+ std::default::Default
+ std::clone::Clone
- + AsBytes
- + FromBytes
- + PartialOrd;
+ + super::AsBytes
+ + super::FromBytes
+ + super::SliceAsBytes
+ + PartialOrd
+ {
+ /// Return the most primitive version of encoding a given type
+ fn encoded(&self) -> EncodedValue<'_>;
+
+ /// Return the encoded size for a type
+ fn dict_encoding_size(&self) -> (usize, usize) {
+ (std::mem::size_of::<Self>(), 1)
+ }
+
+ /// Return the value as i64 if possible
+ ///
+ /// This is essentially the same as `std::convert::TryInto<i64>` but can
+ /// implemented for `f32` and `f64`, types that would fail orphan rules
+ fn as_i64(&self) -> Result<i64> {
+ Err(general_err!("Type cannot be converted to i64"))
+ }
+
+ /// Return the value as u64 if possible
+ ///
+ /// This is essentially the same as `std::convert::TryInto<u64>` but can
+ /// implemented for `f32` and `f64`, types that would fail orphan rules
+ fn as_u64(&self) -> Result<u64> {
+ self.as_i64()
+ .map_err(|_| general_err!("Type cannot be converted to u64"))
+ .map(|x| x as u64)
+ }
+
+ /// Return the value as an Any to allow for downcasts without transmutation
+ fn as_any(&self) -> &dyn std::any::Any;
+
+ /// Return the value as an mutable Any to allow for downcasts without transmutation
+ fn as_mut_any(&mut self) -> &mut dyn std::any::Any;
+ }
+
+ impl ParquetValueType for bool {
+ fn encoded(&self) -> EncodedValue<'_> {
+ EncodedValue::Bits { index: *self as u64 }
+ }
+
+ fn as_i64(&self) -> Result<i64> {
+ Ok(*self as i64)
+ }
+
+ fn as_any(&self) -> &dyn std::any::Any {
+ self
+ }
+
+ fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
+ self
+ }
+ }
+
+ /// Hopelessly unsafe function that emulates `num::as_ne_bytes`
+ ///
+ /// It is not recommended to use this outside of this private module as, while it
+ /// _should_ work for primitive values, it is little better than a transmutation
+ /// and can act as a backdoor into mis-interpreting types as arbitary byte slices
+ fn as_raw<'a, T>(value: *const T) -> &'a [u8] {
+ unsafe {
+ let value = value as *const u8;
+ std::slice::from_raw_parts(value, std::mem::size_of::<T>())
+ }
+ }
+
+ macro_rules! impl_from_raw {
+ ($ty: ty, $self: ident => $as_i64: block) => {
+ impl ParquetValueType for $ty {
+ fn encoded(&self) -> EncodedValue<'_> {
+ EncodedValue::Bytes { data: as_raw(&*self) }
+ }
+
+ fn as_i64(&$self) -> Result<i64> {
+ $as_i64
+ }
+
+ fn as_any(&self) -> &dyn std::any::Any {
+ self
+ }
+
+ fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
+ self
+ }
+ }
+ }
+ }
+
+ impl_from_raw!(i32, self => { Ok(*self as i64) });
+ impl_from_raw!(i64, self => { Ok(*self) });
+ impl_from_raw!(f32, self => { Err(general_err!("Type cannot be converted to i64")) });
+ impl_from_raw!(f64, self => { Err(general_err!("Type cannot be converted to i64")) });
Review comment:
```suggestion
impl_from_raw!(f32, self => { Err(general_err!("Type f32 cannot be converted to i64")) });
impl_from_raw!(f64, self => { Err(general_err!("Type f64 cannot be converted to i64")) });
```
##########
File path: rust/rust-toolchain
##########
@@ -1 +0,0 @@
-nightly-2020-11-14
Review comment:
π
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -424,16 +460,165 @@ impl AsBytes for str {
}
}
-/// Contains the Parquet physical type information as well as the Rust primitive type
-/// presentation.
-pub trait DataType: 'static {
- type T: std::cmp::PartialEq
+pub(crate) mod private {
+ use super::{AsBytes, Result, ParquetError};
+
+ pub type BitIndex = u64;
+
+ #[derive(Copy, Clone)]
+ pub enum EncodedValue<'a> {
+ /// The value can be encoded from the following bytes
+ Bytes {
+ data: &'a [u8]
+ },
+ /// The value encodes as a specific set bit at a given index
Review comment:
```suggestion
/// The value encoded as a specific set bit at a given index
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-729397709
I think I have a solution with `std::any::Any` for most of the transmutes
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534653851
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -558,18 +674,59 @@ pub(crate) mod private {
macro_rules! impl_from_raw {
($ty: ty, $self: ident => $as_i64: block) => {
impl ParquetValueType for $ty {
- fn encoded(&self) -> EncodedValue<'_> {
- EncodedValue::Bytes { data: as_raw(&*self) }
+ #[inline]
+ fn encode<W: std::io::Write>(values: &[Self], writer: &mut W, _: &mut BitWriter) -> Result<()> {
+ let raw = unsafe {
+ std::slice::from_raw_parts(
+ values.as_ptr() as *const u8,
+ std::mem::size_of::<$ty>() * values.len(),
+ )
+ };
+ writer.write_all(raw)?;
+
+ Ok(())
+ }
+
+ #[inline]
+ fn set_data(decoder: &mut PlainDecoderDetails, data: ByteBufferPtr, num_values: usize) {
+ decoder.data.replace(data);
+ decoder.start = 0;
+ decoder.num_values = num_values;
+ }
+
+ #[inline]
+ fn decode(buffer: &mut [Self], decoder: &mut PlainDecoderDetails) -> Result<usize> {
+ let data = decoder.data.as_mut().unwrap();
Review comment:
NIT: This should follow the pattern of avoiding the double check by using `expect`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-740144213
Its all good, I expect this to go for a few rounds of review and tweak (a rebase on #8821 is probably in order to).
As a word of warning the full benchmark suite takes quite some time to run
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-740173397
> As a word of warning the full benchmark suite takes quite some time to run
@GregBowyer indeed -- I have spun up a VM in google cloud in fact to run it :) The numbers are being gathered now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-740585522
Well, I left the benchmarks running overnight on my cloud instance and they haven't finished yet... I am going to spend some time trying to pair them down to run in a more reasonable timeframe
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-729478929
I think I pulled all the transmutes out so thats good
> Thanks @GregBowyer for the PR. It's will be awesome if we can get this done. I think before merging this we should get some benchmark results showing the diff before & after. We use to have [some benchmark in the old repo](https://github.com/sunchao/parquet-rs/tree/master/benches) but haven't port them to arrow yet.
I will just pull them in tomorrow and get some baselines, I dont think it will be that hard.
I _doubt_ there will be anything here for performance. I mean there are some more branches now, but I suspect the predict effortlessly, and on the flip side we are not chasing things around the I-Cache so I suspect its a wash in the end. However better checked than assumed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-737668292
Benchmarks are here:
http://gbowyer.freeshell.org/rust-parquet-perf-test/report/
Raw baseline:
http://gbowyer.freeshell.org/rust-parquet-perf-test/baseline_raw.txt
Raw output for changes:
http://gbowyer.freeshell.org/rust-parquet-perf-test/changes_raw.txt
# Machine
```
processor : 31
vendor_id : GenuineIntel
cpu family : 6
model : 45
model name : Intel(R) Xeon(R) CPU E5-2670 0 @ 2.60GHz
stepping : 7
microcode : 0x710
cpu MHz : 2611.349
cache size : 20480 KB
physical id : 1
siblings : 16
core id : 7
cpu cores : 8
apicid : 47
initial apicid : 47
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc cpuid aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx lahf_lm epb pti tpr_shadow vnmi flexpriority ept vpid xsaveopt dtherm ida arat pln pts
vmx flags : vnmi preemption_timer invvpid ept_x_only ept_1gb flexpriority tsc_offset vtpr mtf vapic ept vpid unrestricted_guest ple
bugs : cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass l1tf mds swapgs itlb_multihit
bogomips : 5241.92
clflush size : 64
cache_alignment : 64
address sizes : 46 bits physical, 48 bits virtual
power management:
```
Some notes:
* The compression benchmark seems super noisy and I dont think it matters for this change
* Most of the benchmarks seem to have got faster with the performance tweaks
* I think allocs are a little too noisy to care about
* `_boolean` seems to have got a little slower, I cannot for the life of me figure out why.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Dandandan commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-745566525
Wow, awesome work everyone π―
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534657727
##########
File path: rust/parquet/src/util/bit_util.rs
##########
@@ -124,11 +125,7 @@ where
/// Returns the ceil of value/divisor
#[inline]
pub fn ceil(value: i64, divisor: i64) -> i64 {
- let mut result = value / divisor;
- if value % divisor != 0 {
- result += 1
- };
- result
+ value / divisor + ((value % divisor != 0) as i64)
Review comment:
Performance notes:
Small but measurable performance tweak, we can elide the branch and add on the `+1` from the gaurentees about the representation of the boolean
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-745559206
Filed https://issues.apache.org/jira/browse/ARROW-10929 to track migrating CI to stable rust
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r538756445
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -327,7 +422,12 @@ pub trait AsBytes {
pub trait SliceAsBytes: Sized {
/// Returns slice of bytes for a slice of this data type.
fn slice_as_bytes(self_: &[Self]) -> &[u8];
- fn slice_as_bytes_mut(self_: &mut [Self]) -> &mut [u8];
+ /// Return the internal representation as a mutable slice
+ ///
+ /// # Safety
+ /// If modified you are _required_ to ensure the internal representation
+ /// is valid and correct for the actual raw data
+ unsafe fn slice_as_bytes_mut(self_: &mut [Self]) -> &mut [u8];
Review comment:
Notes to other reviewers -- this isn't any new unsafe that was added -- I think it was just hoised into a trait (previously the code inside the impl was marked as unsafe)
##########
File path: rust/parquet/src/encodings/decoding.rs
##########
@@ -384,36 +278,18 @@ impl<T: DataType> Decoder<T> for DictDecoder<T> {
/// See [`RleValueEncoder`](crate::encoding::RleValueEncoder) for more information.
pub struct RleValueDecoder<T: DataType> {
values_left: usize,
- decoder: Option<RleDecoder>,
+ decoder: RleDecoder,
Review comment:
My results show a significant improvement in rle encoder and decoder, FWIW.
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -424,16 +552,396 @@ impl AsBytes for str {
}
}
-/// Contains the Parquet physical type information as well as the Rust primitive type
-/// presentation.
-pub trait DataType: 'static {
- type T: std::cmp::PartialEq
+pub(crate) mod private {
+ use crate::encodings::decoding::PlainDecoderDetails;
+ use crate::util::bit_util::{BitWriter, BitReader};
+ use crate::util::memory::ByteBufferPtr;
+
+ use byteorder::ByteOrder;
+ use std::convert::TryInto;
+
+ use super::{Result, ParquetError, SliceAsBytes};
+
+ pub type BitIndex = u64;
+
+ /// Sealed trait to start to remove specialisation from implementations
+ ///
+ /// This is done to force the associated value type to be unimplementable outside of this
+ /// crate, and thus hint to the type system (and end user) traits are public for the contract
+ /// and not for extension.
+ pub trait ParquetValueType : std::cmp::PartialEq
+ std::fmt::Debug
+ + std::fmt::Display
+ std::default::Default
+ std::clone::Clone
- + AsBytes
- + FromBytes
- + PartialOrd;
+ + super::AsBytes
+ + super::FromBytes
+ + super::SliceAsBytes
+ + PartialOrd
+ {
+ /// Encode the value directly from a higher level encoder
+ fn encode<W: std::io::Write>(values: &[Self], writer: &mut W, bit_writer: &mut BitWriter) -> Result<()>;
+
+ /// Establish the data that will be decoded in a buffer
+ fn set_data(decoder: &mut PlainDecoderDetails, data: ByteBufferPtr, num_values: usize);
+
+ /// Decode the value from a given buffer for a higher level decoder
+ fn decode(buffer: &mut [Self], decoder: &mut PlainDecoderDetails) -> Result<usize>;
+
+ /// Return the encoded size for a type
+ fn dict_encoding_size(&self) -> (usize, usize) {
+ (std::mem::size_of::<Self>(), 1)
+ }
+
+ /// Return the value as i64 if possible
+ ///
+ /// This is essentially the same as `std::convert::TryInto<i64>` but can
+ /// implemented for `f32` and `f64`, types that would fail orphan rules
+ fn as_i64(&self) -> Result<i64> {
+ Err(general_err!("Type cannot be converted to i64"))
+ }
+
+ /// Return the value as u64 if possible
+ ///
+ /// This is essentially the same as `std::convert::TryInto<u64>` but can
+ /// implemented for `f32` and `f64`, types that would fail orphan rules
+ fn as_u64(&self) -> Result<u64> {
+ self.as_i64()
+ .map_err(|_| general_err!("Type cannot be converted to u64"))
+ .map(|x| x as u64)
+ }
+
+ /// Return the value as an Any to allow for downcasts without transmutation
+ fn as_any(&self) -> &dyn std::any::Any;
+
+ /// Return the value as an mutable Any to allow for downcasts without transmutation
+ fn as_mut_any(&mut self) -> &mut dyn std::any::Any;
+ }
+
+ impl ParquetValueType for bool {
+ #[inline]
+ fn encode<W: std::io::Write>(values: &[Self], _: &mut W, bit_writer: &mut BitWriter) -> Result<()> {
+ for value in values {
+ bit_writer.put_value(*value as u64, 1);
+ }
+ Ok(())
+ }
+
+ #[inline]
+ fn set_data(decoder: &mut PlainDecoderDetails, data: ByteBufferPtr, num_values: usize) {
+ decoder.bit_reader.replace(BitReader::new(data));
+ decoder.num_values = num_values;
+ }
+
+ #[inline]
+ fn decode(buffer: &mut [Self], decoder: &mut PlainDecoderDetails) -> Result<usize> {
+ let bit_reader = decoder.bit_reader.as_mut().unwrap();
+ let num_values = std::cmp::min(buffer.len(), decoder.num_values);
+ let values_read = bit_reader.get_batch(&mut buffer[..num_values], 1);
+ decoder.num_values -= values_read;
+ Ok(values_read)
+ }
+
+ #[inline]
+ fn as_i64(&self) -> Result<i64> {
+ Ok(*self as i64)
+ }
+
+ #[inline]
+ fn as_any(&self) -> &dyn std::any::Any {
+ self
+ }
+
+ #[inline]
+ fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
+ self
+ }
+ }
+
+ /// Hopelessly unsafe function that emulates `num::as_ne_bytes`
+ ///
+ /// It is not recommended to use this outside of this private module as, while it
+ /// _should_ work for primitive values, it is little better than a transmutation
+ /// and can act as a backdoor into mis-interpreting types as arbitary byte slices
+ #[inline]
+ fn as_raw<'a, T>(value: *const T) -> &'a [u8] {
+ unsafe {
+ let value = value as *const u8;
+ std::slice::from_raw_parts(value, std::mem::size_of::<T>())
+ }
+ }
+
+ macro_rules! impl_from_raw {
+ ($ty: ty, $self: ident => $as_i64: block) => {
+ impl ParquetValueType for $ty {
+ #[inline]
+ fn encode<W: std::io::Write>(values: &[Self], writer: &mut W, _: &mut BitWriter) -> Result<()> {
+ let raw = unsafe {
+ std::slice::from_raw_parts(
+ values.as_ptr() as *const u8,
+ std::mem::size_of::<$ty>() * values.len(),
+ )
+ };
+ writer.write_all(raw)?;
+
+ Ok(())
+ }
+
+ #[inline]
+ fn set_data(decoder: &mut PlainDecoderDetails, data: ByteBufferPtr, num_values: usize) {
+ decoder.data.replace(data);
+ decoder.start = 0;
+ decoder.num_values = num_values;
+ }
+
+ #[inline]
+ fn decode(buffer: &mut [Self], decoder: &mut PlainDecoderDetails) -> Result<usize> {
+ let data = decoder.data.as_mut().unwrap();
+ let num_values = std::cmp::min(buffer.len(), decoder.num_values);
+ let bytes_left = data.len() - decoder.start;
+ let bytes_to_decode = std::mem::size_of::<Self>() * num_values;
+
+ if bytes_left < bytes_to_decode {
+ return Err(eof_err!("Not enough bytes to decode"));
+ }
+
+ // SAFETY: Raw types should be as per the standard rust bit-vectors
+ unsafe {
+ let raw_buffer = &mut Self::slice_as_bytes_mut(buffer)[..bytes_to_decode];
+ raw_buffer.copy_from_slice(data.range(decoder.start, bytes_to_decode).as_ref());
+ };
+ decoder.start += bytes_to_decode;
+ decoder.num_values -= num_values;
+
+ Ok(num_values)
+ }
+
+ #[inline]
+ fn as_i64(&$self) -> Result<i64> {
+ $as_i64
+ }
+
+ #[inline]
+ fn as_any(&self) -> &dyn std::any::Any {
+ self
+ }
+
+ #[inline]
+ fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
+ self
+ }
+ }
+ }
+ }
+
+ impl_from_raw!(i32, self => { Ok(*self as i64) });
+ impl_from_raw!(i64, self => { Ok(*self) });
+ impl_from_raw!(f32, self => { Err(general_err!("Type cannot be converted to i64")) });
+ impl_from_raw!(f64, self => { Err(general_err!("Type cannot be converted to i64")) });
+
+ impl ParquetValueType for super::Int96 {
+ #[inline]
+ fn encode<W: std::io::Write>(values: &[Self], writer: &mut W, _: &mut BitWriter) -> Result<()> {
+ for value in values {
+ let raw = unsafe {
+ std::slice::from_raw_parts(value.data() as *const [u32] as *const u8, 12)
+ };
+ writer.write_all(raw)?;
+ }
+ Ok(())
+ }
+
+ #[inline]
+ fn set_data(decoder: &mut PlainDecoderDetails, data: ByteBufferPtr, num_values: usize) {
+ decoder.data.replace(data);
+ decoder.start = 0;
+ decoder.num_values = num_values;
+ }
+
+ #[inline]
+ fn decode(buffer: &mut [Self], decoder: &mut PlainDecoderDetails) -> Result<usize> {
+ // TODO - Remove the duplication between this and the general slice method
+ let data = decoder.data.as_ref().expect("set_data should have been called");
+ let num_values = std::cmp::min(buffer.len(), decoder.num_values);
+ let bytes_left = data.len() - decoder.start;
+ let bytes_to_decode = 12 * num_values;
+
+ if bytes_left < bytes_to_decode {
+ return Err(eof_err!("Not enough bytes to decode"));
+ }
+
+ let data_range = data.range(decoder.start, bytes_to_decode);
+ let bytes: &[u8] = data_range.data();
+ decoder.start += bytes_to_decode;
+
+ let mut pos = 0; // position in byte array
+ for i in 0..num_values {
+ let elem0 = byteorder::LittleEndian::read_u32(&bytes[pos..pos + 4]);
+ let elem1 = byteorder::LittleEndian::read_u32(&bytes[pos + 4..pos + 8]);
+ let elem2 = byteorder::LittleEndian::read_u32(&bytes[pos + 8..pos + 12]);
+
+ buffer[i]
+ .as_mut_any()
+ .downcast_mut::<Self>()
+ .unwrap()
+ .set_data(elem0, elem1, elem2);
+
+ pos += 12;
+ }
+ decoder.num_values -= num_values;
+
+ Ok(num_values)
+ }
+
+ #[inline]
+ fn as_any(&self) -> &dyn std::any::Any {
+ self
+ }
+
+ #[inline]
+ fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
+ self
+ }
+ }
+
+ // TODO - Why does macro importing fail?
Review comment:
I don't understand this comment
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -229,6 +235,82 @@ impl fmt::Display for ByteArray {
}
}
+/// Wrapper type for performance reasons, this represents `FIXED_LEN_BYTE_ARRAY` but in all other
+/// considerations behaves the same as `ByteArray`
+#[repr(transparent)]
+#[derive(Clone, Debug, Default)]
+pub struct FixedLenByteArray(ByteArray);
Review comment:
I suggest you add this context in the comments of this file, for the benefit of future readers (who will likely look at the code and wonder why there is a wrapper type that doesn't seem to add any additional logic)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534656244
##########
File path: rust/parquet/src/encodings/encoding.rs
##########
@@ -130,6 +130,7 @@ impl<T: DataType> PlainEncoder<T> {
}
impl<T: DataType> Encoder<T> for PlainEncoder<T> {
+ #[cold]
Review comment:
Performance Notes:
As far as I can tell these functions are rarely called and as such we can hint to the compiler that they dont need to be folded into hot locations in the final output.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-740958408
@alamb benchmarks formatted as a table (from worse to best, statistical insignificant and changes <5% ignored). No judgement, just info:
| benchmark | variation (%) |
|-------------- | -------------- |
| decompress_snappy::wall_time/fixed | 11458.0 |
| compress_snappy::wall_time/fixed | 11265.0 |
| compress_gzip::wall_time/int32 | 6472.1 |
| compress_brotli::wall_time/int32 | 4928.4 |
| decompress_brotli::wall_time/int96 | 4830.6 |
| decompress_snappy::wall_time/int96 | 3491.3 |
| decompress_gzip::wall_time/int96 | 3269.0 |
| decompress_brotli::wall_time/fixed | 2586.1 |
| compress_gzip::wall_time/int96 | 2534.1 |
| decompress_zstd::wall_time/double | 2414.9 |
| compress_snappy::wall_time/int96 | 2146.0 |
| compress_zstd::wall_time/int32 | 1895.5 |
| decompress_zstd::wall_time/int32 | 1585.4 |
| compress_gzip::wall_time/double | 1409.6 |
| decompress_snappy::wall_time/double | 1407.4 |
| compress_brotli::wall_time/int96 | 1344.0 |
| compress_lz4::wall_time/double | 1246.5 |
| decompress_gzip::wall_time/double | 1245.3 |
| decompress_lz4::wall_time/int96 | 1082.6 |
| compress_brotli::wall_time/double | 1051.9 |
| compress_snappy::wall_time/int32 | 983.8 |
| decompress_lz4::wall_time/double | 965.8 |
| compress_snappy::wall_time/double | 856.6 |
| decompress_gzip::wall_time/int32 | 739.0 |
| decompress_brotli::wall_time/int32 | 611.3 |
| decompress_lz4::wall_time/fixed | 593.6 |
| decompress_brotli::wall_time/double | 568.2 |
| compress_zstd::wall_time/double | 563.5 |
| decompress_zstd::wall_time/int96 | 509.0 |
| compress_lz4::wall_time/int96 | 490.8 |
| compress_lz4::wall_time/int32 | 470.8 |
| decompress_gzip::wall_time/fixed | 469.8 |
| compress_zstd::wall_time/int96 | 428.8 |
| decompress_lz4::wall_time/int32 | 426.8 |
| compress_brotli::wall_time/fixed | 377.3 |
| compress_gzip::wall_time/fixed | 257.7 |
| decompress_brotli::wall_time/float | 199.0 |
| decompress_zstd::wall_time/fixed | 196.2 |
| compress_brotli::wall_time/float | 126.2 |
| decompress_zstd::wall_time/float | 121.9 |
| compress_lz4::wall_time/fixed | 108.4 |
| compress_lz4::wall_time/binary | 72.6 |
| decompress_snappy::wall_time/int32 | 38.5 |
| delta_len_byte_arry_decoder_byte_array::wall_time/128 | 38.2 |
| delta_byte_arry_decoder_byte_array::wall_time/64 | 37.9 |
| delta_byte_arry_decoder_byte_array::wall_time/16 | 37.9 |
| delta_byte_arry_decoder_byte_array::wall_time/8 | 37.8 |
| delta_len_byte_arry_decoder_byte_array::wall_time/8 | 37.8 |
| delta_len_byte_arry_decoder_byte_array::wall_time/32 | 37.7 |
| compress_zstd::wall_time/fixed | 37.5 |
| delta_len_byte_arry_decoder_byte_array::wall_time/16 | 37.5 |
| delta_byte_arry_decoder_byte_array::wall_time/32 | 37.5 |
| delta_byte_arry_decoder_byte_array::wall_time/128 | 37.4 |
| delta_len_byte_arry_decoder_byte_array::wall_time/64 | 35.3 |
| plain_encoder_byte_array::wall_time/64 | 31.8 |
| plain_encoder_byte_array::wall_time/128 | 14.4 |
| delta_byte_arry_encoder_byte_array::wall_time/8 | 11.5 |
| delta_bin_packed_encoder_int64::wall_time/64 | 11.2 |
| delta_len_byte_arry_encoder_byte_array::wall_time/8 | 10.5 |
| decompress_snappy::wall_time/binary | 10.4 |
| compress_zstd::wall_time/binary | 9.3 |
| delta_byte_arry_encoder_byte_array::wall_time/32 | 8.4 |
| plain_decoder_boolean::wall_time/64 | 8.2 |
| plain_decoder_boolean::wall_time/32 | 8.1 |
| delta_bin_packed_decoder_int32::wall_time/128 | 7.8 |
| delta_bin_packed_decoder_int64::wall_time/32 | 7.8 |
| delta_bin_packed_encoder_int64::wall_time/16 | 7.8 |
| delta_bin_packed_decoder_int32::wall_time/64 | 7.7 |
| delta_bin_packed_decoder_int64::wall_time/8 | 7.7 |
| delta_bin_packed_decoder_int64::wall_time/16 | 7.7 |
| plain_decoder_boolean::wall_time/8 | 7.7 |
| delta_bin_packed_decoder_int32::wall_time/16 | 7.6 |
| delta_bin_packed_decoder_int32::wall_time/32 | 7.5 |
| delta_bin_packed_decoder_int32::wall_time/8 | 7.5 |
| delta_bin_packed_decoder_int64::wall_time/64 | 7.5 |
| delta_bin_packed_decoder_int64::wall_time/128 | 7.4 |
| plain_decoder_boolean::wall_time/16 | 7.4 |
| delta_len_byte_arry_encoder_byte_array::wall_time/16 | 7.4 |
| plain_decoder_boolean::wall_time/128 | 7.4 |
| delta_byte_arry_encoder_byte_array::wall_time/64 | 6.9 |
| delta_byte_arry_encoder_byte_array::wall_time/16 | 6.8 |
| plain_decoder_float::wall_time/8 | 6.6 |
| decompress_lz4::wall_time/binary | 6.6 |
| plain_encoder_byte_array::wall_time/32 | 5.7 |
| delta_bin_packed_encoder_int64::wall_time/8 | 5.6 |
| delta_bin_packed_encoder_int64::wall_time/32 | 5.4 |
| dict_decoder_int64::wall_time/64 | 5.1 |
| dict_decoder_int64::wall_time/8 | 5.0 |
| plain_encoder_byte_array::wall_time/16 | -5.1 |
| dict_encoder_float::wall_time/8 | -5.5 |
| dict_encoder_byte_array::wall_time/8 | -6.3 |
| dict_encoder_int96::wall_time/64 | -6.4 |
| plain_encoder_int64::wall_time/32 | -7.3 |
| plain_encoder_byte_array::wall_time/8 | -7.8 |
| reader_wall_time/stock-simulated-column | -7.9 |
| dict_encoder_double::wall_time/128 | -9.6 |
| plain_encoder_int32::wall_time/128 | -10.0 |
| plain_encoder_float::wall_time/128 | -10.4 |
| plain_encoder_int96::wall_time/128 | -10.7 |
| plain_encoder_float::wall_time/64 | -10.7 |
| plain_encoder_int32::wall_time/64 | -10.8 |
| plain_encoder_int32::wall_time/32 | -11.5 |
| compress_gzip::wall_time/binary | -11.6 |
| plain_encoder_int96::wall_time/16 | -11.6 |
| plain_encoder_float::wall_time/32 | -11.6 |
| plain_encoder_double::wall_time/32 | -11.7 |
| plain_encoder_int32::wall_time/16 | -11.9 |
| decompress_gzip::wall_time/binary | -12.1 |
| plain_encoder_float::wall_time/8 | -12.1 |
| plain_encoder_float::wall_time/16 | -12.2 |
| plain_encoder_int96::wall_time/8 | -12.2 |
| plain_encoder_int32::wall_time/8 | -12.3 |
| plain_encoder_int96::wall_time/64 | -12.7 |
| plain_encoder_double::wall_time/128 | -12.7 |
| plain_encoder_int96::wall_time/32 | -12.8 |
| plain_encoder_int64::wall_time/16 | -12.9 |
| plain_encoder_int64::wall_time/64 | -13.2 |
| plain_encoder_double::wall_time/64 | -13.3 |
| plain_encoder_double::wall_time/16 | -13.5 |
| plain_encoder_double::wall_time/8 | -14.3 |
| rle_encoder_boolean::wall_time/8 | -14.7 |
| plain_encoder_int64::wall_time/128 | -15.1 |
| plain_encoder_boolean::wall_time/8 | -16.4 |
| plain_encoder_int64::wall_time/8 | -17.2 |
| rle_encoder_boolean::wall_time/16 | -20.5 |
| plain_encoder_boolean::wall_time/16 | -24.7 |
| rle_encoder_boolean::wall_time/32 | -25.5 |
| plain_decoder_float::wall_time/16 | -28.3 |
| rle_encoder_boolean::wall_time/64 | -29.5 |
| rle_encoder_boolean::wall_time/128 | -33.7 |
| plain_encoder_boolean::wall_time/32 | -38.6 |
| decompress_gzip::wall_time/float | -40.3 |
| compress_gzip::wall_time/float | -42.2 |
| rle_decoder_boolean::wall_time/128 | -46.3 |
| rle_decoder_boolean::wall_time/64 | -46.3 |
| rle_decoder_boolean::wall_time/32 | -46.4 |
| rle_decoder_boolean::wall_time/16 | -46.4 |
| plain_encoder_boolean::wall_time/64 | -46.6 |
| compress_snappy::wall_time/float | -47.2 |
| decompress_snappy::wall_time/float | -48.5 |
| decompress_lz4::wall_time/float | -48.5 |
| rle_decoder_boolean::wall_time/8 | -48.9 |
| compress_zstd::wall_time/float | -49.3 |
| plain_encoder_boolean::wall_time/128 | -53.6 |
| compress_lz4::wall_time/float | -65.1 |
| decompress_zstd::wall_time/binary | -74.9 |
| compress_lz4::wall_time/boolean | -77.1 |
| compress_brotli::wall_time/binary | -77.7 |
| compress_zstd::wall_time/boolean | -79.0 |
| decompress_brotli::wall_time/binary | -80.7 |
| decompress_lz4::wall_time/boolean | -91.0 |
| compress_brotli::wall_time/boolean | -93.0 |
| compress_snappy::wall_time/boolean | -94.7 |
| decompress_zstd::wall_time/boolean | -95.6 |
| decompress_gzip::wall_time/boolean | -96.2 |
| compress_gzip::wall_time/boolean | -96.6 |
| decompress_snappy::wall_time/boolean | -97.2 |
| decompress_brotli::wall_time/boolean | -98.4 |
| compress_lz4::wall_time/int64 | -98.9 |
| compress_zstd::wall_time/int64 | -99.2 |
| decompress_zstd::wall_time/int64 | -99.3 |
| decompress_lz4::wall_time/int64 | -99.4 |
| decompress_gzip::wall_time/int64 | -99.8 |
| decompress_brotli::wall_time/int64 | -99.8 |
| decompress_snappy::wall_time/int64 | -99.9 |
| compress_brotli::wall_time/int64 | -99.9 |
| compress_gzip::wall_time/int64 | -99.9 |
| compress_snappy::wall_time/int64 | -100.0 |
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r541456929
##########
File path: .github/workflows/rust.yml
##########
@@ -50,7 +50,10 @@ jobs:
strategy:
fail-fast: false
matrix:
- rust: [nightly-2020-11-24]
Review comment:
changes reverted, I will probably need help getting the small CI parts fixed up
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r535404259
##########
File path: rust/parquet/src/encodings/encoding.rs
##########
@@ -130,6 +130,7 @@ impl<T: DataType> PlainEncoder<T> {
}
impl<T: DataType> Encoder<T> for PlainEncoder<T> {
+ #[cold]
Review comment:
that might be helpful context to add as a comment in the source code so it outlasts this PR review
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r541459341
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -327,7 +422,12 @@ pub trait AsBytes {
pub trait SliceAsBytes: Sized {
/// Returns slice of bytes for a slice of this data type.
fn slice_as_bytes(self_: &[Self]) -> &[u8];
- fn slice_as_bytes_mut(self_: &mut [Self]) -> &mut [u8];
+ /// Return the internal representation as a mutable slice
+ ///
+ /// # Safety
+ /// If modified you are _required_ to ensure the internal representation
+ /// is valid and correct for the actual raw data
+ unsafe fn slice_as_bytes_mut(self_: &mut [Self]) -> &mut [u8];
Review comment:
yes, I made this more obviously unsafe given its sneaky type breaking nature, I wanted to highlight a bit more obviously what this was 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao edited a comment on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
jorgecarleitao edited a comment on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-740958408
@alamb benchmarks formatted as a table (from worse to best, statistical insignificant and changes <5% ignored). No judgement, just info:
| benchmark | variation (%) |
|-------------- | -------------- |
| decompress_snappy::wall_time/fixed | 11458.0 |
| compress_snappy::wall_time/fixed | 11265.0 |
| compress_gzip::wall_time/int32 | 6472.1 |
| compress_brotli::wall_time/int32 | 4928.4 |
| decompress_brotli::wall_time/int96 | 4830.6 |
| decompress_snappy::wall_time/int96 | 3491.3 |
| decompress_gzip::wall_time/int96 | 3269.0 |
| decompress_brotli::wall_time/fixed | 2586.1 |
| compress_gzip::wall_time/int96 | 2534.1 |
| decompress_zstd::wall_time/double | 2414.9 |
| compress_snappy::wall_time/int96 | 2146.0 |
| compress_zstd::wall_time/int32 | 1895.5 |
| decompress_zstd::wall_time/int32 | 1585.4 |
| compress_gzip::wall_time/double | 1409.6 |
| decompress_snappy::wall_time/double | 1407.4 |
| compress_brotli::wall_time/int96 | 1344.0 |
| compress_lz4::wall_time/double | 1246.5 |
| decompress_gzip::wall_time/double | 1245.3 |
| decompress_lz4::wall_time/int96 | 1082.6 |
| compress_brotli::wall_time/double | 1051.9 |
| compress_snappy::wall_time/int32 | 983.8 |
| decompress_lz4::wall_time/double | 965.8 |
| compress_snappy::wall_time/double | 856.6 |
| decompress_gzip::wall_time/int32 | 739.0 |
| decompress_brotli::wall_time/int32 | 611.3 |
| decompress_lz4::wall_time/fixed | 593.6 |
| decompress_brotli::wall_time/double | 568.2 |
| compress_zstd::wall_time/double | 563.5 |
| decompress_zstd::wall_time/int96 | 509.0 |
| compress_lz4::wall_time/int96 | 490.8 |
| compress_lz4::wall_time/int32 | 470.8 |
| decompress_gzip::wall_time/fixed | 469.8 |
| compress_zstd::wall_time/int96 | 428.8 |
| decompress_lz4::wall_time/int32 | 426.8 |
| compress_brotli::wall_time/fixed | 377.3 |
| compress_gzip::wall_time/fixed | 257.7 |
| decompress_brotli::wall_time/float | 199.0 |
| decompress_zstd::wall_time/fixed | 196.2 |
| compress_brotli::wall_time/float | 126.2 |
| decompress_zstd::wall_time/float | 121.9 |
| compress_lz4::wall_time/fixed | 108.4 |
| compress_lz4::wall_time/binary | 72.6 |
| decompress_snappy::wall_time/int32 | 38.5 |
| delta_len_byte_arry_decoder_byte_array::wall_time/128 | 38.2 |
| delta_byte_arry_decoder_byte_array::wall_time/64 | 37.9 |
| delta_byte_arry_decoder_byte_array::wall_time/16 | 37.9 |
| delta_byte_arry_decoder_byte_array::wall_time/8 | 37.8 |
| delta_len_byte_arry_decoder_byte_array::wall_time/8 | 37.8 |
| delta_len_byte_arry_decoder_byte_array::wall_time/32 | 37.7 |
| compress_zstd::wall_time/fixed | 37.5 |
| delta_len_byte_arry_decoder_byte_array::wall_time/16 | 37.5 |
| delta_byte_arry_decoder_byte_array::wall_time/32 | 37.5 |
| delta_byte_arry_decoder_byte_array::wall_time/128 | 37.4 |
| delta_len_byte_arry_decoder_byte_array::wall_time/64 | 35.3 |
| plain_encoder_byte_array::wall_time/64 | 31.8 |
| plain_encoder_byte_array::wall_time/128 | 14.4 |
| delta_byte_arry_encoder_byte_array::wall_time/8 | 11.5 |
| delta_bin_packed_encoder_int64::wall_time/64 | 11.2 |
| delta_len_byte_arry_encoder_byte_array::wall_time/8 | 10.5 |
| decompress_snappy::wall_time/binary | 10.4 |
| compress_zstd::wall_time/binary | 9.3 |
| delta_byte_arry_encoder_byte_array::wall_time/32 | 8.4 |
| plain_decoder_boolean::wall_time/64 | 8.2 |
| plain_decoder_boolean::wall_time/32 | 8.1 |
| delta_bin_packed_decoder_int32::wall_time/128 | 7.8 |
| delta_bin_packed_decoder_int64::wall_time/32 | 7.8 |
| delta_bin_packed_encoder_int64::wall_time/16 | 7.8 |
| delta_bin_packed_decoder_int32::wall_time/64 | 7.7 |
| delta_bin_packed_decoder_int64::wall_time/8 | 7.7 |
| delta_bin_packed_decoder_int64::wall_time/16 | 7.7 |
| plain_decoder_boolean::wall_time/8 | 7.7 |
| delta_bin_packed_decoder_int32::wall_time/16 | 7.6 |
| delta_bin_packed_decoder_int32::wall_time/32 | 7.5 |
| delta_bin_packed_decoder_int32::wall_time/8 | 7.5 |
| delta_bin_packed_decoder_int64::wall_time/64 | 7.5 |
| delta_bin_packed_decoder_int64::wall_time/128 | 7.4 |
| plain_decoder_boolean::wall_time/16 | 7.4 |
| delta_len_byte_arry_encoder_byte_array::wall_time/16 | 7.4 |
| plain_decoder_boolean::wall_time/128 | 7.4 |
| delta_byte_arry_encoder_byte_array::wall_time/64 | 6.9 |
| delta_byte_arry_encoder_byte_array::wall_time/16 | 6.8 |
| plain_decoder_float::wall_time/8 | 6.6 |
| decompress_lz4::wall_time/binary | 6.6 |
| plain_encoder_byte_array::wall_time/32 | 5.7 |
| delta_bin_packed_encoder_int64::wall_time/8 | 5.6 |
| delta_bin_packed_encoder_int64::wall_time/32 | 5.4 |
| dict_decoder_int64::wall_time/64 | 5.1 |
| dict_decoder_int64::wall_time/8 | 5.0 |
| plain_encoder_byte_array::wall_time/16 | -5.1 |
| dict_encoder_float::wall_time/8 | -5.5 |
| dict_encoder_byte_array::wall_time/8 | -6.3 |
| dict_encoder_int96::wall_time/64 | -6.4 |
| plain_encoder_int64::wall_time/32 | -7.3 |
| plain_encoder_byte_array::wall_time/8 | -7.8 |
| reader_wall_time/stock-simulated-column | -7.9 |
| dict_encoder_double::wall_time/128 | -9.6 |
| plain_encoder_int32::wall_time/128 | -10.0 |
| plain_encoder_float::wall_time/128 | -10.4 |
| plain_encoder_int96::wall_time/128 | -10.7 |
| plain_encoder_float::wall_time/64 | -10.7 |
| plain_encoder_int32::wall_time/64 | -10.8 |
| plain_encoder_int32::wall_time/32 | -11.5 |
| compress_gzip::wall_time/binary | -11.6 |
| plain_encoder_int96::wall_time/16 | -11.6 |
| plain_encoder_float::wall_time/32 | -11.6 |
| plain_encoder_double::wall_time/32 | -11.7 |
| plain_encoder_int32::wall_time/16 | -11.9 |
| decompress_gzip::wall_time/binary | -12.1 |
| plain_encoder_float::wall_time/8 | -12.1 |
| plain_encoder_float::wall_time/16 | -12.2 |
| plain_encoder_int96::wall_time/8 | -12.2 |
| plain_encoder_int32::wall_time/8 | -12.3 |
| plain_encoder_int96::wall_time/64 | -12.7 |
| plain_encoder_double::wall_time/128 | -12.7 |
| plain_encoder_int96::wall_time/32 | -12.8 |
| plain_encoder_int64::wall_time/16 | -12.9 |
| plain_encoder_int64::wall_time/64 | -13.2 |
| plain_encoder_double::wall_time/64 | -13.3 |
| plain_encoder_double::wall_time/16 | -13.5 |
| plain_encoder_double::wall_time/8 | -14.3 |
| rle_encoder_boolean::wall_time/8 | -14.7 |
| plain_encoder_int64::wall_time/128 | -15.1 |
| plain_encoder_boolean::wall_time/8 | -16.4 |
| plain_encoder_int64::wall_time/8 | -17.2 |
| rle_encoder_boolean::wall_time/16 | -20.5 |
| plain_encoder_boolean::wall_time/16 | -24.7 |
| rle_encoder_boolean::wall_time/32 | -25.5 |
| plain_decoder_float::wall_time/16 | -28.3 |
| rle_encoder_boolean::wall_time/64 | -29.5 |
| rle_encoder_boolean::wall_time/128 | -33.7 |
| plain_encoder_boolean::wall_time/32 | -38.6 |
| decompress_gzip::wall_time/float | -40.3 |
| compress_gzip::wall_time/float | -42.2 |
| rle_decoder_boolean::wall_time/128 | -46.3 |
| rle_decoder_boolean::wall_time/64 | -46.3 |
| rle_decoder_boolean::wall_time/32 | -46.4 |
| rle_decoder_boolean::wall_time/16 | -46.4 |
| plain_encoder_boolean::wall_time/64 | -46.6 |
| compress_snappy::wall_time/float | -47.2 |
| decompress_snappy::wall_time/float | -48.5 |
| decompress_lz4::wall_time/float | -48.5 |
| rle_decoder_boolean::wall_time/8 | -48.9 |
| compress_zstd::wall_time/float | -49.3 |
| plain_encoder_boolean::wall_time/128 | -53.6 |
| compress_lz4::wall_time/float | -65.1 |
| decompress_zstd::wall_time/binary | -74.9 |
| compress_lz4::wall_time/boolean | -77.1 |
| compress_brotli::wall_time/binary | -77.7 |
| compress_zstd::wall_time/boolean | -79.0 |
| decompress_brotli::wall_time/binary | -80.7 |
| decompress_lz4::wall_time/boolean | -91.0 |
| compress_brotli::wall_time/boolean | -93.0 |
| compress_snappy::wall_time/boolean | -94.7 |
| decompress_zstd::wall_time/boolean | -95.6 |
| decompress_gzip::wall_time/boolean | -96.2 |
| compress_gzip::wall_time/boolean | -96.6 |
| decompress_snappy::wall_time/boolean | -97.2 |
| decompress_brotli::wall_time/boolean | -98.4 |
| compress_lz4::wall_time/int64 | -98.9 |
| compress_zstd::wall_time/int64 | -99.2 |
| decompress_zstd::wall_time/int64 | -99.3 |
| decompress_lz4::wall_time/int64 | -99.4 |
| decompress_gzip::wall_time/int64 | -99.8 |
| decompress_brotli::wall_time/int64 | -99.8 |
| decompress_snappy::wall_time/int64 | -99.9 |
| compress_brotli::wall_time/int64 | -99.9 |
| compress_gzip::wall_time/int64 | -99.9 |
| compress_snappy::wall_time/int64 | -100.0 |
(@alamb feel free to delete this comment and add this table to your comment. I did not want to edit your comment)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-743539756
Ok review comments been addressed.
I seperated out the benchmark code from this PR and rebased it to match comment style in git.
I think when CI can be made to function I am good with the PR, I am thinking of following up with a round of performance tuning in compression and delta_fixed_bin
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-745563137
I did one last local test run after merging from master to make sure the tests passed. They did. So :shipit: I'll also send a note over to the dev channel. Again thanks so much for this work @GregBowyer
cc @sunchao @andygrove @jorgecarleitao @vertexclique @jhorstmann @Dandandan
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-743979446
Thanks a lot, @GregBowyer ! I think that this is almost done here.
Two comments:
1. It seems that there are some changes on the submodule `testing` committed to this PR that I am not sure are intended. Could you double check? Typically this happens when `git add -u` is called before `git submodule update`.
2. I have been updating the CI for this PR [here](https://github.com/jorgecarleitao/arrow/tree/GregBowyer-remove-rust-specialization), and in particular [this patch](https://github.com/jorgecarleitao/arrow/commit/ca66d6d945e265dd2c83464bd80ff1dd7d231f7c).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sunchao commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
sunchao commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-729451511
Thanks @GregBowyer for the PR. It's will be awesome if we can get this done. I think before merging this we should get some benchmark results showing the diff before & after. We use to have [some benchmark in the old repo](https://github.com/sunchao/parquet-rs/tree/master/benches) but haven't port them to arrow 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534653225
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -461,21 +553,16 @@ impl AsBytes for str {
}
pub(crate) mod private {
- use super::{AsBytes, Result, ParquetError};
+ use crate::encodings::decoding::PlainDecoderDetails;
+ use crate::util::bit_util::{BitWriter, BitReader};
+ use crate::util::memory::ByteBufferPtr;
- pub type BitIndex = u64;
+ use byteorder::ByteOrder;
+ use std::convert::TryInto;
- #[derive(Copy, Clone)]
- pub enum EncodedValue<'a> {
Review comment:
Performance notes:
This type and its behaviour introduces branch misses that _unfortunately_ impact performance. Since removing this pushes the logic down towards the implementation type `ParquetValueType` it becomes needless (and actually I think more messy code).
As such in pushing the logic down towards the base types this goes away
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534653577
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -492,8 +579,14 @@ pub(crate) mod private {
+ super::SliceAsBytes
+ PartialOrd
{
- /// Return the most primitive version of encoding a given type
- fn encoded(&self) -> EncodedValue<'_>;
+ /// Encode the value directly from a higher level encoder
+ fn encode<W: std::io::Write>(values: &[Self], writer: &mut W, bit_writer: &mut BitWriter) -> Result<()>;
Review comment:
Performance notes:
This is similar to specialisation in that it makes the required behaviours part of the trait implementation for the actual data carrying types in the code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534658141
##########
File path: rust/parquet/src/util/memory.rs
##########
@@ -383,12 +383,10 @@ impl<T: Debug> Display for BufferPtr<T> {
impl<T> Drop for BufferPtr<T> {
fn drop(&mut self) {
- if self.is_mem_tracked()
- && Arc::strong_count(&self.data) == 1
- && Arc::weak_count(&self.data) == 0
- {
- let mc = self.mem_tracker.as_ref().unwrap();
- mc.alloc(-(self.data.capacity() as i64));
+ if let Some(ref mc) = self.mem_tracker {
Review comment:
Performance Notes:
This construction reduces the cost of the branch a little by avoiding checking ref counts if we are not tracking memory sizes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r541457466
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -229,6 +235,82 @@ impl fmt::Display for ByteArray {
}
}
+/// Wrapper type for performance reasons, this represents `FIXED_LEN_BYTE_ARRAY` but in all other
+/// considerations behaves the same as `ByteArray`
+#[repr(transparent)]
+#[derive(Clone, Debug, Default)]
+pub struct FixedLenByteArray(ByteArray);
Review comment:
I copied the above comment in wholesale
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-729809822
Here is a potential contribution to this effort: https://github.com/apache/arrow/pull/8708 (a PR with the bench marks ported -- fyi @GregBowyer ).
To run:
```
cd arrow/rust/parquet
cargo bench
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-732583123
As per benchmarking this with the changes in https://github.com/alamb/arrow/pull/2 this is slower than specialisation (I ran benchmarks a lot as its _very noisy_). I have solutions for the speed in encoding (boolean and int96 are to be solved but shouldn't be hard) I will work on decoding shortly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-730570547
I do not have time to review this, but having tried this myself once (and failed miserably), I am just leaving a big thank you note to @GregBowyer for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534652479
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -229,6 +235,82 @@ impl fmt::Display for ByteArray {
}
}
+/// Wrapper type for performance reasons, this represents `FIXED_LEN_BYTE_ARRAY` but in all other
+/// considerations behaves the same as `ByteArray`
+#[repr(transparent)]
+#[derive(Clone, Debug, Default)]
+pub struct FixedLenByteArray(ByteArray);
Review comment:
Performance review note:
This type is a little unfortunate, without it the compiler generates code that takes quite a big hit on the CPU pipeline. Essentially the previous version stalls awaiting the result of `T::get_physical_type() == Type::FIXED_LEN_BYTE_ARRAY`.
Its debatable if this is wanted, it is out of spec for what parquet documents as its base types, although I feel that enough code paths in the rust (and potentially the C++) versions warrant this.
With this wrapper type the compiler generates more targetted code paths matching the higher level logical types, removing the data-hazard from all decoding and encoding paths.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-741052690
Thank you @jorgecarleitao -- that table is much better than my hand wavy analysis.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-739499760
@GregBowyer -- I am starting to go through this PR again, but it will definitely take me some time. I plan initially yo see if I can reproduce the performance numbers.
This looks like an epic PR and is really important. Thank you for all your efforts
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534657801
##########
File path: rust/parquet/src/util/bit_util.rs
##########
@@ -655,15 +647,14 @@ impl BitReader {
///
/// Returns `None` if the number of bytes there's not enough bytes in the stream.
/// `Some` otherwise.
- #[inline]
+ //#[inline]
Review comment:
NIT: This should either be a true `inline` or removed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-744883852
I am ok merging as is and putting a fixup in later.
I pushed commits that should not alter the testing submodule
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534653150
##########
File path: .github/workflows/rust.yml
##########
@@ -50,7 +50,10 @@ jobs:
strategy:
fail-fast: false
matrix:
- rust: [nightly-2020-11-24]
Review comment:
I'll submit a PR on your branch
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-745563594
I filed https://issues.apache.org/jira/browse/ARROW-10931 to track follow on work to improve compressors
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534610365
##########
File path: .github/workflows/rust.yml
##########
@@ -50,7 +50,10 @@ jobs:
strategy:
fail-fast: false
matrix:
- rust: [nightly-2020-11-24]
Review comment:
I know this is still in-flight, but the better strategy here is to run the Parquet tests with `cargo +stable test` as we also add the stable toolchain to compile arrow with. You might need to rebase to see that in the dev scripts, as this change was added last week.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534655770
##########
File path: rust/parquet/src/encodings/decoding.rs
##########
@@ -384,36 +278,18 @@ impl<T: DataType> Decoder<T> for DictDecoder<T> {
/// See [`RleValueEncoder`](crate::encoding::RleValueEncoder) for more information.
pub struct RleValueDecoder<T: DataType> {
values_left: usize,
- decoder: Option<RleDecoder>,
+ decoder: RleDecoder,
Review comment:
Performance Notes:
Small and minor this gives a 5% boost on performance as the RLE encoder can just be established before use without needing to constantly check if it has been initialised for every value.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-737177343
> I have been working on this w.r.t performance, I think I have most parts performing better than the original. I am running off clean benchmarks right now to validate.
I am very excited to see it. Thank you so much @GregBowyer
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-740944598
FYI @sunchao @nevi-me @jorgecarleitao -- I spent quite a while verifying the performance of this approach and reviewing the code (treatise is here: https://github.com/apache/arrow/pull/8698#pullrequestreview-547571376, sorry for the length). I think we should accept these changes and run with stable rust
Thanks again @GregBowyer
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534657321
##########
File path: rust/parquet/src/encodings/encoding.rs
##########
@@ -561,6 +545,7 @@ impl<T: DataType> DeltaBitPackEncoder<T> {
}
// Write current delta buffer (<= 'block size' values) into bit writer
+ #[inline(never)]
Review comment:
Performance notes:
In the code there are a few places where the inlines have been shifted about. From `perf` and `vtune` its pretty obvious that the code is a bit overly inlined. This leads to places where we include a lot of code at a given compilation site that is either seldom used, or is just rather large.
This impacts the instruction cache size and has a pronounced impact on performance. As a rule I have removed inlines that show up as causing front-end stalls in various tools, or are likly to be quite large (general size, large loops etc).
For some parts the inlines are removed letting the compiler do its thing, in a few select places inlining is disabled since it swells the size of the resulting code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] GregBowyer commented on a change in pull request #8698: Remove rust specialization
Posted by GitBox <gi...@apache.org>.
GregBowyer commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r525735281
##########
File path: rust/parquet/src/data_type.rs
##########
@@ -95,6 +96,12 @@ impl From<Vec<u32>> for Int96 {
}
}
+impl fmt::Display for Int96 {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ write!(f, "{:?}", self.data())
Review comment:
Minor: Maybe as a followup this should print the number rather than its raw bytes?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-744471710
A small update: the CI runs successfully for everything except Clippy (issue unrelated to this PR) π ππ
I fielded #8909 that addresses the Clippy warning.
I see two options:
1. incorporate the patch with the CI changes mentioned above and merge this PR with those changes
2. merge this as is now and PR the change to the CI separately
Let me know what you prefer, @GregBowyer π
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io commented on pull request #8698: ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#issuecomment-743562629
# [Codecov](https://codecov.io/gh/apache/arrow/pull/8698?src=pr&el=h1) Report
> Merging [#8698](https://codecov.io/gh/apache/arrow/pull/8698?src=pr&el=desc) (f8f9749) into [master](https://codecov.io/gh/apache/arrow/commit/edff65dfe73d3380a211b678f3760a60a66e5544?el=desc) (edff65d) will **increase** coverage by `7.47%`.
> The diff coverage is `84.08%`.
[](https://codecov.io/gh/apache/arrow/pull/8698?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8698 +/- ##
==========================================
+ Coverage 76.81% 84.28% +7.47%
==========================================
Files 181 194 +13
Lines 40985 47951 +6966
==========================================
+ Hits 31483 40417 +8934
+ Misses 9502 7534 -1968
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/8698?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [rust/parquet/src/record/triplet.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9yZWNvcmQvdHJpcGxldC5ycw==) | `93.24% <0.00%> (ΓΈ)` | |
| [rust/parquet/src/data\_type.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9kYXRhX3R5cGUucnM=) | `80.29% <75.64%> (-4.32%)` | :arrow_down: |
| [rust/parquet/src/encodings/decoding.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9lbmNvZGluZ3MvZGVjb2RpbmcucnM=) | `92.49% <86.91%> (+1.03%)` | :arrow_up: |
| [rust/parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9lbmNvZGluZ3MvZW5jb2RpbmcucnM=) | `95.59% <91.02%> (+2.41%)` | :arrow_up: |
| [rust/parquet/src/encodings/rle.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9lbmNvZGluZ3MvcmxlLnJz) | `93.13% <93.33%> (+0.10%)` | :arrow_up: |
| [rust/parquet/src/arrow/arrow\_reader.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9hcnJvdy9hcnJvd19yZWFkZXIucnM=) | `90.58% <100.00%> (ΓΈ)` | |
| [rust/parquet/src/arrow/converter.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9hcnJvdy9jb252ZXJ0ZXIucnM=) | `62.96% <100.00%> (+7.40%)` | :arrow_up: |
| [rust/parquet/src/column/writer.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9jb2x1bW4vd3JpdGVyLnJz) | `94.02% <100.00%> (+0.12%)` | :arrow_up: |
| [rust/parquet/src/file/statistics.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9maWxlL3N0YXRpc3RpY3MucnM=) | `93.80% <100.00%> (-0.60%)` | :arrow_down: |
| [rust/parquet/src/util/bit\_util.rs](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy91dGlsL2JpdF91dGlsLnJz) | `92.39% <100.00%> (+0.14%)` | :arrow_up: |
| ... and [102 more](https://codecov.io/gh/apache/arrow/pull/8698/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/8698?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/8698?src=pr&el=footer). Last update [edff65d...f8f9749](https://codecov.io/gh/apache/arrow/pull/8698?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #8698: ARROW-10636: [Rust][Parquet] Remove rust specialization
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8698:
URL: https://github.com/apache/arrow/pull/8698#discussion_r534655644
##########
File path: .github/workflows/rust.yml
##########
@@ -50,7 +50,10 @@ jobs:
strategy:
fail-fast: false
matrix:
- rust: [nightly-2020-11-24]
Review comment:
I think the safest option is to revert the CI changes, then we can add parquet to stable tests separately in #8821
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org