Skip to content

Detect when filters on unique constraints make subqueries scalar - #8312

Merged
alamb merged 2 commits into
apache:mainfrom
Jesse-Bakker:scalar-filter
Dec 6, 2023
Merged

Detect when filters on unique constraints make subqueries scalar#8312
alamb merged 2 commits into
apache:mainfrom
Jesse-Bakker:scalar-filter

Conversation

@Jesse-Bakker

@Jesse-BakkerJesse-Bakker commented Nov 23, 2023

Copy link
Copy Markdown
Contributor

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 to Filter, which will check if there is a
unique functional dependence which is covered by the Filter's predicate.
This is used in LogicalPlan::max_rows() to provide a tighter bound on the
maximum number of rows returned in the presence of Filters.

Are these changes tested?

This is directly tested with a unit test and with a sqllogictest that exercises
the 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

@github-actionsgithub-actionsBot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Nov 23, 2023
@Jesse-Bakker
Jesse-Bakkerforce-pushed the scalar-filter branch 3 times, most recently from 2f4cb79 to 44383c2CompareNovember 23, 2023 09:28
@Jesse-BakkerJesse-Bakker changed the title Detect when filters make subqueries scalarDetect when filters on unique constraints make subqueries scalarNov 23, 2023
@alamb

alamb commented Nov 27, 2023

Copy link
Copy Markdown
Contributor

Thank you @Jesse-Bakker -- I plan to review this carefully tomorrow

@alambalamb mentioned this pull request Nov 27, 2023
Closed
8 tasks
alamb
alamb reviewed Nov 28, 2023
View reviewed changes

@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.

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

Comment threaddatafusion/expr/src/logical_plan/plan.rs Outdated
Comment threaddatafusion/expr/src/logical_plan/plan.rs
Comment threaddatafusion/expr/src/logical_plan/plan.rs Outdated
@alamb

alamb commented Nov 28, 2023

Copy link
Copy Markdown
Contributor

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
Jesse-Bakkerforce-pushed the scalar-filter branch 2 times, most recently from 52fadb7 to f33a843CompareNovember 29, 2023 08:57
@Jesse-BakkerJesse-Bakker mentioned this pull request Nov 29, 2023
Merged
@jackwener
jackwener self-requested a review November 29, 2023 09:35
@jackwener

jackwener commented Nov 30, 2023•
edited
Loading

Copy link
Copy Markdown
Member

unique constraints and function dependences is complex feature (Worthy of being carefully designed), I prepare to think more about it.

I will review this PR and think more about it. If this PR isn't urgent, please wait for me.

jackwener
jackwener approved these changes Dec 2, 2023
View reviewed changes

@jackwenerjackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @Jesse-Bakker

Comment threaddatafusion/expr/src/logical_plan/plan.rs
/// `Filter(b = 2).is_scalar() == false`
/// and
/// `Filter(a = 2 OR b = 2).is_scalar() == false`
fn is_scalar(&self) -> bool {

@jackwenerjackwenerDec 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@Jesse-BakkerJesse-BakkerDec 4, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Do I understand correctly that the suggestion is to rename this method to Filter::is_uniform()?

@jackwenerjackwenerDec 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, and add these definition into doc/

Comment threaddatafusion/expr/src/logical_plan/plan.rs
@alambalamb mentioned this pull request Dec 5, 2023
Closed
7 tasks
alamb
alamb approved these changes Dec 5, 2023
View reviewed changes

@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.

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

Comment threaddatafusion/expr/src/logical_plan/plan.rs
jackwener
jackwener approved these changes Dec 6, 2023
View reviewed changes

@jackwenerjackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @Jesse-Bakker@alamb

I have no other question.

@alamb

alamb commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

I took the liberty of merging up from main to resolve a conflict in this PR

@alamb
alamb merged commit c8e1c84 into apache:mainDec 6, 2023
@alamb

alamb commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

Thanks again @Jesse-Bakker and @jackwener

appletreeisyellow pushed a commit to appletreeisyellow/datafusion that referenced this pull request Dec 15, 2023
@Jesse-Bakker@alamb@appletreeisyellow
Detect when filters make subqueries scalar (apache#8312)
304d7ae
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@appletreeisyellowappletreeisyellow mentioned this pull request Dec 15, 2023
Closed
@matthewgappmatthewgapp mentioned this pull request Jan 11, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes
@jackwenerjackwenerjackwener approved these changes

Assignees

No one assigned

Labels

logical-exprLogical plan and expressionssqllogictestSQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Jesse-Bakker@alamb@jackwener