Skip to content

Conversation

@jimexist
Copy link
Member

@jimexist jimexist commented May 4, 2021

Which issue does this PR close?

We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example Closes #123 indicates that this PR will close issue #123.

Closes #254

Rationale for this change

Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.

What changes are included in this PR?

There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.

Basically a new fn without the mut ref accumulator list param is added, wrapping the original one. All call sites are updated.

Are there any user-facing changes?

  • No

If there are user-facing changes then we may require documentation to be updated before approving the PR.

If there are any breaking changes to public APIs, please add the breaking change label.

@jimexist jimexist marked this pull request as ready for review May 4, 2021 13:37
@jimexist jimexist changed the title refactor common build_file_list refactor physical plan common build_file_list May 4, 2021
@jimexist jimexist changed the title refactor physical plan common build_file_list refactor datafusion/src/physical_plan/common.rs build_file_list May 4, 2021
@jimexist jimexist changed the title refactor datafusion/src/physical_plan/common.rs build_file_list Refactor datafusion/src/physical_plan/common.rs build_file_list to take less param and reuse code May 4, 2021
join_time: 0,
random_state: self.random_state.clone(),
visited_left_side: visited_left_side,
visited_left_side,
Copy link
Member Author

Choose a reason for hiding this comment

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

this fixes a clippy warning

Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed the same thing #259

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @jimexist -- looks like a nice improvement to me.

This is likely technically a "breaking change" (as the function is pub) but I think it is minor and not likely to be a huge problem

join_time: 0,
random_state: self.random_state.clone(),
visited_left_side: visited_left_side,
visited_left_side,
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed the same thing #259

Copy link
Contributor

@Dandandan Dandandan left a comment

Choose a reason for hiding this comment

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

Nice idea @jimexist

@alamb alamb merged commit f7bd7b9 into apache:master May 4, 2021
@jimexist jimexist deleted the refactor-common-build_file_list branch May 5, 2021 01:25
@houqp houqp added api change Changes the API exposed to users of the crate datafusion labels Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api change Changes the API exposed to users of the crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor datafusion/src/physical_plan/common.rs build_file_list to take less param and reuse code

4 participants