Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Jun 6, 2023

Which issue does this PR close?

Part of #1754 (see #1754 (comment))

Rationale for this change

I am trying to extract physical_plan into its own crate to improve modularity and compile times

DisplayablePlan and related structures to are used in physical_plan and thus they need to be out of datafusion core to pull out the physical plan:

What changes are included in this PR?

  1. Move PlanType, StringifiedPlan and ToStringifiedPlan datafusion_common
  2. Update use paths

Are these changes tested?

Existing tests

Are there any user-facing changes?

No, I left a backwards compatible pub use

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Jun 6, 2023
@alamb alamb marked this pull request as ready for review June 6, 2023 21:04
@github-actions github-actions bot added the core Core DataFusion crate label Jun 6, 2023
@alamb alamb marked this pull request as draft June 6, 2023 21:04
use std::fmt;

use crate::logical_expr::{StringifiedPlan, ToStringifiedPlan};
use datafusion_common::display::{StringifiedPlan, ToStringifiedPlan};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the whole point of this PR is to remove the logical_expr from the use statements in physical_plan

@alamb alamb changed the title Move StringifiedPlan to datafusion_common Move PlanType, StringifiedPlan and ToStringifiedPlan datafusion_common Jun 6, 2023
@alamb alamb marked this pull request as ready for review June 7, 2023 11:03
@alamb alamb changed the title Move PlanType, StringifiedPlan and ToStringifiedPlan datafusion_common Minor: Move PlanType, StringifiedPlan and ToStringifiedPlan datafusion_common Jun 8, 2023
Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

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

Make sense to me, thanks @alamb

Comment on lines +45 to +46
// backwards compatibility
pub use datafusion_common::display::{PlanType, StringifiedPlan, ToStringifiedPlan};
Copy link
Member

Choose a reason for hiding this comment

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

For such things, I'm wondering as no deprecated message, it is possibly we can remove this kind of compatibility later?

Copy link
Contributor Author

@alamb alamb Jun 10, 2023

Choose a reason for hiding this comment

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

I tried to add a deprecated message (that could point people to use the new location) but it didn't seem to work.

I think we can remove the pub use whenever we want, it will simply mean users of the crate will have to update their pub uses (and will technically be a breaking API change)

I guess I am hoping that over time we can remove some of the old pub use but we don't really have a structured plan for doing so. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yea, for such re-exported things, seems it is impossible to add deprecated message. So the only way is just to remove this kind of old APIs after some time (e.g., few releases?).

@alamb alamb merged commit f0bf016 into apache:main Jun 11, 2023
jayzhan211 pushed a commit to jayzhan211/datafusion that referenced this pull request Jun 12, 2023
…tafusion_common` (apache#6571)

* Move DisplayablePlan to `datafusion_common`

* Update uses

* Update datafusion/common/src/display.rs

Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>

---------

Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants