Skip to content

Move LogicalPlan enum to datafusion-expr crate - #2294

Merged
andygrove merged 4 commits into
apache:masterfrom
andygrove:move-logical-plan-1-of-n
Apr 21, 2022
Merged

Move LogicalPlan enum to datafusion-expr crate#2294
andygrove merged 4 commits into
apache:masterfrom
andygrove:move-logical-plan-1-of-n

Conversation

@andygrove

@andygroveandygrove commented Apr 20, 2022

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes#2245

Rationale for this change

We want to move the logical plan to the expr crate so that we can implement subquery expressions (this requires introducing new expressions that reference logical plans).

What changes are included in this PR?

  • Mostly just moving code between crates
  • Removed the FromStr implementation on FileType in the SQL parser and converted into parse_file_type method. This is more consistent with other parsing code and fixed a compilation issue.
  • Ignored the rustdoc documentation on some methods in LogicalPlan. See follow-on issue for more details.

Follow-on issues

Are there any user-facing changes?

Yes - code has moved around so this is an API change. I did try to minimize the impact by re-exporting types from their previous location.

@andygrove

Copy link
Copy Markdown
MemberAuthor

AMD64 test failure seems unrelated. Maybe an unreliable test?

thread 'sql::explain_analyze::explain_analyze_baseline_metrics' panicked at 'plan: GlobalLimitExec: limit=3, metrics=[output_rows=1, elapsed_compute=NOT RECORDED, spill_count=0, spilled_bytes=0, mem_used=0]
', datafusion/core/tests/sql/explain_analyze.rs:145:13
failures:
sql::explain_analyze::explain_analyze_baseline_metrics
```

@@ -0,0 +1,23 @@
// Licensed to the Apache Software Foundation (ASF) under one

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if making this a separate crate like datafusion-logical-plan might be considered? Is there any reason the logical plan structs need to be in the expr? I could (potentially) see a use of just Exprs without also LogicalPlan

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I was working towards having the logical plan and logical expressions in the same create (currently named datafusion-expr but potentially could be renamed to datafusion-logical-plan).

The main motivation is that I was planning on adding something like:

pubenumExpr{Exists{subquery:Arc<LogicalPlan>,negated:bool}
...
}

This would be similar to Apache Spark:

caseclassSubquery(child: LogicalPlan, correlated: Boolean) extendsOrderPreservingUnaryNode {

Also similar to Calcite:

publicclassRexSubQueryextendsRexCall {
publicfinalRelNoderel;

I'm not sure how else we would model this but am open to suggestions.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jackwener@xudong963@mingmwang@paveltiunov fyi ☝️ this is related to conversations you have all been involved in previously so would like to get your feedback on this PR if possible

@andygroveandygrove changed the title Move some logical plan types to datafusion-expr crateMove LogicalPlan enum to datafusion-expr crateApr 21, 2022
@andygroveandygrove added the api change Changes the API exposed to users of the crate label Apr 21, 2022

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

YOLO! Let's do this

@andygrove
andygrove merged commit 23f1c77 into apache:masterApr 21, 2022
@andygrove
andygrove deleted the move-logical-plan-1-of-n branch April 21, 2022 16:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api changeChanges the API exposed to users of the crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move LogicalPlan to datafusion-expr crate

2 participants

@andygrove@alamb