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

JoinType and related structures to are used in physical_plan and I would like to avoid physical_plan depending on the logical plans

What changes are included in this PR?

  1. Move JoinType and JoinCondition to 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 core Core DataFusion crate logical-expr Logical plan and expressions labels Jun 6, 2023
use crate::arrow::datatypes::TimeUnit;
use crate::error::{DataFusionError, Result};
use crate::execution::{context::TaskContext, memory_pool::MemoryConsumer};
use crate::logical_expr::JoinType;
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 point of this PR is to remove the logical_expr uses from physical_plan

@alamb alamb changed the title Move JoinType and JoinCondition to datafusion_common Move JoinType and JoinCondition to datafusion_common Jun 6, 2023
@alamb alamb marked this pull request as ready for review June 7, 2023 11:03
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.

LGTM

@alamb alamb merged commit cbfb7c8 into apache:main Jun 7, 2023
@alamb
Copy link
Contributor Author

alamb commented Jun 7, 2023

Thank you for the review @Dandandan !

@alamb alamb deleted the alamb/move_join_type branch June 7, 2023 13:20
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.

2 participants