-
Notifications
You must be signed in to change notification settings - Fork 20
Conversation
@sunchao Could you review this PR? The goal of this work is having documentation/some starting point for people who want to read Parquet files. Publishing docsI added badge in README that points to docs.rs for parquet (it is broken currently). I thought about this and I think that publishing on docs.rs is arguably the best way to do it right now, which is what you also mentioned on the issue #78 . My main points are:
What stops us from doing this currently is generating When we need to update format, we will create a special PR that updates Reasons for committing
Let me know your thoughts, as you are more familiar with overall setup and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sadikovi . This is excellent work!
README.md
Outdated
} | ||
``` | ||
See crate documentation on available API. | ||
|
||
## Requirements | ||
- Rust nightly | ||
- Thrift 0.11.0 or higher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite related, but we may want to also point out the parquet format version that we support - currently it is 2.3.2?
src/basic.rs
Outdated
/// - FLOAT - 4 bytes per value, stored as little-endian. | ||
/// - DOUBLE - 8 bytes per value, stored as little-endian. | ||
/// - BYTE_ARRAY - 4 byte length stored as little endian, followed by bytes. | ||
/// FIXED_LEN_BYTE_ARRAY - just the bytes are stored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missed -
at the beginning.
src/lib.rs
Outdated
#[macro_use] | ||
pub mod errors; | ||
pub mod basic; | ||
pub mod data_type; | ||
mod parquet_thrift; | ||
|
||
#[macro_use] | ||
pub mod util; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break cargo bench
. We need to find a way to expose only the necessary APIs.
src/schema/types.rs
Outdated
/// Conversion from Thrift equivalents | ||
|
||
/// Conversion from Thrift equivalents. | ||
/// Should be considered private. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pub
can't be removed? add a TODO?
I just need to document |
@sunchao I think it is ready for review now. Would you mind having a look again? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! with a few minor comments.
README.md
Outdated
``` | ||
See crate documentation on available API. | ||
|
||
## Versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of Versions
, maybe Supported Parquet Version
?
Also, the version we support right now is 2.4.0
src/schema/types.rs
Outdated
// Creates a new `PrimitiveType` instance from the gathered attributes. | ||
// This also checks various illegal conditions and returns `Err` if that that happen. | ||
/// Creates a new `PrimitiveType` instance from the gathered attributes. | ||
/// Also checks various illegal conditions and returns `Err` if that that happen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra that. happen -> happens.
I updated the code, can you have a look again? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks! |
This PR updates crate documentation and makes some very minor changes, either in code or comments.
Following is done:
cargo doc
. I learned that it is good to runcargo doc --no-deps
, to generate docs for this crate only when developing.column/reader.rs
tests to give an overview on how setup works.I would encourage to view changes as generated documentation as well as code. You can check out this branch and run
cargo doc --no-deps --open
to open documentation in your browser.Closes #58
Closes #77
Closes #78