Skip to content
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

API for more ergonomic construction of RecordBatchOptions #2728

Closed
alamb opened this issue Sep 14, 2022 · 3 comments · Fixed by #2729
Closed

API for more ergonomic construction of RecordBatchOptions #2728

alamb opened this issue Sep 14, 2022 · 3 comments · Fixed by #2729
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Sep 14, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
When creating a RecordBatchOptions object it would be nice to use a standard Rust patterns of either

A builder interface :

let options = RecordBatchOptions::new()
  .with_rows_count(Some(row_count))

Or Default::default() for pub structs:

let options = RecordBatchOptions{
      row_count:Some(row_count),
      ..Default::default()
    };

However, there is no builder interface provided and RecordBatchOptions is marked as non-exhaustive so you get this error

error[E0639]: cannot create non-exhaustive struct using struct expression
   --> /home/isidentical/projects/arrow-datafusion/datafusion/core/src/physical_plan/coalesce_batches.rs:295:19
    |
295 |       let options = RecordBatchOptions {
    |  ___________________^
296 | |         row_count: Some(row_count),
297 | |         ..Default::default()
298 | |     };
    | |_____^

For more information about this error, try `rustc --explain E0639`.

This means you have to use mut like:

  let mut options = RecordBatchOptions::default();
  options.row_count = Some(row_count);

Describe the solution you'd like
Add a builder interface for RecordBatchOptions

Additional context

See comments from @isidentical apache/datafusion#3439 (comment)

@alamb alamb added enhancement Any new improvement worthy of a entry in the changelog arrow Changes to the arrow crate labels Sep 14, 2022
@alamb alamb changed the title Make constructing RecordBatchOptions more ergonomic API for more ergonomic construction of RecordBatchOptions Sep 14, 2022
@alamb alamb added the good first issue Good for newcomers label Sep 14, 2022
@alamb
Copy link
Contributor Author

alamb commented Sep 14, 2022

I think this is a good first issue because the API is relatively new, adding a builder interface is backwards compatible, and this would be a good way to learn the codebase and process in arrow

@askoa
Copy link
Contributor

askoa commented Sep 14, 2022

I'll pick this one!

@askoa
Copy link
Contributor

askoa commented Sep 14, 2022

let options = RecordBatchOptions{
      row_count:Some(row_count),
      ..Default::default()
    };

I don't think the above option is possible due to non-exhaustive clause. There is already default implemented for this struct and not working outside of the crate.

Edit: If the builder pattern is implemented then there'll be two forms of initialization for RecordBatchOptions as Default is already implemented and working within the crate. My suggestion is to remove the Default and retain only the Builder. The impact will be within the crate as Default cannot be used outside the crate. Let me know if this is okay.

impl Default for RecordBatchOptions {
fn default() -> Self {
Self {
match_field_names: true,
row_count: None,
}
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants