generate parquet schema from rust struct #539
Conversation
|
Might be of interest to @xrl and @bryantbiggs as you were the original authors |
Codecov Report
@@ Coverage Diff @@
## master #539 +/- ##
==========================================
- Coverage 82.60% 82.45% -0.15%
==========================================
Files 167 167
Lines 45984 46084 +100
==========================================
+ Hits 37984 37999 +15
- Misses 8000 8085 +85
Continue to review full report at Codecov.
|
|
nice!!! 🎉 |
nevi-me
force-pushed
the
parquet-derive-generate-schema
branch
from
aa6b1b0
to
e13b9e7
Compare
| @@ -69,7 +64,7 @@ mod parquet_field; | |||
| /// } | |||
| /// ]; | |||
| /// | |||
| /// let schema = Arc::new(parse_message_type(schema_str).unwrap()); | |||
| /// let schema = samples.as_slice().schema(); | |||
There was a problem hiding this comment.
It would be cool to avoid having to use as_slice() but this PR seems significantly better than previously, so 👍 for me. We can make it even better in the future
There was a problem hiding this comment.
Yeah, I didn't like it too, but RecordWriter is implemented on &[Struct], so this was the least disruptive change to the trait I could make
There was a problem hiding this comment.
maybe we could implement it for anything that implemented IntoIterator<Item=P> where P: AsRef<Struct> or whatever
There was a problem hiding this comment.
I'll try that out in future PRs. The code that gets generated iterates through the slice n times, where n is the number of fields. It's not a true record writer in the parquet sense, but it uses the column writer internally.
I think we'll be able to improve on it when I work on nested type support.
There was a problem hiding this comment.
I appreciate the extra attention to this code. My team relies on the derive macro to translate simple structs to parquet, which we do very often in our data pipeline. I'd be happy to test out future, more comprehensive versions. I applaud you effort, I was too intimidated by the record writer to keep going!
| use parquet::basic::*; | ||
|
|
||
| let mut fields: Vec<TypePtr> = Vec::new(); | ||
| #( |
| @@ -174,6 +174,50 @@ impl Field { | |||
| } | |||
| } | |||
|
|
|||
| pub fn parquet_type(&self) -> proc_macro2::TokenStream { | |||
| // TODO: Support group types | |||
There was a problem hiding this comment.
maybe we should add notes about these TODO's in the docstrings (aka "group types are not yet supported") ?
| @@ -57,27 +64,32 @@ mod tests { | |||
| #[test] | |||
| fn test_parquet_derive_hello() { | |||
| let file = get_temp_file("test_parquet_derive_hello", &[]); | |||
| let schema_str = "message schema { | |||
|
|
|||
| // The schema is not required, but this tests that the generated | |||
| let schema = Arc::new(parse_message_type(schema_str).unwrap()); | ||
| let generated_schema = drs.as_slice().schema().unwrap(); | ||
|
|
||
| assert_eq!(&schema, &generated_schema); |
|
I plan to merge this later today (will be included in 5.0) unless anyone else wants more time to review |
alamb
added
the
enhancement
Which issue does this PR close?
None
Rationale for this change
Users of
parquet_derivecurrently have to write the schema of their Rust struct by hand.This is inconvenient, and can be generated for them.
What changes are included in this PR?
Adds
parquet::record::RecordWriter::schema()trait member, and an implementation inparquet_deriveto generate a schema for the user.Are there any user-facing changes?
Yes, new API. The breakage is probably irrelevant as I don't think that there are many users of
parquet_derive.