Skip to content

Fix Predicate Pushdown: split_members should be able to split aliased predicate - #1368

Merged
Dandandan merged 5 commits into
apache:masterfrom
viirya:issue_1367
Nov 28, 2021
Merged

Fix Predicate Pushdown: split_members should be able to split aliased predicate#1368
Dandandan merged 5 commits into
apache:masterfrom
viirya:issue_1367

Conversation

@viirya

@viiryaviirya commented Nov 26, 2021

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes#1367.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@viirya

viirya commented Nov 26, 2021

Copy link
Copy Markdown
MemberAuthor

cc @Dandandan

Dandandan
Dandandan reviewed Nov 27, 2021
View reviewed changes
Comment threaddatafusion/src/optimizer/constant_folding.rs Outdated
Projection: #test.a\
\n Filter: NOT #test.c AS test.c = Boolean(false)\
\n Filter: #test.b AS test.b = Boolean(true)\
\n Filter: NOT #test.c\

@DandandanDandandanNov 27, 2021

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.

👍

@Dandandan

Dandandan commented Nov 27, 2021

Copy link
Copy Markdown
Contributor

thanks @viirya for the quick change / PR! I think this is a good change.

I am wondering if there's more places where we don't want to add an alias.

Also we probably need some more tests to cover filter pushdown in combination with other optimization passes like this one.

Another thing to consider might be for the filter operation to ignore the alias.

FYI @alamb WDYT

alamb
alamb reviewed Nov 27, 2021
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.

I don't really understand how aliases can mess up filter push down (perhaps filter pushdown should be stripping off the aliases)?

Comment threaddatafusion/src/optimizer/constant_folding.rs Outdated
} else {
Ok(new_e)
match plan {
LogicalPlan::Filter { .. } => Ok(new_e),

@alambalambNov 27, 2021

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 would like to recommend adding a comment here about why we don't add aliases for filter.

@alamb

alamb commented Nov 27, 2021

Copy link
Copy Markdown
Contributor

Yeah, I suggest:

  1. Adding a test with the TPCH explain plans (so we can detect such errors earlier in the future)
  2. Looking into what is going on with Filter pushdown that aliases disabled them.

I "feel" like there should be nothing wrong with aliasing the output of a Filter

@viirya

viirya commented Nov 28, 2021

Copy link
Copy Markdown
MemberAuthor

Thanks. I'm adding a test for TPCH plans. I will take a look on Filter pushdown with aliases later.

@viirya

viirya commented Nov 28, 2021

Copy link
Copy Markdown
MemberAuthor

Currently the filter_push_down rule fails on TPCH q10 with aliased on Filter because split_members method doesn't address Alias so a predicate like #orders.o_orderdate >= Date32("8674") AND #orders.o_orderdate < Date32("8766") AND #lineitem.l_returnflag = Utf8("R") AS ... will not be split so it won't be pushed down.

By adding Alias into split_members, the rule can push down aliased predicate.

@viiryaviirya changed the title Skip adding aliases for Filtersplit_members should be able to split aliased predicateNov 28, 2021
Dandandan
Dandandan reviewed Nov 28, 2021
View reviewed changes
Comment threaddatafusion/tests/sql.rs
}

#[tokio::test]
async fn tpch_explain_q10() -> Result<()> {

@DandandanDandandanNov 28, 2021

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.

Nice 👍
Maybe it would be good to have those in the tpch crate instead?
In that case we could include some / all other queries as well.

@alambalambNov 28, 2021

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 agree -- filed #1377 to track this

@alambalamb mentioned this pull request Nov 28, 2021
Closed
alamb
alamb approved these changes Nov 28, 2021
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 @viirya !

Would it be possible to move the tpch tests into the tpch crate, as suggested by @Dandandan as part of this PR?

Someone can add explain plans for the other TPCH queries as part of #1377 so I don't think that is necessary.

Thanks for sticking with this one and finding the elegant solution

Comment threaddatafusion/src/optimizer/filter_push_down.rs
split_members(left, predicates);
split_members(right, predicates);
}
Expr::Alias(expr, _) => {

@alambalambNov 28, 2021

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.

💯 excellent

Comment threaddatafusion/tests/sql.rs
}

#[tokio::test]
async fn tpch_explain_q10() -> Result<()> {

@alambalambNov 28, 2021

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 agree -- filed #1377 to track this

Comment threaddatafusion/tests/sql.rs
\n Join: #customer.c_custkey = #orders.o_custkey\
\n TableScan: customer projection=Some([0, 1, 2, 3, 4, 5, 7])\
\n Filter: #orders.o_orderdate >= Date32(\"8674\") AND #orders.o_orderdate < Date32(\"8766\")\
\n TableScan: orders projection=Some([0, 1, 4]), filters=[#orders.o_orderdate >= Date32(\"8674\"), #orders.o_orderdate < Date32(\"8766\")]\

@alambalambNov 28, 2021

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.

so lovely to see those filters pushed down ❤️

@alambalamb changed the title split_members should be able to split aliased predicateFix Predicate Pushdown: split_members should be able to split aliased predicateNov 28, 2021
@viirya

viirya commented Nov 28, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Thanks @alamb@Dandandan ! Yeah, if I find some time later to (or someone can) add tests for the other TPCH queries, we can move the test together.

@Dandandan
Dandandan self-requested a review November 28, 2021 18:39
@Dandandan
Dandandan merged commit 15db291 into apache:masterNov 28, 2021
@houqphouqp added the bug Something isn't working label Nov 29, 2021
@alambalamb mentioned this pull request Nov 30, 2021
Merged
@houqphouqp added the performance Make DataFusion faster label Dec 2, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DandandanDandandanDandandan approved these changes
@alambalambalamb approved these changes

Assignees

No one assigned

Labels

bugSomething isn't workingperformanceMake DataFusion faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TPC-H q10 performance regression (expression for filter with added alias is not pushed down)

4 participants

@viirya@Dandandan@alamb@houqp