You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/12/05 22:09:30 UTC
[PR] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
alamb opened a new pull request, #6840:
URL: https://github.com/apache/arrow-rs/pull/6840
# Which issue does this PR close?
- Part of https://github.com/apache/arrow-rs/issues/1938
# Rationale for this change
While working on the upgrade to arrow 54 in DataFusion https://github.com/apache/datafusion/pull/13663 I found it was not clear what the `coerce_types` argument in `arrow_to_parquet_schema` meant or what value to pick to keep the existing behavior.
I had to go digging on https://github.com/apache/arrow-rs/pull/6313 to find what the default / old behavior was (`true` or `false`)
Turns out the answer is
```rust
let schema_desc = arrow_to_parquet_schema(&schema, false)?;
```
It is clearly documented on `WriterProperties` but not on the lower level apis
Thus I would like to propose making a new API for converting schemas where the defaults are clearer and deprecate `arrow_to_parquet_schema` (and revert the unnecessary breaking API change)
# What changes are included in this PR?
1. Add `ArrowToParquetSchemaConverter`,
2. Lots of comments / examples
3. Revert the changes to `arrow_to_parquet_schema` made in https://github.com/apache/arrow-rs/pull/6313
4. Deprecate `arrow_to_parquet_schema` with a clear message about what to use instread
# Are there any user-facing changes?
A new API and a deprecation of an existing one
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please call them out.
-->
--
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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1874099620
##########
parquet/src/file/properties.rs:
##########
@@ -780,22 +779,24 @@ impl WriterPropertiesBuilder {
self
}
- /// Sets flag to control if type coercion is enabled (defaults to `false`).
+ /// Should the writer coerce types to parquet native types (defaults to `false`).
///
- /// # Notes
- /// Some Arrow types do not have a corresponding Parquet logical type.
- /// Affected Arrow data types include `Date64`, `Timestamp` and `Interval`.
- /// Also, for [`List`] and [`Map`] types, Parquet expects certain schema elements
- /// to have specific names to be considered fully compliant.
- /// Writers have the option to coerce these types and names to match those required
- /// by the Parquet specification.
- /// This type coercion allows for meaningful representations that do not require
- /// downstream readers to consider the embedded Arrow schema, and can allow for greater
- /// compatibility with other Parquet implementations. However, type
- /// coercion also prevents the data from being losslessly round-tripped.
- ///
- /// [`List`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
- /// [`Map`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps
+ /// Leaving this option the default `false` will ensure the exact same data
+ /// written to parquet using this library will be read.
+ ///
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but potentially lose information in the process.
+ ///
+ /// * Types such as [`DataType::Date64`], which have no direct corresponding
+ /// Parquet type, may be stored with lower precision.
+ ///
+ /// * The internal field names of [`List`] and [`Map`] will be renamed if
Review Comment:
```suggestion
/// * The internal field names of `List` and `Map` types will be renamed if
```
Since we already link to the new converter, I don't know if it's worth linking to the parquet docs here too.
--
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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1887567117
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +219,134 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+///
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
Review Comment:
It is also more consistent with things like https://docs.rs/parquet/latest/parquet/arrow/arrow_reader/struct.ArrowReaderBuilder.html -- I will change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#issuecomment-2521652449
@alamb could you take a look at #6828? Some of the doc changes will conflict, at least. +1 on the idea, 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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1887571560
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +219,134 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+///
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we coerce Arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool,
}
-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(
- schema: &Schema,
- root: &str,
- coerce_types: bool,
-) -> Result<SchemaDescriptor> {
- let fields = schema
- .fields()
- .iter()
- .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
- .collect::<Result<_>>()?;
- let group = Type::group_type_builder(root).with_fields(fields).build()?;
- Ok(SchemaDescriptor::new(Arc::new(group)))
+impl<'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should Arrow types be coerced into Parquet native types (default `false`).
+ ///
+ /// Setting this option to `true` will result in Parquet files that can be
+ /// read by more readers, but may lose precision for Arrow types such as
+ /// [`DataType::Date64`] which have no direct [corresponding Parquet type].
+ ///
+ /// By default, this converter does not coerce to native Parquet types. Enabling type
+ /// coercion allows for meaningful representations that do not require
+ /// downstream readers to consider the embedded Arrow schema, and can allow
+ /// for greater compatibility with other Parquet implementations. However,
+ /// type coercion also prevents data from being losslessly round-tripped.
+ ///
+ /// # Discussion
+ ///
+ /// Some Arrow types such as `Date64`, `Timestamp` and `Interval` have no
+ /// corresponding Parquet logical type. Thus, they can not be losslessly
+ /// round-tripped when stored using the appropriate Parquet logical type.
+ /// For example, some Date64 values may be truncated when stored with
+ /// parquet's native 32 bit date type.
+ ///
+ /// For [`List`] and [`Map`] types, some Parquet readers expect certain
+ /// schema elements to have specific names (earlier versions of the spec
+ /// were somewhat ambiguous on this point). Type coercion will use the names
+ /// prescribed by the Parquet specification, potentially losing naming
+ /// metadata from the Arrow schema.
+ ///
+ /// [`List`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
+ /// [`Map`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps
+ /// [corresponding Parquet type]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#date
+ ///
+ pub fn with_coerce_types(mut self, coerce_types: bool) -> Self {
+ self.coerce_types = coerce_types;
+ self
+ }
+
+ /// Set the root schema element name (defaults to `"arrow_schema"`).
+ pub fn schema_root(mut self, schema_root: &'a str) -> Self {
Review Comment:
Actually, I double checked and Ie think `with_schema_root` is more consistent with `ArrowWriterOptions`
https://github.com/apache/arrow-rs/blob/9ffa06543be51613ea1f509e63f6e7405b7d9989/parquet/src/arrow/arrow_writer/mod.rs#L392-L393
--
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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1887571560
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +219,134 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+///
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we coerce Arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool,
}
-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(
- schema: &Schema,
- root: &str,
- coerce_types: bool,
-) -> Result<SchemaDescriptor> {
- let fields = schema
- .fields()
- .iter()
- .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
- .collect::<Result<_>>()?;
- let group = Type::group_type_builder(root).with_fields(fields).build()?;
- Ok(SchemaDescriptor::new(Arc::new(group)))
+impl<'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should Arrow types be coerced into Parquet native types (default `false`).
+ ///
+ /// Setting this option to `true` will result in Parquet files that can be
+ /// read by more readers, but may lose precision for Arrow types such as
+ /// [`DataType::Date64`] which have no direct [corresponding Parquet type].
+ ///
+ /// By default, this converter does not coerce to native Parquet types. Enabling type
+ /// coercion allows for meaningful representations that do not require
+ /// downstream readers to consider the embedded Arrow schema, and can allow
+ /// for greater compatibility with other Parquet implementations. However,
+ /// type coercion also prevents data from being losslessly round-tripped.
+ ///
+ /// # Discussion
+ ///
+ /// Some Arrow types such as `Date64`, `Timestamp` and `Interval` have no
+ /// corresponding Parquet logical type. Thus, they can not be losslessly
+ /// round-tripped when stored using the appropriate Parquet logical type.
+ /// For example, some Date64 values may be truncated when stored with
+ /// parquet's native 32 bit date type.
+ ///
+ /// For [`List`] and [`Map`] types, some Parquet readers expect certain
+ /// schema elements to have specific names (earlier versions of the spec
+ /// were somewhat ambiguous on this point). Type coercion will use the names
+ /// prescribed by the Parquet specification, potentially losing naming
+ /// metadata from the Arrow schema.
+ ///
+ /// [`List`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
+ /// [`Map`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps
+ /// [corresponding Parquet type]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#date
+ ///
+ pub fn with_coerce_types(mut self, coerce_types: bool) -> Self {
+ self.coerce_types = coerce_types;
+ self
+ }
+
+ /// Set the root schema element name (defaults to `"arrow_schema"`).
+ pub fn schema_root(mut self, schema_root: &'a str) -> Self {
Review Comment:
Actually, I think `with_schema_root` is more consistent with other options:
https://github.com/apache/arrow-rs/blob/9ffa06543be51613ea1f509e63f6e7405b7d9989/parquet/src/arrow/arrow_writer/mod.rs#L393-L392
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +219,134 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+///
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we coerce Arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool,
}
-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(
- schema: &Schema,
- root: &str,
- coerce_types: bool,
-) -> Result<SchemaDescriptor> {
- let fields = schema
- .fields()
- .iter()
- .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
- .collect::<Result<_>>()?;
- let group = Type::group_type_builder(root).with_fields(fields).build()?;
- Ok(SchemaDescriptor::new(Arc::new(group)))
+impl<'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should Arrow types be coerced into Parquet native types (default `false`).
+ ///
+ /// Setting this option to `true` will result in Parquet files that can be
+ /// read by more readers, but may lose precision for Arrow types such as
+ /// [`DataType::Date64`] which have no direct [corresponding Parquet type].
+ ///
+ /// By default, this converter does not coerce to native Parquet types. Enabling type
+ /// coercion allows for meaningful representations that do not require
+ /// downstream readers to consider the embedded Arrow schema, and can allow
+ /// for greater compatibility with other Parquet implementations. However,
+ /// type coercion also prevents data from being losslessly round-tripped.
+ ///
+ /// # Discussion
+ ///
+ /// Some Arrow types such as `Date64`, `Timestamp` and `Interval` have no
+ /// corresponding Parquet logical type. Thus, they can not be losslessly
+ /// round-tripped when stored using the appropriate Parquet logical type.
+ /// For example, some Date64 values may be truncated when stored with
+ /// parquet's native 32 bit date type.
+ ///
+ /// For [`List`] and [`Map`] types, some Parquet readers expect certain
+ /// schema elements to have specific names (earlier versions of the spec
+ /// were somewhat ambiguous on this point). Type coercion will use the names
+ /// prescribed by the Parquet specification, potentially losing naming
+ /// metadata from the Arrow schema.
+ ///
+ /// [`List`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
+ /// [`Map`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps
+ /// [corresponding Parquet type]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#date
+ ///
+ pub fn with_coerce_types(mut self, coerce_types: bool) -> Self {
+ self.coerce_types = coerce_types;
+ self
+ }
+
+ /// Set the root schema element name (defaults to `"arrow_schema"`).
+ pub fn schema_root(mut self, schema_root: &'a str) -> Self {
Review Comment:
Actually, I think `with_schema_root_name` is more consistent with other options:
https://github.com/apache/arrow-rs/blob/9ffa06543be51613ea1f509e63f6e7405b7d9989/parquet/src/arrow/arrow_writer/mod.rs#L393-L392
--
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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#issuecomment-2546757187
> I did wonder if this should instead be an options struct that gets passed to a method (or has a member function that accepts the actual schema). This would allow using the same config for multiple files, and is perhaps more consistent with other APIs?
I agree, as there is no state held in the converter there is no reason for it to be a consuming builder. I made this change in 0ab4eb4ccc
--
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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1874049637
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +225,130 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
Review Comment:
I think we should strive for always capitalizing them `Arrow` and `Parquet` -- i will fix
--
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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1872225592
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,29 +225,99 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
+/// Converter for arrow schema to parquet schema
+///
+/// Example:
+/// ```
+/// # use arrow_schema::{Field, Schema, DataType};
+/// use parquet::arrow::ArrowToParquetSchemaConverter;
+/// let schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, false),
+/// Field::new("b", DataType::Date32, false),
+/// ];
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&schema)
+/// .build()
+/// .unwrap();
+///
+///
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we Coerce arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool
+}
+
+impl <'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should arrow types be coerced into parquet native types (default false).
+ ///
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but may lose precision for arrow types such as
+ /// [`DataType::Date64`] which have no direct corresponding Parquet type.
+ ///
+ /// # Discussion
+ ///
+ /// Some Arrow types such as `Date64`, `Timestamp` and `Interval` have no
+ /// corresponding Parquet logical type. Thus, they can not be losslessly
+ /// round-tripped when stored using the appropriate Parquet logical type.
+ ///
+ /// For example, some Date64 values may be truncated when stored with
+ /// parquet's native 32 bit date type.
+ ///
+ /// By default, the arrow writer does not coerce to native parquet types. It
+ /// writes data in such a way that it can be lossless round tripped.
+ /// However, this means downstream readers must be aware of and correctly
+ /// interpret the embedded Arrow schema.
+ pub fn with_coerce_types(mut self, coerce_types: bool) -> Self {
+ self.coerce_types = coerce_types;
+ self
+ }
+
+ /// Set the root schema element name (defaults to `"arrow_schema"`).
+ pub fn schema_root(mut self, schema_root: &'a str) -> Self {
+ self.schema_root = schema_root;
+ self
+ }
+
+ /// Build the desired parquet [`SchemaDescriptor`]
+ pub fn build(self) -> Result<SchemaDescriptor> {
+ let Self { schema, schema_root: root_schema_name, coerce_types } = self;
+ let fields = schema
+ .fields()
+ .iter()
+ .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
+ .collect::<Result<_>>()?;
+ let group = Type::group_type_builder(root_schema_name).with_fields(fields).build()?;
+ Ok(SchemaDescriptor::new(Arc::new(group)))
+ }
+}
+
/// Convert arrow schema to parquet schema
///
/// The name of the root schema element defaults to `"arrow_schema"`, this can be
/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
-}
+#[deprecated(since = "54.0.0", note = "Use `ArrowToParquetSchemaConverter` instead")]
+pub fn arrow_to_parquet_schema(schema: &Schema) -> Result<SchemaDescriptor> {
Review Comment:
This reverts the changes made in https://github.com/apache/arrow-rs/pull/6313/files#diff-6a684124e78f254fa6ea23b98f04be0a3ff5a2e4b24890d9c6f4dd850ba11333L232
##########
parquet/src/file/properties.rs:
##########
@@ -287,15 +286,13 @@ impl WriterProperties {
self.statistics_truncate_length
}
- /// Returns `coerce_types` boolean
+ /// Should the writer coerce types to parquet native types.
+ ///
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but may lose precision for arrow types such as
+ /// [`DataType::Date64`] which have no direct corresponding Parquet type.
Review Comment:
I also felt it would help if we described more explicitly what the impact of enabling this option was (and left a link to the longer backstory)
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,29 +225,99 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
+/// Converter for arrow schema to parquet schema
+///
+/// Example:
+/// ```
+/// # use arrow_schema::{Field, Schema, DataType};
+/// use parquet::arrow::ArrowToParquetSchemaConverter;
+/// let schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, false),
+/// Field::new("b", DataType::Date32, false),
+/// ];
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&schema)
+/// .build()
+/// .unwrap();
+///
+///
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we Coerce arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool
+}
+
+impl <'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should arrow types be coerced into parquet native types (default false).
+ ///
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but may lose precision for arrow types such as
+ /// [`DataType::Date64`] which have no direct corresponding Parquet type.
+ ///
+ /// # Discussion
+ ///
+ /// Some Arrow types such as `Date64`, `Timestamp` and `Interval` have no
+ /// corresponding Parquet logical type. Thus, they can not be losslessly
+ /// round-tripped when stored using the appropriate Parquet logical type.
+ ///
+ /// For example, some Date64 values may be truncated when stored with
+ /// parquet's native 32 bit date type.
+ ///
+ /// By default, the arrow writer does not coerce to native parquet types. It
+ /// writes data in such a way that it can be lossless round tripped.
+ /// However, this means downstream readers must be aware of and correctly
+ /// interpret the embedded Arrow schema.
+ pub fn with_coerce_types(mut self, coerce_types: bool) -> Self {
+ self.coerce_types = coerce_types;
+ self
+ }
+
+ /// Set the root schema element name (defaults to `"arrow_schema"`).
+ pub fn schema_root(mut self, schema_root: &'a str) -> Self {
+ self.schema_root = schema_root;
+ self
+ }
+
+ /// Build the desired parquet [`SchemaDescriptor`]
+ pub fn build(self) -> Result<SchemaDescriptor> {
+ let Self { schema, schema_root: root_schema_name, coerce_types } = self;
+ let fields = schema
+ .fields()
+ .iter()
+ .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
+ .collect::<Result<_>>()?;
+ let group = Type::group_type_builder(root_schema_name).with_fields(fields).build()?;
+ Ok(SchemaDescriptor::new(Arc::new(group)))
+ }
+}
+
/// Convert arrow schema to parquet schema
///
/// The name of the root schema element defaults to `"arrow_schema"`, this can be
/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
-}
+#[deprecated(since = "54.0.0", note = "Use `ArrowToParquetSchemaConverter` instead")]
+pub fn arrow_to_parquet_schema(schema: &Schema) -> Result<SchemaDescriptor> {
-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(
Review Comment:
It turns out this function is not actually exported (it is pub in this module, but not pub exported):
https://docs.rs/parquet/latest/parquet/?search=arrow_to_parquet_schema_with_root
Returns no results
The compiler told me it was unused once I switched everything over to use `ArrowToParquetSchemaConverter`
--
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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#issuecomment-2546844793
Thanks again @etseidl and @tustvold
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840
--
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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1883013129
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +219,134 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+///
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
Review Comment:
```suggestion
pub struct ArrowSchemaConverter<'a> {
```
Perhaps, I don't feel strongly but might reduce the word salad
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +219,134 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+///
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we coerce Arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool,
}
-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(
- schema: &Schema,
- root: &str,
- coerce_types: bool,
-) -> Result<SchemaDescriptor> {
- let fields = schema
- .fields()
- .iter()
- .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
- .collect::<Result<_>>()?;
- let group = Type::group_type_builder(root).with_fields(fields).build()?;
- Ok(SchemaDescriptor::new(Arc::new(group)))
+impl<'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should Arrow types be coerced into Parquet native types (default `false`).
+ ///
+ /// Setting this option to `true` will result in Parquet files that can be
+ /// read by more readers, but may lose precision for Arrow types such as
+ /// [`DataType::Date64`] which have no direct [corresponding Parquet type].
+ ///
+ /// By default, this converter does not coerce to native Parquet types. Enabling type
+ /// coercion allows for meaningful representations that do not require
+ /// downstream readers to consider the embedded Arrow schema, and can allow
+ /// for greater compatibility with other Parquet implementations. However,
+ /// type coercion also prevents data from being losslessly round-tripped.
+ ///
+ /// # Discussion
+ ///
+ /// Some Arrow types such as `Date64`, `Timestamp` and `Interval` have no
+ /// corresponding Parquet logical type. Thus, they can not be losslessly
+ /// round-tripped when stored using the appropriate Parquet logical type.
+ /// For example, some Date64 values may be truncated when stored with
+ /// parquet's native 32 bit date type.
+ ///
+ /// For [`List`] and [`Map`] types, some Parquet readers expect certain
+ /// schema elements to have specific names (earlier versions of the spec
+ /// were somewhat ambiguous on this point). Type coercion will use the names
+ /// prescribed by the Parquet specification, potentially losing naming
+ /// metadata from the Arrow schema.
+ ///
+ /// [`List`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
+ /// [`Map`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps
+ /// [corresponding Parquet type]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#date
+ ///
+ pub fn with_coerce_types(mut self, coerce_types: bool) -> Self {
+ self.coerce_types = coerce_types;
+ self
+ }
+
+ /// Set the root schema element name (defaults to `"arrow_schema"`).
+ pub fn schema_root(mut self, schema_root: &'a str) -> Self {
Review Comment:
```suggestion
pub fn with_root(mut self, schema_root: &'a str) -> Self {
```
Perhaps? For consistency and because the schema is perhaps redundant?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1874084200
##########
parquet/src/file/properties.rs:
##########
@@ -780,22 +779,16 @@ impl WriterPropertiesBuilder {
self
}
- /// Sets flag to control if type coercion is enabled (defaults to `false`).
+ /// Should the writer coerce types to parquet native types (defaults to `false`).
///
- /// # Notes
- /// Some Arrow types do not have a corresponding Parquet logical type.
- /// Affected Arrow data types include `Date64`, `Timestamp` and `Interval`.
- /// Also, for [`List`] and [`Map`] types, Parquet expects certain schema elements
- /// to have specific names to be considered fully compliant.
- /// Writers have the option to coerce these types and names to match those required
- /// by the Parquet specification.
- /// This type coercion allows for meaningful representations that do not require
- /// downstream readers to consider the embedded Arrow schema, and can allow for greater
- /// compatibility with other Parquet implementations. However, type
- /// coercion also prevents the data from being losslessly round-tripped.
- ///
- /// [`List`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
- /// [`Map`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but may lose precision for arrow types such as
+ /// [`DataType::Date64`] which have no direct corresponding Parquet type.
Review 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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#issuecomment-2525095146
> Just the doc CI failure to fix. Otherwise looks ready to go (mod capitalization which can be a follow on 😉). Thanks again @alamb.
Thank you 🙏
I fixed a few more capitalization inconsistencies and added a link to the date section of the parquet spec too
--
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] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1872335773
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -1487,7 +1590,10 @@ mod tests {
";
let parquet_group_type = parse_message_type(message_type).unwrap();
let parquet_schema = SchemaDescriptor::new(Arc::new(parquet_group_type));
- let converted_arrow_schema = arrow_to_parquet_schema(&arrow_schema, true).unwrap();
+ let converted_arrow_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+ .with_coerce_types(true)
Review Comment:
I love how this makes what's going on more explicit.
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +225,130 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+/// //
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we Coerce arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool,
}
-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(
- schema: &Schema,
- root: &str,
- coerce_types: bool,
-) -> Result<SchemaDescriptor> {
- let fields = schema
- .fields()
- .iter()
- .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
- .collect::<Result<_>>()?;
- let group = Type::group_type_builder(root).with_fields(fields).build()?;
- Ok(SchemaDescriptor::new(Arc::new(group)))
+impl<'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should arrow types be coerced into parquet native types (default false).
+ ///
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but may lose precision for arrow types such as
+ /// [`DataType::Date64`] which have no direct corresponding Parquet type.
+ ///
+ /// By default, does not coerce to native parquet types. Enabling type
+ /// coercion allows for meaningful representations that do not require
+ /// downstream readers to consider the embedded Arrow schema, and can allow
+ /// for greater compatibility with other Parquet implementations. However,
+ /// type coercion also prevents data from being losslessly round-tripped.
+ ///
+ /// # Discussion
+ ///
+ /// Some Arrow types such as `Date64`, `Timestamp` and `Interval` have no
+ /// corresponding Parquet logical type. Thus, they can not be losslessly
+ /// round-tripped when stored using the appropriate Parquet logical type.
+ ///
+ /// For example, some Date64 values may be truncated when stored with
+ /// parquet's native 32 bit date type. For [`List`] and [`Map`] types, some
+ /// Parquet readers expect certain schema elements to have specific names
+ /// (earlier versions of the spec was somewhat ambiguous on this point).
Review Comment:
```suggestion
/// round-tripped when stored using the appropriate Parquet logical type.
/// For example, some Date64 values may be truncated when stored with
/// parquet's native 32 bit date type.
///
/// For [`List`] and [`Map`] types, some
/// Parquet readers expect certain schema elements to have specific names
/// (earlier versions of the spec were somewhat ambiguous on this point).
/// Type coercion will use the names prescribed by the Parquet specification,
/// potentially losing naming metadata from the Arrow schema.
```
Just trying to make clear what the naming impacts are...not sure if this is an improvement.
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +225,130 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
Review Comment:
The docs are pretty inconsistent on whether "arrow" and "parquet" are capitalized or not. Do you have a preference?
##########
parquet/src/file/properties.rs:
##########
@@ -780,22 +779,16 @@ impl WriterPropertiesBuilder {
self
}
- /// Sets flag to control if type coercion is enabled (defaults to `false`).
+ /// Should the writer coerce types to parquet native types (defaults to `false`).
///
- /// # Notes
- /// Some Arrow types do not have a corresponding Parquet logical type.
- /// Affected Arrow data types include `Date64`, `Timestamp` and `Interval`.
- /// Also, for [`List`] and [`Map`] types, Parquet expects certain schema elements
- /// to have specific names to be considered fully compliant.
- /// Writers have the option to coerce these types and names to match those required
- /// by the Parquet specification.
- /// This type coercion allows for meaningful representations that do not require
- /// downstream readers to consider the embedded Arrow schema, and can allow for greater
- /// compatibility with other Parquet implementations. However, type
- /// coercion also prevents the data from being losslessly round-tripped.
- ///
- /// [`List`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
- /// [`Map`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but may lose precision for arrow types such as
+ /// [`DataType::Date64`] which have no direct corresponding Parquet type.
Review Comment:
I think we should still mention the naming impacts here. There seem to be a number of users who want Parquet naming, and I think this is the first place they'd look to figure out how to do so. I'm not sure I'd think to follow the link to the converter when I want my lists named properly.
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +225,130 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+/// //
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we Coerce arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool,
}
-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(
- schema: &Schema,
- root: &str,
- coerce_types: bool,
-) -> Result<SchemaDescriptor> {
- let fields = schema
- .fields()
- .iter()
- .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
- .collect::<Result<_>>()?;
- let group = Type::group_type_builder(root).with_fields(fields).build()?;
- Ok(SchemaDescriptor::new(Arc::new(group)))
+impl<'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should arrow types be coerced into parquet native types (default false).
+ ///
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but may lose precision for arrow types such as
+ /// [`DataType::Date64`] which have no direct corresponding Parquet type.
+ ///
+ /// By default, does not coerce to native parquet types. Enabling type
Review Comment:
```suggestion
/// By default, this converter does not coerce to native parquet types. Enabling type
```
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +225,130 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+/// //
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we Coerce arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool,
}
-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(
- schema: &Schema,
- root: &str,
- coerce_types: bool,
-) -> Result<SchemaDescriptor> {
- let fields = schema
- .fields()
- .iter()
- .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
- .collect::<Result<_>>()?;
- let group = Type::group_type_builder(root).with_fields(fields).build()?;
- Ok(SchemaDescriptor::new(Arc::new(group)))
+impl<'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should arrow types be coerced into parquet native types (default false).
Review Comment:
```suggestion
/// Should arrow types be coerced into parquet native types (default `false`).
```
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +225,130 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+/// //
Review Comment:
```suggestion
///
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6840:
URL: https://github.com/apache/arrow-rs/pull/6840#discussion_r1874054517
##########
parquet/src/arrow/schema/mod.rs:
##########
@@ -225,27 +225,130 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
}
}
-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema
///
-/// The name of the root schema element defaults to `"arrow_schema"`, this can be
-/// overridden with [`arrow_to_parquet_schema_with_root`]
-pub fn arrow_to_parquet_schema(schema: &Schema, coerce_types: bool) -> Result<SchemaDescriptor> {
- arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
+/// Example:
+/// ```
+/// # use std::sync::Arc;
+/// # use arrow_schema::{Field, Schema, DataType};
+/// # use parquet::arrow::ArrowToParquetSchemaConverter;
+/// use parquet::schema::types::{SchemaDescriptor, Type};
+/// use parquet::basic; // note there are two `Type`s in the following example
+/// let arrow_schema = Schema::new(vec![
+/// Field::new("a", DataType::Int64, true),
+/// Field::new("b", DataType::Date32, true),
+/// ]);
+///
+/// let parquet_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+/// .build()
+/// .unwrap();
+/// //
+/// let expected_parquet_schema = SchemaDescriptor::new(
+/// Arc::new(
+/// Type::group_type_builder("arrow_schema")
+/// .with_fields(vec![
+/// Arc::new(
+/// Type::primitive_type_builder("a", basic::Type::INT64)
+/// .build().unwrap()
+/// ),
+/// Arc::new(
+/// Type::primitive_type_builder("b", basic::Type::INT32)
+/// .with_converted_type(basic::ConvertedType::DATE)
+/// .with_logical_type(Some(basic::LogicalType::Date))
+/// .build().unwrap()
+/// ),
+/// ])
+/// .build().unwrap()
+/// )
+/// );
+///
+/// assert_eq!(parquet_schema, expected_parquet_schema);
+/// ```
+#[derive(Debug)]
+pub struct ArrowToParquetSchemaConverter<'a> {
+ /// The schema to convert
+ schema: &'a Schema,
+ /// Name of the root schema in Parquet
+ schema_root: &'a str,
+ /// Should we Coerce arrow types to compatible Parquet types?
+ ///
+ /// See docs on [Self::with_coerce_types]`
+ coerce_types: bool,
}
-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(
- schema: &Schema,
- root: &str,
- coerce_types: bool,
-) -> Result<SchemaDescriptor> {
- let fields = schema
- .fields()
- .iter()
- .map(|field| arrow_to_parquet_type(field, coerce_types).map(Arc::new))
- .collect::<Result<_>>()?;
- let group = Type::group_type_builder(root).with_fields(fields).build()?;
- Ok(SchemaDescriptor::new(Arc::new(group)))
+impl<'a> ArrowToParquetSchemaConverter<'a> {
+ /// Create a new converter
+ pub fn new(schema: &'a Schema) -> Self {
+ Self {
+ schema,
+ schema_root: "arrow_schema",
+ coerce_types: false,
+ }
+ }
+
+ /// Should arrow types be coerced into parquet native types (default false).
+ ///
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but may lose precision for arrow types such as
+ /// [`DataType::Date64`] which have no direct corresponding Parquet type.
+ ///
+ /// By default, does not coerce to native parquet types. Enabling type
+ /// coercion allows for meaningful representations that do not require
+ /// downstream readers to consider the embedded Arrow schema, and can allow
+ /// for greater compatibility with other Parquet implementations. However,
+ /// type coercion also prevents data from being losslessly round-tripped.
+ ///
+ /// # Discussion
+ ///
+ /// Some Arrow types such as `Date64`, `Timestamp` and `Interval` have no
+ /// corresponding Parquet logical type. Thus, they can not be losslessly
+ /// round-tripped when stored using the appropriate Parquet logical type.
+ ///
+ /// For example, some Date64 values may be truncated when stored with
+ /// parquet's native 32 bit date type. For [`List`] and [`Map`] types, some
+ /// Parquet readers expect certain schema elements to have specific names
+ /// (earlier versions of the spec was somewhat ambiguous on this point).
Review Comment:
I think it is an improvment
##########
parquet/src/file/properties.rs:
##########
@@ -780,22 +779,16 @@ impl WriterPropertiesBuilder {
self
}
- /// Sets flag to control if type coercion is enabled (defaults to `false`).
+ /// Should the writer coerce types to parquet native types (defaults to `false`).
///
- /// # Notes
- /// Some Arrow types do not have a corresponding Parquet logical type.
- /// Affected Arrow data types include `Date64`, `Timestamp` and `Interval`.
- /// Also, for [`List`] and [`Map`] types, Parquet expects certain schema elements
- /// to have specific names to be considered fully compliant.
- /// Writers have the option to coerce these types and names to match those required
- /// by the Parquet specification.
- /// This type coercion allows for meaningful representations that do not require
- /// downstream readers to consider the embedded Arrow schema, and can allow for greater
- /// compatibility with other Parquet implementations. However, type
- /// coercion also prevents the data from being losslessly round-tripped.
- ///
- /// [`List`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
- /// [`Map`]: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps
+ /// Setting this option to `true` will result in parquet files that can be
+ /// read by more readers, but may lose precision for arrow types such as
+ /// [`DataType::Date64`] which have no direct corresponding Parquet type.
Review Comment:
I agree - I pushed e7b7d20eb3 to try and clarify -- let me know what you think
--
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