Skip to content

Add Filter::try_new with validation - #3796

Merged
alamb merged 12 commits into
apache:masterfrom
andygrove:filter-try-new
Oct 12, 2022
Merged

Add Filter::try_new with validation#3796
alamb merged 12 commits into
apache:masterfrom
andygrove:filter-try-new

Conversation

@andygrove

@andygroveandygrove commented Oct 11, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes#3795

Rationale for this change

Filters should not use aliased expressions

What changes are included in this PR?

  • Implement Filter::try_new and make Filter attributes private to force the use of the constructor in other crates
  • Add accessor methods for Fitler attributes
  • Add check that filter predicate returns a boolean value
  • Remove aliases from filter predicate
  • Update all call sites
  • Update tests

Are there any user-facing changes?

Yes. Filter attributes are now pub(crate) but accessor methods have been added.

@github-actionsgithub-actionsBot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner labels Oct 11, 2022
@andygroveandygrove added the api change Changes the API exposed to users of the crate label Oct 11, 2022
andygrove
andygrove commented Oct 11, 2022
View reviewed changes
Comment threaddatafusion/optimizer/src/projection_push_down.rs

let plan = LogicalPlanBuilder::from(table_scan)
.filter(col("c"))?
.filter(col("c").gt(lit(1)))?

@andygroveandygroveOct 11, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This test was invalid and the new validation checks caught this.

andygrove
andygrove commented Oct 11, 2022
View reviewed changes
Comment threaddatafusion/expr/src/logical_plan/plan.rs
andygrove
andygrove commented Oct 11, 2022
View reviewed changes
Comment threaddatafusion/optimizer/src/simplify_expressions.rs
&plan,
"\
Filter: test.b > Int32(1) AS test.b > Int32(1) AND test.b > Int32(1)\
Filter: test.b > Int32(1)\

@andygroveandygroveOct 11, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The updated tests here demonstrate that we no longer have aliases for filter predicates.

@andygrove
andygrove marked this pull request as ready for review October 11, 2022 14:17
@andygroveandygrove changed the title WIP: Add Filter::try_new with validationAdd Filter::try_new with validationOct 11, 2022
andygrove
andygrove commented Oct 11, 2022
View reviewed changes
Comment threaddatafusion/expr/src/logical_plan/plan.rs
alamb
alamb reviewed Oct 11, 2022
View reviewed changes
Comment threaddatafusion/expr/src/logical_plan/plan.rs Outdated
}
}

// filter predicates should not contain aliased expressions so we remove any aliases here

@alambalambOct 11, 2022

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.

Is the rationale here that the the output of a FilterExpr has the same schema as its input -- since the Filter never changes its input if we find an alias on the FilterExpr then something is likely wrong as it won't change anything?

I would personally prefer to throw an error rather than stripping the aliases and then track down what is adding aliases

@andygroveandygroveOct 12, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

FilterExec::try_new now throws an exception when passed an aliased predicate and the rewrite logic has moved to utils::from_plan which is called from multiple optimizer rules

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

alamb commented Oct 11, 2022

Copy link
Copy Markdown
Contributor

The CI test is unrelated: #3798

alamb
alamb approved these changes Oct 12, 2022
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.

LGTM

Comment threaddatafusion/expr/src/logical_plan/plan.rs Outdated
Comment threaddatafusion/expr/src/logical_plan/plan.rs Outdated
alamb
alamb approved these changes Oct 12, 2022
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.

Let's ship it!

@alamb
alamb merged commit 3af09fb into apache:masterOct 12, 2022
@ursabot

ursabot commented Oct 12, 2022

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = e27e86b and contender = 3af09fb. 3af09fb is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@andygroveandygrove mentioned this pull request Dec 5, 2022
Closed
@andygrove
andygrove deleted the filter-try-new branch January 27, 2023 18:59
@samuelcolvinsamuelcolvin mentioned this pull request Jul 6, 2024
Closed
@alambalamb mentioned this pull request Jul 7, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes
@liukun4515liukun4515Awaiting requested review from liukun4515

Assignees

No one assigned

Labels

api changeChanges the API exposed to users of the cratecoreCore DataFusion cratelogical-exprLogical plan and expressionsoptimizerOptimizer rulessqlSQL Planner

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Filter predicates should not be aliased

3 participants

@andygrove@alamb@ursabot