-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Add ParquetExec::builder()
, deprecate ParquetExec::new
#10636
Conversation
6cefd61
to
313252e
Compare
c6a0dab
to
9c100e4
Compare
9c100e4
to
f4bccc2
Compare
ParquetExec::builder
APIParquetExec::builder()
, deprecate ParquetExec::new
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.
Great refactor 👍 Those docs are also brilliant!
A tiny suggestion, we can add #[must_use]
to those methods on ParquetExecBuilder
to prevent a builder is not .build()
at the end.
Thanks @waynexia !
I added what I think you are referring to in 70a14dc (though I am not quite sure). Let me know if you had a different idea and I will make that change. |
Something like https://github.com/apache/arrow-rs/blob/12c0d007947a2acb01738a737c3be24f779749f8/arrow-flight/src/arrow.flight.protocol.rs#L501-L523 But after some search, I realized that |
Which issue does this PR close?
Part of #10546
Rationale for this change
While working on #10549 it cumbersome to create a
ParquetExec
-- there are a few fields onParquetExec::new
that need default values that make it slightly confusing to use.What changes are included in this PR?
ParquetExecBuilder
andParquetExec::builder()
that handle defaultsParquetExec::new
You can see this in action here #10618
Are these changes tested?
Yes, covered by existing tests and new doc example tests
Are there any user-facing changes?
Better docs / examples / builder.
ParquetExec::new()
is now deprecated