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 2021/03/02 11:32:38 UTC
[GitHub] [arrow] nevi-me opened a new pull request #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
nevi-me opened a new pull request #9612:
URL: https://github.com/apache/arrow/pull/9612
Populate LogicalType when converting from Arrow schema to Parquet schema.
This is on top of #9592
----------------------------------------------------------------
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 a change in pull request #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#discussion_r594048206
##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -364,32 +385,51 @@ fn arrow_to_parquet_type(field: &Field) -> Result<Type> {
DataType::Float64 => Type::primitive_type_builder(name, PhysicalType::DOUBLE)
.with_repetition(repetition)
.build(),
- DataType::Timestamp(time_unit, _) => {
+ DataType::Timestamp(time_unit, zone) => {
Type::primitive_type_builder(name, PhysicalType::INT64)
- .with_converted_type(match time_unit {
- TimeUnit::Second => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Millisecond => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Microsecond => ConvertedType::TIMESTAMP_MICROS,
- TimeUnit::Nanosecond => ConvertedType::TIMESTAMP_MICROS,
- })
+ .with_logical_type(Some(LogicalType::TIMESTAMP(TimestampType {
+ is_adjusted_to_u_t_c: matches!(zone, Some(z) if z.as_str() == "UTC"),
Review comment:
I think we can perhaps follow Arrow C++ and always set it to true whenever the timezone info is set, and normalize the timestamp value to UTC when converting to Parquet. Please see a previous discussion [here](https://github.com/apache/arrow/pull/4421#issuecomment-498413899) and the related C++ code [here](https://github.com/apache/arrow/blob/master/cpp/src/parquet/arrow/schema.cc#L138).
----------------------------------------------------------------
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 pull request #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#issuecomment-796357194
@sunchao may you please have a look at this when you get a chance, thanks :)
----------------------------------------------------------------
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#issuecomment-791724823
The clippy error seems unrelated to this PR:
```
error: unnecessary parentheses around `for` iterator expression
--> datafusion/src/physical_plan/merge.rs:124:31
|
124 | for part_i in (0..input_partitions) {
| ^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
|
= note: `-D unused-parens` implied by `-D warnings`
error: aborting due to previous error
```
I also saw it on @Dandandan 's PR. https://github.com/apache/arrow/pull/9639
----------------------------------------------------------------
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 a change in pull request #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#discussion_r593855164
##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -583,48 +626,109 @@ impl ParquetTypeConverter<'_> {
}
fn from_int32(&self) -> Result<DataType> {
- match self.schema.get_basic_info().converted_type() {
- ConvertedType::NONE => Ok(DataType::Int32),
- ConvertedType::UINT_8 => Ok(DataType::UInt8),
- ConvertedType::UINT_16 => Ok(DataType::UInt16),
- ConvertedType::UINT_32 => Ok(DataType::UInt32),
- ConvertedType::INT_8 => Ok(DataType::Int8),
- ConvertedType::INT_16 => Ok(DataType::Int16),
- ConvertedType::INT_32 => Ok(DataType::Int32),
- ConvertedType::DATE => Ok(DataType::Date32),
- ConvertedType::TIME_MILLIS => Ok(DataType::Time32(TimeUnit::Millisecond)),
- ConvertedType::DECIMAL => Ok(self.to_decimal()),
- other => Err(ArrowError(format!(
- "Unable to convert parquet INT32 logical type {}",
- other
+ match (
+ self.schema.get_basic_info().logical_type(),
Review comment:
nit: perhaps we can first "merge" the logical and converted type into a logical type and then do the conversion, to avoid some of the code duplications. In the case when logical type is not present, we can always convert the converted type into a logical type while losing some information.
We can do this as a follow-up though.
##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -673,6 +784,7 @@ impl ParquetTypeConverter<'_> {
/// This function takes care of logical type and repetition.
fn to_group_type(&self) -> Result<Option<DataType>> {
if self.is_repeated() {
+ dbg!(self.schema.get_basic_info());
Review comment:
nit: remove?
##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -364,32 +385,51 @@ fn arrow_to_parquet_type(field: &Field) -> Result<Type> {
DataType::Float64 => Type::primitive_type_builder(name, PhysicalType::DOUBLE)
.with_repetition(repetition)
.build(),
- DataType::Timestamp(time_unit, _) => {
+ DataType::Timestamp(time_unit, zone) => {
Type::primitive_type_builder(name, PhysicalType::INT64)
- .with_converted_type(match time_unit {
- TimeUnit::Second => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Millisecond => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Microsecond => ConvertedType::TIMESTAMP_MICROS,
- TimeUnit::Nanosecond => ConvertedType::TIMESTAMP_MICROS,
- })
+ .with_logical_type(Some(LogicalType::TIMESTAMP(TimestampType {
+ is_adjusted_to_u_t_c: matches!(zone, Some(z) if z.as_str() == "UTC"),
Review comment:
Hmm this means we'll lose the timezone info right? as `is_adjusted_to_u_t_c` means using local timezone.
##########
File path: rust/parquet/src/schema/types.rs
##########
@@ -419,6 +439,79 @@ impl<'a> PrimitiveTypeBuilder<'a> {
precision: self.precision,
})
}
+
+ #[inline]
+ fn test_decimal_precision_scale(&self) -> Result<()> {
Review comment:
nit: maybe rename `test` to `check` or something else. It's easy to misread this as a test case.
##########
File path: rust/parquet/src/schema/types.rs
##########
@@ -479,13 +572,17 @@ impl<'a> GroupTypeBuilder<'a> {
/// Creates a new `GroupType` instance from the gathered attributes.
pub fn build(self) -> Result<Type> {
- let basic_info = BasicTypeInfo {
+ let mut basic_info = BasicTypeInfo {
name: String::from(self.name),
repetition: self.repetition,
converted_type: self.converted_type,
- logical_type: self.logical_type,
+ logical_type: self.logical_type.clone(),
id: self.id,
};
+ // Populate the converted type if only the logical type is populated
Review comment:
we might need more tests for the case when logical type is set.
----------------------------------------------------------------
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#discussion_r593995672
##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -364,32 +385,51 @@ fn arrow_to_parquet_type(field: &Field) -> Result<Type> {
DataType::Float64 => Type::primitive_type_builder(name, PhysicalType::DOUBLE)
.with_repetition(repetition)
.build(),
- DataType::Timestamp(time_unit, _) => {
+ DataType::Timestamp(time_unit, zone) => {
Type::primitive_type_builder(name, PhysicalType::INT64)
- .with_converted_type(match time_unit {
- TimeUnit::Second => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Millisecond => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Microsecond => ConvertedType::TIMESTAMP_MICROS,
- TimeUnit::Nanosecond => ConvertedType::TIMESTAMP_MICROS,
- })
+ .with_logical_type(Some(LogicalType::TIMESTAMP(TimestampType {
+ is_adjusted_to_u_t_c: matches!(zone, Some(z) if z.as_str() == "UTC"),
Review comment:
Yeah, I think that my logic is faulty. Reading https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#instant-semantics-timestamps-normalized-to-utc again, I now see that it says that `is_adjusted_to_u_t_c = true` is if we actually adjust the timezone.
So, I think it's safer to use `false` always, as we don't adjust any timezones?
----------------------------------------------------------------
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#issuecomment-791725587
Sorry I did not mean to close this PR
----------------------------------------------------------------
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9612:
URL: https://github.com/apache/arrow/pull/9612
----------------------------------------------------------------
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
sunchao commented on pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#issuecomment-798606562
@nevi-me sorry missed this one - will take a look 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] nevi-me closed pull request #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
nevi-me closed pull request #9612:
URL: https://github.com/apache/arrow/pull/9612
--
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 a change in pull request #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#discussion_r594528419
##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -364,32 +385,51 @@ fn arrow_to_parquet_type(field: &Field) -> Result<Type> {
DataType::Float64 => Type::primitive_type_builder(name, PhysicalType::DOUBLE)
.with_repetition(repetition)
.build(),
- DataType::Timestamp(time_unit, _) => {
+ DataType::Timestamp(time_unit, zone) => {
Type::primitive_type_builder(name, PhysicalType::INT64)
- .with_converted_type(match time_unit {
- TimeUnit::Second => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Millisecond => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Microsecond => ConvertedType::TIMESTAMP_MICROS,
- TimeUnit::Nanosecond => ConvertedType::TIMESTAMP_MICROS,
- })
+ .with_logical_type(Some(LogicalType::TIMESTAMP(TimestampType {
+ is_adjusted_to_u_t_c: matches!(zone, Some(z) if z.as_str() == "UTC"),
Review comment:
I see. In that case we don't need to do the normalization part right? Yes +1 on setting `is_adjusted_to_u_t_c = true` whenever there's a timezone. We should also handle the case when the timezone string is empty the same way as it is not set.
----------------------------------------------------------------
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#discussion_r594478176
##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -364,32 +385,51 @@ fn arrow_to_parquet_type(field: &Field) -> Result<Type> {
DataType::Float64 => Type::primitive_type_builder(name, PhysicalType::DOUBLE)
.with_repetition(repetition)
.build(),
- DataType::Timestamp(time_unit, _) => {
+ DataType::Timestamp(time_unit, zone) => {
Type::primitive_type_builder(name, PhysicalType::INT64)
- .with_converted_type(match time_unit {
- TimeUnit::Second => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Millisecond => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Microsecond => ConvertedType::TIMESTAMP_MICROS,
- TimeUnit::Nanosecond => ConvertedType::TIMESTAMP_MICROS,
- })
+ .with_logical_type(Some(LogicalType::TIMESTAMP(TimestampType {
+ is_adjusted_to_u_t_c: matches!(zone, Some(z) if z.as_str() == "UTC"),
Review comment:
I've read the thread, and my interpretation is this:
- What I was initially doing:
```
no timezone: false
"UTC": true
other timezone: false
```
- What's done in C++
```
no timezone: false
"UTC": true
other timezone: true
```
> and normalize the timestamp value to UTC when converting to Parquet
Arrow timestamps are always in UTC, such that any non-UTC timezone is for display purposes only (e.g. if we want to print formatted timestamps).
So, we shouldn't need to normalise timezones as they'll always be adjusted to UTC.
My initial approach was to set `is_adjusted_to_u_t_c = true` whenever there's a timezone, but I second-guessed myself while working on this code. I had looked at the C++ implementation, but somehow interpreted the `true` value to only be set if `timezone = UTC`.
@sunchao are you fine with setting `is_adjusted_to_u_t_c = true` whenever there's a timezone?
----------------------------------------------------------------
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#issuecomment-788753290
https://issues.apache.org/jira/browse/ARROW-11824
----------------------------------------------------------------
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 pull request #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#issuecomment-802155822
Hey @sunchao I've updated this PR. I can't add some tests for logical type conversion as I need to complete #9705 for them. I'll extend the arrow schema conversion test coverage in that PR instead.
PTAL when you can :smile:
--
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#discussion_r593995897
##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -583,48 +626,109 @@ impl ParquetTypeConverter<'_> {
}
fn from_int32(&self) -> Result<DataType> {
- match self.schema.get_basic_info().converted_type() {
- ConvertedType::NONE => Ok(DataType::Int32),
- ConvertedType::UINT_8 => Ok(DataType::UInt8),
- ConvertedType::UINT_16 => Ok(DataType::UInt16),
- ConvertedType::UINT_32 => Ok(DataType::UInt32),
- ConvertedType::INT_8 => Ok(DataType::Int8),
- ConvertedType::INT_16 => Ok(DataType::Int16),
- ConvertedType::INT_32 => Ok(DataType::Int32),
- ConvertedType::DATE => Ok(DataType::Date32),
- ConvertedType::TIME_MILLIS => Ok(DataType::Time32(TimeUnit::Millisecond)),
- ConvertedType::DECIMAL => Ok(self.to_decimal()),
- other => Err(ArrowError(format!(
- "Unable to convert parquet INT32 logical type {}",
- other
+ match (
+ self.schema.get_basic_info().logical_type(),
Review comment:
Yeah, I can do it as a follow-up when I've completed the overall 2.6.0 type support
----------------------------------------------------------------
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#discussion_r597113861
##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -364,32 +385,51 @@ fn arrow_to_parquet_type(field: &Field) -> Result<Type> {
DataType::Float64 => Type::primitive_type_builder(name, PhysicalType::DOUBLE)
.with_repetition(repetition)
.build(),
- DataType::Timestamp(time_unit, _) => {
+ DataType::Timestamp(time_unit, zone) => {
Type::primitive_type_builder(name, PhysicalType::INT64)
- .with_converted_type(match time_unit {
- TimeUnit::Second => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Millisecond => ConvertedType::TIMESTAMP_MILLIS,
- TimeUnit::Microsecond => ConvertedType::TIMESTAMP_MICROS,
- TimeUnit::Nanosecond => ConvertedType::TIMESTAMP_MICROS,
- })
+ .with_logical_type(Some(LogicalType::TIMESTAMP(TimestampType {
+ is_adjusted_to_u_t_c: matches!(zone, Some(z) if z.as_str() == "UTC"),
Review comment:
Yes, we don't need the normalisation. I've modified the code, to check if a timezone string is not empty
--
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 #9612: ARROW-11824: [Rust] [Parquet] Use logical types in Arrow schema conversion
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9612:
URL: https://github.com/apache/arrow/pull/9612#discussion_r597113266
##########
File path: rust/parquet/src/schema/types.rs
##########
@@ -479,13 +572,17 @@ impl<'a> GroupTypeBuilder<'a> {
/// Creates a new `GroupType` instance from the gathered attributes.
pub fn build(self) -> Result<Type> {
- let basic_info = BasicTypeInfo {
+ let mut basic_info = BasicTypeInfo {
name: String::from(self.name),
repetition: self.repetition,
converted_type: self.converted_type,
- logical_type: self.logical_type,
+ logical_type: self.logical_type.clone(),
id: self.id,
};
+ // Populate the converted type if only the logical type is populated
Review comment:
I added a test for the group type (modified an existing one), but for primitive types, I need the schema printer + parser. So, I'll increase the test coverage as part of #9705
--
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