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

ARROW-9758 [Rust] [DataFusion] Rename PhysicalPlannerImpl to DefaultPhysicalPlanner #7980

Closed
wants to merge 1 commit into from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Aug 17, 2020

Proposed follow up to #7975 -- rename PhysicalPlannerImpl to DefaultPhysicalPlanner to better describe what it is and that the design allows for more than one.

I considered a more specific name like SingleNodeMultiThreadedPlanner but I felt a lot of that was a function of the Physical nodes that got created rather than the planner itself.

@github-actions
Copy link

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -33,7 +33,8 @@ use arrow::{
};
use udf::ScalarFunction;

/// Physical query planner
/// Physical query planner that converts a `LogicalPlan` to an
/// `ExecutionPlan` suitable for execution.
Copy link
Member

Choose a reason for hiding this comment

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

Note that in the rustdoc format we can actually link to other items. e.g. [ExecutionPlan](trait.ExecutionPlan.html)

@andygrove andygrove closed this in 18181fe Aug 17, 2020
@jorisvandenbossche jorisvandenbossche changed the title ARROW-9768 [Rust] [DataFusion] Rename PhysicalPlannerImpl to DefaultPhysicalPlanner ARROW-9758 [Rust] [DataFusion] Rename PhysicalPlannerImpl to DefaultPhysicalPlanner Aug 17, 2020
emkornfield pushed a commit to emkornfield/arrow that referenced this pull request Sep 8, 2020
…hysicalPlanner

Proposed follow up to  apache#7975 -- rename `PhysicalPlannerImpl` to `DefaultPhysicalPlanner` to better describe what it is and that the design allows for more than one.

I considered a more specific name like `SingleNodeMultiThreadedPlanner` but I felt a lot of that was a function of the Physical nodes that got created rather than the planner itself.

Closes apache#7980 from alamb/alamb/arrow-9758-rename

Authored-by: alamb <andrew@nerdnetworks.org>
Signed-off-by: Andy Grove <andygrove73@gmail.com>
emkornfield pushed a commit to emkornfield/arrow that referenced this pull request Oct 16, 2020
…hysicalPlanner

Proposed follow up to  apache#7975 -- rename `PhysicalPlannerImpl` to `DefaultPhysicalPlanner` to better describe what it is and that the design allows for more than one.

I considered a more specific name like `SingleNodeMultiThreadedPlanner` but I felt a lot of that was a function of the Physical nodes that got created rather than the planner itself.

Closes apache#7980 from alamb/alamb/arrow-9758-rename

Authored-by: alamb <andrew@nerdnetworks.org>
Signed-off-by: Andy Grove <andygrove73@gmail.com>
@alamb alamb deleted the alamb/arrow-9758-rename branch October 26, 2020 20:05
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
…hysicalPlanner

Proposed follow up to  apache#7975 -- rename `PhysicalPlannerImpl` to `DefaultPhysicalPlanner` to better describe what it is and that the design allows for more than one.

I considered a more specific name like `SingleNodeMultiThreadedPlanner` but I felt a lot of that was a function of the Physical nodes that got created rather than the planner itself.

Closes apache#7980 from alamb/alamb/arrow-9758-rename

Authored-by: alamb <andrew@nerdnetworks.org>
Signed-off-by: Andy Grove <andygrove73@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants