-
Notifications
You must be signed in to change notification settings - Fork 163
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
[CHORE] Remove existing LogicalPlan from all execution concepts #1208
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1208 +/- ##
==========================================
- Coverage 88.41% 88.31% -0.11%
==========================================
Files 55 55
Lines 5620 5630 +10
==========================================
+ Hits 4969 4972 +3
- Misses 651 658 +7
|
daft/execution/execution_step.py
Outdated
@@ -5,6 +5,8 @@ | |||
from dataclasses import dataclass, field | |||
from typing import Generic, TypeVar | |||
|
|||
import fsspec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be behind a TYPE_CHECKING
guard since fsspec
is an optional dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, done!
daft/execution/physical_plan.py
Outdated
@@ -17,8 +17,10 @@ | |||
from collections import deque | |||
from typing import Generator, Iterator, TypeVar, Union | |||
|
|||
import fsspec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🙌
Currently, a lot of query fragment executors are parameterized by a LogicalPlan node object, since it holds relevant subfields. This is not necessary; the executors can be parameterized by the subfields directly.
This PR removes LogicalPlan from all query execution concepts.