You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/04/03 11:08:08 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #5842: Move `TransactionStart`/`TransactionEnd`/`SetVariable` into `LogicalPlan::Statement`
alamb opened a new pull request, #5842:
URL: https://github.com/apache/arrow-datafusion/pull/5842
# Which issue does this PR close?
Follow on to https://github.com/apache/arrow-datafusion/pull/5827 from @avantgardnerio
related to https://github.com/apache/arrow-datafusion/issues/3349
# Rationale for this change
As I have been working on documenting datafusion (see #5499), the number of distinct variants of LogicalPlan is confusing. As I was reviewing https://github.com/apache/arrow-datafusion/pull/5827 I realized it made the problem (slightly) worse and figured it was time to set us on a more sustainable course.
I believe we need to invest to help make the code base easier to understand and extend.
# What changes are included in this PR?
1. Adding another layer of indirection in `LogicalPlan::Statement` and (start) gathering statements together
# Are these changes tested?
Existing tests
# Are there any user-facing changes?
If anyone used `SetVariable` directly they will now have to update their code slightly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #5842: Move `TransactionStart`/`TransactionEnd`/`SetVariable` into `LogicalPlan::Statement`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5842:
URL: https://github.com/apache/arrow-datafusion/pull/5842
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5842: Move `TransactionStart`/`TransactionEnd`/`SetVariable` into `LogicalPlan::Statement`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5842:
URL: https://github.com/apache/arrow-datafusion/pull/5842#discussion_r1155817155
##########
datafusion/expr/src/logical_plan/mod.rs:
##########
@@ -19,16 +19,20 @@ pub mod builder;
pub mod display;
mod extension;
mod plan;
+mod statement;
pub use builder::{table_scan, LogicalPlanBuilder};
pub use plan::{
Aggregate, Analyze, CreateCatalog, CreateCatalogSchema, CreateExternalTable,
- CreateMemoryTable, CreateView, CrossJoin, DescribeTable, Distinct, DmlStatement,
- DropTable, DropView, EmptyRelation, Explain, Extension, Filter, Join, JoinConstraint,
- JoinType, Limit, LogicalPlan, Partitioning, PlanType, Prepare, Projection,
- Repartition, SetVariable, Sort, StringifiedPlan, Subquery, SubqueryAlias, TableScan,
- ToStringifiedPlan, TransactionAccessMode, TransactionConclusion, TransactionEnd,
- TransactionIsolationLevel, TransactionStart, Union, Unnest, Values, Window, WriteOp,
+ CreateMemoryTable, CreateView, CrossJoin, DescribeTable, Distinct, DropTable,
Review Comment:
this makes the imports consistent
##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -1157,21 +1157,11 @@ impl DefaultPhysicalPlanner {
"Unsupported logical plan: Dml".to_string(),
))
}
- LogicalPlan::TransactionStart(_) => {
+ LogicalPlan::Statement(statement) => {
Review Comment:
this is the key difference -- collapse several LogicalPlan variants into one that all have the same behavior
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1226,18 +1200,6 @@ pub struct DropView {
pub schema: DFSchemaRef,
}
-/// Set a Variable's value -- value in
Review Comment:
moved into `statement.rs`
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1559,83 +1521,6 @@ impl Hash for CreateExternalTable {
}
}
-#[derive(Clone, PartialEq, Eq, Hash)]
Review Comment:
Moved to statement.rs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org