Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Oct 26, 2020

As of #8300, it no longer appears possible to implement a ParquetWriter to provide a custom writer because it is not possible to implement the trait as TryClone is not publicly exported.

This PR publically exports that trait and adds an end to end test demonstrating it is possible to create a custom writer

Here is what happens if you try and use TryClone on master

error[E0603]: module `util` is private
  --> delorean_parquet/src/writer.rs:11:5
   |
11 |     util::io::TryClone,
   |     ^^^^ private module
   |
note: the module `util` is defined here
  --> /Users/alamb/.cargo/git/checkouts/arrow-3a9cfebb6b7b2bdc/7155cd5/rust/parquet/src/lib.rs:39:1
   |
39 | mod util;
   | ^^^^^^^^^

@alamb
Copy link
Contributor Author

alamb commented Oct 26, 2020

FYI @rdettai / @sunchao

@alamb alamb changed the title [Rust][Parquet] Ensure it is possible to create custom parquet writers ARROW-10390: [Rust][Parquet] Ensure it is possible to create custom parquet writers Oct 26, 2020
@github-actions
Copy link

Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

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

LGTM, just 1 line change

// ----------------------------------------------------------------------
// Serialized impl for file & row group writers

pub use crate::util::io::TryClone;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if the import is still close to line 41. Here it's hidden, especially as a pub use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved!

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

LGTM. @alamb cargo fmt pls?

@@ -0,0 +1,83 @@
use std::fs::File;
Copy link
Member

Choose a reason for hiding this comment

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

nit: license header

use crate::util::io::{FileSink, Position, TryClone};
use crate::util::io::{FileSink, Position};

// Exposed publically so client code can implement ParquetWriter
Copy link
Member

Choose a reason for hiding this comment

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

nit: perhaps make ParquetWriter a linked text.

@alamb
Copy link
Contributor Author

alamb commented Oct 26, 2020

LGTM. @alamb cargo fmt pls?

@jorgecarleitao 👍

alamb@MacBook-Pro rust % cargo fmt --all
alamb@MacBook-Pro rust % git status
On branch alamb/ARROW-10390-custom-parquet-writer
Your branch is up to date with 'alamb/alamb/ARROW-10390-custom-parquet-writer'.

nothing to commit, working tree clean
alamb@MacBook-Pro rust % 

@nevi-me nevi-me closed this in 06d4f17 Oct 26, 2020
@alamb alamb deleted the alamb/ARROW-10390-custom-parquet-writer branch December 7, 2020 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants