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

Fix clippy lint for the number of arguments to CsvExec::new() #11565

Closed
alamb opened this issue Jul 20, 2024 · 1 comment · Fixed by #11633
Closed

Fix clippy lint for the number of arguments to CsvExec::new() #11565

alamb opened this issue Jul 20, 2024 · 1 comment · Fixed by #11633
Labels
enhancement New feature or request

Comments

@alamb
Copy link
Contributor

alamb commented Jul 20, 2024

Is your feature request related to a problem or challenge?

As of #11533 ❤️ from @connec the CsvExec::new() function gets marked by clippy as having too many arguments

We have suppressed the clippy error for now, but that is not a good long term solution

Describe the solution you'd like

I think we should fix the clippy lint for real rather than just suppressing it

Describe alternatives you've considered

I would personally suggest a builder pattern such as the following as it is easy to document and extend in the future without breaking API changes

let csv_exec = CsvExec::builder()
  .with_has_header(true)
  .with_newlines_in_values(true)
  ...
  .build()?

I think we could follow the model of https://docs.rs/datafusion/latest/datafusion/datasource/physical_plan/parquet/struct.ParquetExecBuilder.html

Additional context

No response

@alamb alamb added the enhancement New feature or request label Jul 20, 2024
@connec
Copy link
Contributor

connec commented Jul 20, 2024

I can pick this up as a follow-on from #11533.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants