You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "dsgibbons (via GitHub)" <gi...@apache.org> on 2024/08/27 12:53:42 UTC
[PR] feat(parquet)!: coerce_types flag for date64 (#1938) [arrow-rs]
dsgibbons opened a new pull request, #6313:
URL: https://github.com/apache/arrow-rs/pull/6313
# Which issue does this PR close?
Relates to #1938, which will be closed by future PRs.
# What changes are included in this PR?
This PR introduces the `coerce_types` flag for `WriterProperties`. To start, I've only addressed the `Date64` case. The desired behaviour for `Date64` is captured in #1938. I've added tests to ensure that `Date64` is handled correctly when `coerce_types=false` and `coerce_types=true`. I've also added some testing around `Date32` to ensure I haven't accidentally broken anything there.
I've deliberately avoided the other types mentioned in #1938 because I wanted to make sure we are happy with how `coerce_types` works for `Date64` first. Once accepted, I'll raise PRs for the remaining types to finally close out #1938.
One thing missing from this PR is validation on `Date64`. The [C++ implementation](https://github.com/apache/arrow/blob/bda727f9fe56e0abd4fa2770d7175c9074306573/cpp/src/arrow/array/validate.cc#L172-L190) has a `full_validation` option that checks that all `Date64` lie on a date boundary (i.e., a multiple of 1000 * 60 * 60 * 24). I've started work on adding this in arrow-rs and intend to raise a separate PR for enabling `Date64` validation in the `arrow-array` crate.
# Are there any user-facing changes?
Breaking changes:
- `arrow_to_parquet_schema(schema: &Schema, coerce_types: bool)` - I wasn't sure how else we could propagate the `coerce_types` option down to the parquet schema.
- `arrow_to_parquet_schema_with_root(schema: &Schema, root: &str, coerce_types: bool)` - same as above.
- `coerce_types` flag in `WriterProperties` (not a major issue as typically created via `WriterPropertiesBuilder`.
User facing changes:
- The ability to read/write parquet files with the native `Date64` logical type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "dsgibbons (via GitHub)" <gi...@apache.org>.
dsgibbons commented on code in PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#discussion_r1779172902
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -1577,7 +1589,7 @@ mod tests {
false,
)];
let arrow_schema = Schema::new(arrow_fields);
- let converted_arrow_schema = arrow_to_parquet_schema(&arrow_schema);
+ let converted_arrow_schema = arrow_to_parquet_schema(&arrow_schema, true);
Review Comment:
The original unit test implicitly used the `coerce_types` behaviour, so I retained it with the new API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2359994984
Thanks for taking this on @dsgibbons. I may be confused, but it appears that the approach you use for the non-coerced case is to write `INT64` with a `DATE` annotation to the Parquet file. The problem is that the Parquet spec does not allow this ([ref](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#date)). I believe the approach called for in #1938 is to write un-annotated `INT64`, and rely on the encoded arrow schema to know how to interpret the column. For instance, in `arrow/schema/mod.rs`, perhaps the logic should be more like
```rust
DataType::Date64 => {
if coerce_types {
Type::primitive_type_builder(name, PhysicalType::INT32)
.with_logical_type(Some(LogicalType::Date))
.with_repetition(repetition)
.with_id(id)
.build()
} else {
Type::primitive_type_builder(name, PhysicalType::INT64)
.with_repetition(repetition)
.with_id(id)
.build()
}
},
```
I don't think the write side is too far off.
On the read side, I think you'll still have to account for the `INT32(DATE)->Date64` conversion for the case of coerced data. It seems to me like you've removed the code to handle this case, but again I may be confused.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "dsgibbons (via GitHub)" <gi...@apache.org>.
dsgibbons commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2347127062
> I wonder if you need this feature yourself or if you are trying to help workdown the arrow-rs backlog
The latter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "dsgibbons (via GitHub)" <gi...@apache.org>.
dsgibbons commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2344755712
@tustvold I know this isn't high priority, but just bumping this so I can get some feedback before working on accompanying PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#discussion_r1779167027
##########
parquet/src/arrow/arrow_reader/statistics.rs:
##########
@@ -371,8 +371,7 @@ macro_rules! get_statistics {
[<$stat_type_prefix Int32StatsIterator>]::new($iterator).map(|x| x.copied()),
))),
DataType::Date64 => Ok(Arc::new(Date64Array::from_iter(
- [<$stat_type_prefix Int32StatsIterator>]::new($iterator)
- .map(|x| x.map(|x| i64::from(*x) * 24 * 60 * 60 * 1000)),
+ [<$stat_type_prefix Int64StatsIterator>]::new($iterator).map(|x| x.copied()),
Review Comment:
The `Statistics` struct keeps the parquet physical type info, but I'm not sure how to get at it here. 🤔
We have to know whether the physical type is INT32 or INT64, and if INT32 we still have to do the scaling of days to milliseconds (and use the `Int32StatsIterator`). Perhaps add physical type to the `StatisticsConverter` struct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2313107750
Flagging @tustvold who filed the original issue https://github.com/apache/arrow-rs/issues/1938
Thank you @dsgibbons
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "dsgibbons (via GitHub)" <gi...@apache.org>.
dsgibbons commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2363746738
Thank you for taking the time to look at this @etseidl. I'm still new to the project so I have plenty to learn.
From #1938:
> If not coerce_types, write as Int64 and embed logical type in arrow schema only.
I think I interpreted this as the Parquet `LogicalType`. I hadn't seen that [ref](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#date) before.
> I believe the approach called for in https://github.com/apache/arrow-rs/issues/1938 is to write un-annotated INT64, and rely on the encoded arrow schema to know how to interpret the column.
So if we can't embed the fact that the field refers to a date in the Parquet `LogicalType`, do we provide additional type information during/after reading to interpret `INT64` columns as `Date64`? Is this what was meant by "embed logical type in arrow schema only" from #1938?
I thought that all type information was inferred from the Parquet file. Hence why I removed the `INT32(DATE)->Date64` code, as I didn't think there would be any way to know whether `INT32(DATE)` was coerced or not. Could you please give an example of how a reader would use an arrow schema to correctly interpret the columns?
On another note, are you OK with the breaking change introduced by: `arrow_to_parquet_schema(schema: &Schema, coerce_types: bool)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2359323233
I am depressed about the large review backlog in this crate. We are looking for more help from the community reviewing PRs -- see https://github.com/apache/arrow-rs/issues/6418 for more
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "dsgibbons (via GitHub)" <gi...@apache.org>.
dsgibbons commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2378485555
I finally got a chance to update this. I think this is correct now. Fortunately, this PR is a bit shorter now because some of the previous unit tests no longer apply.
I'm not too sure what to do with `parquet/src/arrow/arrow_reader/statistics.rs`. I made an attempt at this previously, but I'm not sure if what I've done is correct. Any idea what is required here @etseidl?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2346891106
@dsgibbons do you have a usecase for this feature? There is a workaround which is for the user to explicitly call `cast` on any column types that are not supported
I wonder if you need this feature yourself or if you are trying to help workdown the arrow-rs backlog
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2364062347
> Thank you for taking the time to look at this @etseidl. I'm still new to the project so I have plenty to learn.
👋 Welcome! I'm pretty new here too, and still learning as well 😄.
> > I believe the approach called for in #1938 is to write un-annotated INT64, and rely on the encoded arrow schema to know how to interpret the column.
>
> So if we can't embed the fact that the field refers to a date in the Parquet `LogicalType`, do we provide additional type information during/after reading to interpret `INT64` columns as `Date64`? Is this what was meant by "embed logical type in arrow schema only" from #1938?
Yes. The Parquet format allows for key-value pairs in the metadata. What the arrow writers will do for certain types that aren't representable in Parquet (most of them time based), is to embed a base64 encoded _arrow_ schema (which is serialized with flatbuffers) in one of these key/value pairs, with the key being `ARROW:schema`. Take a look at the code [here](https://github.com/apache/arrow-rs/blob/4683c200f457e7f95a22ed8577c48df8997eadaa/parquet/src/arrow/schema/mod.rs#L143), and trace backwards to see how it's used.
My hope is that the arrow schema will preserve the fact that in arrow it was a `Date64` with no further changes (i.e. if `coerce==true` then do what is currently done, if `false` then don't scale the value and write as `INT64` with no Parquet LogicalType). On the read side, I'm hoping the arrow type will still be `Date64`, and then you just need to account for the case where the Parquet type is `INT64` (the `INT32` case is already there since coercing is the current behavior).
Did that make sense? Hopefully someone with more arrow-side experience will correct me if I've messed up some details.
> On another note, are you OK with the breaking change introduced by: `arrow_to_parquet_schema(schema: &Schema, coerce_types: bool)`?
Breaking changes are ok, but with the caveat that they're only allowed in major releases (with the next 54.0.0 release slated for December). IIUC what will happen is once this is PR ready for merge, it will be changed to "Draft" until development on 54.0.0 commences. I'll take another look and see if there's a way to avoid breaking the API. Perhaps new functions that accept the `coerce` flag, and the existing ones call the new ones passing `true`. I don't know offhand how many types are impacted, and whether they all currently behave as if coerce is true, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "dsgibbons (via GitHub)" <gi...@apache.org>.
dsgibbons commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2364193539
Makes sense. I didn't know about those `ARROW:schema` KV pairs. That helps a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#discussion_r1779132562
##########
parquet/src/file/properties.rs:
##########
@@ -279,6 +282,13 @@ impl WriterProperties {
self.statistics_truncate_length
}
+ /// Returns `coerce_types` boolean
+ ///
+ /// `true` if type coercion enabled.
Review Comment:
Might want to add some words about what type coercion does.
##########
parquet/src/arrow/array_reader/primitive_array.rs:
##########
@@ -208,10 +208,10 @@ where
// As there is not always a 1:1 mapping between Arrow and Parquet, there
// are datatypes which we must convert explicitly.
// These are:
- // - date64: we should cast int32 to date32, then date32 to date64.
- // - decimal: cast in32 to decimal, int64 to decimal
+ // - date64: cast int32 to date32, then date32 to date64.
+ // - decimal: cast int32 to decimal, int64 to decimal
let array = match target_type {
- ArrowType::Date64 => {
+ ArrowType::Date64 if *(array.data_type()) == ArrowType::Int32 => {
Review Comment:
❤️
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -1577,7 +1589,7 @@ mod tests {
false,
)];
let arrow_schema = Schema::new(arrow_fields);
- let converted_arrow_schema = arrow_to_parquet_schema(&arrow_schema);
+ let converted_arrow_schema = arrow_to_parquet_schema(&arrow_schema, true);
Review Comment:
Any reason this is set to `true`?
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -1851,7 +1863,7 @@ mod tests {
// don't pass metadata so field ids are read from Parquet and not from serialized Arrow schema
let arrow_schema = crate::arrow::parquet_to_arrow_schema(&schema_descriptor, None)?;
- let parq_schema_descr = crate::arrow::arrow_to_parquet_schema(&arrow_schema)?;
+ let parq_schema_descr = crate::arrow::arrow_to_parquet_schema(&arrow_schema, true)?;
Review Comment:
same 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2347211593
> > I wonder if you need this feature yourself or if you are trying to help workdown the arrow-rs backlog
>
> The latter
Thank you. In that case I will let @tustvold comment as he filed the original ticket and I have no additional context
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet)!: coerce_types flag for date64 [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6313:
URL: https://github.com/apache/arrow-rs/pull/6313#issuecomment-2347387737
IIRC the request came from people coming from arrow2 which did something similar. I'm afraid I've lost most context on this and don't have time at the moment to review, but I'm sure one of the other maintainers will be able to advise on a path forward
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org