Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Jun 1, 2023

Which issue does this PR close?

This is part of #1754

Rationale for this change

While trying to extract physical_plan out of the core datafusion crate, the implementation of the various formats need to instantiate ExecutionPlans (e.g. ParquetExec) -- so therefore I conclude I need to remove the dependency out of physical_plan

What changes are included in this PR?

  1. Move physical_plan::file_format to datasource::physical_plan

Are these changes tested?

Existing tests

Are there any user-facing changes?

Yes, this does change the import path for anyone who uses structures like ParquetExec, etc

from

use crate::physical_plan::file_format::{ParquetExec, FileScanConfig};

To

use crate::datasource::physical_plan::{ParquetExec, FileScanConfig};

I would be open to other suggestions too.

Sorry in advance

@alamb alamb added the api change Changes the API exposed to users of the crate label Jun 1, 2023
@github-actions github-actions bot added core Core DataFusion crate substrait Changes to the substrait crate labels Jun 1, 2023
@alamb alamb marked this pull request as ready for review June 1, 2023 15:30
@alamb alamb changed the title Move physical_plan::file_format to datasource::plan RFC: Move physical_plan::file_format to datasource::plan Jun 1, 2023
@alamb
Copy link
Contributor Author

alamb commented Jun 5, 2023

After a bunch of thought, I am convinced this is the right next step towards splitting out physical plan

Thus I plan to update and merge this PR after #6526 from @mustafasrepo is merged

@alamb alamb changed the title RFC: Move physical_plan::file_format to datasource::plan Move physical_plan::file_format to datasource::plan Jun 5, 2023
@alamb
Copy link
Contributor Author

alamb commented Jun 5, 2023

I think after this PR I can move physical_plan out of datafusion core

I will then work on extracting datafusion_physical_optimizer out as well

@alamb alamb force-pushed the alamb/consolidate_file_format_plans branch from ca9f38c to 843cc54 Compare June 6, 2023 19:56
@alamb alamb force-pushed the alamb/consolidate_file_format_plans branch from 843cc54 to 14229be Compare June 6, 2023 19:58
@alamb
Copy link
Contributor Author

alamb commented Jun 6, 2023

Since this has a substantial chance of conflicts I am going to merge it in now to avoid them as much as possible

@alamb alamb merged commit 786f222 into apache:main Jun 6, 2023
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 core Core DataFusion crate substrait Changes to the substrait crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants