Detect when filters on unique constraints make subqueries scalar #8312
Conversation
github-actions
bot
added
logical-expr
Jesse-Bakker
force-pushed
the
scalar-filter
branch
3 times, most recently
from
2f4cb79 to
44383c2
Compare
Jesse-Bakker
changed the title
|
Thank you @Jesse-Bakker -- I plan to review this carefully tomorrow |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @Jesse-Bakker -- this looks great. I had a few suggestions, mostly about comments, and code reuse. But otherwise I think this PR is pretty much ready
cc @liukun4515 and @jackwener who may have some more thoughts about subquery rewrites
|
The only thing I think is strictly needed to approve this PR is docstrings for "is_scalar" -- the rest is "nice to have" from my perspective / could be done as a follow on PR |
Jesse-Bakker
force-pushed
the
scalar-filter
branch
2 times, most recently
from
52fadb7 to
f33a843
Compare
|
I will review this PR and think more about it. If this PR isn't urgent, please wait for me. |
Jesse-Bakker
force-pushed
the
scalar-filter
branch
from
f33a843 to
16db847
Compare
| /// `Filter(b = 2).is_scalar() == false` | ||
| /// and | ||
| /// `Filter(a = 2 OR b = 2).is_scalar() == false` | ||
| fn is_scalar(&self) -> bool { |
There was a problem hiding this comment.
I suggest use Uniform slot and Unique slot to describe FDs instead of scalar.
Commonly used Functional dependencies : including
uniform slot, which means ndv <= 1
unique slot, which means ndv = row
There was a problem hiding this comment.
Do I understand correctly that the suggestion is to rename this method to Filter::is_uniform()?
There was a problem hiding this comment.
Yes, and add these definition into doc/
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @Jesse-Bakker - let's see if @jackwener has a chance to clarify. If we haven't heard by tomorrow I'll merge this PR
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Thanks @Jesse-Bakker @alamb
I have no other question.
|
I took the liberty of merging up from main to resolve a conflict in this PR |
|
Thanks again @Jesse-Bakker and @jackwener |
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
3 participants
Which issue does this PR close?
Part of #3725
Rationale for this change
In some cases, it is possible to prove that a Filter only ever produces one
row. In those cases, such a filter may be used in a scalar subquery to ensure
it is in fact scalar, allowing for more flexible use of scalar subqueries.
What changes are included in this PR?
This adds an
is_scalar()method toFilter, which will check if there is aunique functional dependence which is covered by the
Filter's predicate.This is used in
LogicalPlan::max_rows()to provide a tighter bound on themaximum number of rows returned in the presence of
Filters.Are these changes tested?
This is directly tested with a unit test and with a
sqllogictestthat exercisesthe more flexible use of scalar subqueries.
Are there any user-facing changes?
Scalar subqueries are more flexible. Previous constraints were not documented
as far as I can tell