Skip to content

ARROW-11992: [Rust][Parquet] Add upgrade notes on 4.0 rename of LogicalType #9731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alamb wants to merge 3 commits into from
Closed

ARROW-11992: [Rust][Parquet] Add upgrade notes on 4.0 rename of LogicalType #9731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1c20aa
Add upgrade notes on 4.0 rename of LogicalType
alamb Mar 16, 2021
c80b6b5
Improve wording in readme.md
alamb Mar 18, 2021
487b631
reduce redundancy in comments on `LogicalType`
alamb Mar 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions rust/parquet/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ while let Some(record) = iter.next() {
```
See [crate documentation](https://docs.rs/crate/parquet/4.0.0-SNAPSHOT) on available API.

## Upgrading from versions prior to 4.0
Copy link
Member

@sunchao sunchao Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to add this temporarily in README.md but I think we should put them this alongside other breaking changes from 4.0 (seems there're quite a few of them) in a separate doc eventually. Something similar from Apache Spark.

Copy link
Contributor Author

@alamb alamb Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summarizing breaking changes would definitely be valuable. I don't think I have the time to do so at this moment, but I am very supportive of the idea

Copy link
Contributor

@nevi-me nevi-me Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a guide as part of https://issues.apache.org/jira/browse/ARROW-12019, to provide examples of how one can start using LogicalType.


If you are upgrading from version 3.0 or previous of this crate, you
likely need to change your code to use [`ConvertedType`] rather than
[`LogicalType`]. Version 4.0 introduces an *entirely new* struct
called `LogicalType` to align with the `LogicalType` introduced in
Parquet Format 2.4.0. The type previously called `LogicalType` was was
renamed to `ConvertedType`.


## Supported Parquet Version
- Parquet-format 2.4.0

Expand Down Expand Up @@ -84,7 +94,7 @@ Run `cargo test` for unit tests. To also run tests related to the binaries, use
## Binaries
The following binaries are provided (use `cargo install --features cli` to install them):
- **parquet-schema** for printing Parquet file schema and metadata.
`Usage: parquet-schema <file-path>`, where `file-path` is the path to a Parquet file. Use `-v/--verbose` flag
`Usage: parquet-schema <file-path>`, where `file-path` is the path to a Parquet file. Use `-v/--verbose` flag
to print full metadata or schema only (when not specified only schema will be printed).

- **parquet-read** for reading records from a Parquet file.
Expand All @@ -93,8 +103,8 @@ and `num-records` is the number of records to read from a file (when not specifi
be printed). Use `-j/--json` to print records in JSON lines format.

- **parquet-rowcount** for reporting the number of records in one or more Parquet files.
`Usage: parquet-rowcount <file-paths>...`, where `<file-paths>...` is a space separated list of one or more
files to read.
`Usage: parquet-rowcount <file-paths>...`, where `<file-paths>...` is a space separated list of one or more
files to read.

If you see `Library not loaded` error, please make sure `LD_LIBRARY_PATH` is set properly:
```
Expand Down
6 changes: 6 additions & 0 deletions rust/parquet/src/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ pub enum Type {
/// Common types (converted types) used by frameworks when using Parquet.
/// This helps map between types in those frameworks to the base types in Parquet.
/// This is only metadata and not needed to read or write the data.
///
/// *Upgrade Note*: This struct was renamed from `LogicalType` in version 4.0.0.
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum ConvertedType {
NONE,
Expand Down Expand Up @@ -156,6 +158,10 @@ pub enum ConvertedType {
// Mirrors `parquet::LogicalType`

/// Logical types used by version 2 of the Parquet format.
///
/// *Upgrade Note*: This is an *entirely new* struct as of version
Copy link
Member

@sunchao sunchao Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if its useful to have this message here as well. Maybe the one in README is enough?

Copy link
Contributor Author

@alamb alamb Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is important to put a hint as close to where people will hit the compiler error as possible (e.g. I wouldn't have known to look in the README when I hit the error on upgrade).

That being said, I think you are right that the actual content doesn't need to be duplicated here -- I will change it to point people at the readme for further details.

/// 4.0.0. The struct previously named `LogicalType` was renamed to
/// [`ConvertedType`]. Please see the README.md for more details.
#[derive(Debug, Clone, PartialEq)]
pub enum LogicalType {
STRING(StringType),
Expand Down