-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add LogicalPlan::Distinct #2792
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2792 +/- ##
==========================================
+ Coverage 84.94% 85.13% +0.18%
==========================================
Files 272 273 +1
Lines 48221 48293 +72
==========================================
+ Hits 40963 41113 +150
+ Misses 7258 7180 -78
Continue to review full report at Codecov.
|
|
cc @andygrove |
|
Thanks @mrob95. I plan on reviewing this later this week. |
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.
Thanks @mrob95 this looks great. I think you raise a good point about Union containing Vec<LogicalPlan> rather than Vec<Arc<LogicalPlan>> but I think that should be fixed as a separate issue/PR.
|
@mrob95 There is a conflict that needs resolving. I can merge once that is resolved. |
|
Thanks, should be fixed now 🙂 |
|
Filed #2816 to track the |
Which issue does this PR close?
Closes #2573.
Rationale for this change
What changes are included in this PR?
Adds a new type of logical plan for
DISTINCToperations, which is converted into an aggregation during physical planning.Something I am unsure of: all of the
LogicalPlanstructs containArc<LogicalPlan>where they refer to other plans, exceptUnion, which has aVec<LogicalPlan>. I think this caused a couple of extraLogicalPlanclones due to needing an ownedLogicalPlanto put into a union. ShouldUnioncontainVec<Arc<LogicalPlan>>instead?Are there any user-facing changes?