Skip to content

Minor: Simplify conjunction and disjunction, improve docs#10446

Merged
comphead merged 2 commits intoapache:mainfrom
alamb:alamb/simplify_conjucntion
May 13, 2024
Merged

Minor: Simplify conjunction and disjunction, improve docs#10446
comphead merged 2 commits intoapache:mainfrom
alamb:alamb/simplify_conjucntion

Conversation

@alamb
Copy link
Copy Markdown
Contributor

@alamb alamb commented May 10, 2024

Which issue does this PR close?

N/A

Rationale for this change

I ran into this code while working on #10291

What changes are included in this PR?

  1. Simplify implementation of conjunction and disjunction
  2. Add some more docs with examples

Are these changes tested?

New doc test and existing CI

Are there any user-facing changes?

@github-actions github-actions bot added the logical-expr Logical plan and expressions label May 10, 2024
/// ```
pub fn conjunction(filters: impl IntoIterator<Item = Expr>) -> Option<Expr> {
filters.into_iter().reduce(|accum, expr| accum.and(expr))
filters.into_iter().reduce(Expr::and)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is the very minor simplification and while I was in here I figured I would improve the docs too

@alamb alamb force-pushed the alamb/simplify_conjucntion branch from c873b4f to 9bc4fbf Compare May 10, 2024 18:41
@alamb alamb marked this pull request as ready for review May 13, 2024 12:01
/// ```
pub fn disjunction(filters: impl IntoIterator<Item = Expr>) -> Option<Expr> {
filters.into_iter().reduce(|accum, expr| accum.or(expr))
filters.into_iter().reduce(Expr::or)
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.

Wow, surprise that this works

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was my first reaction when I saw this in Filter pushdown and it took me a while to figure out what was actually doing 🤯

Copy link
Copy Markdown
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

👍

@comphead comphead merged commit 9cc981b into apache:main May 13, 2024
@alamb alamb deleted the alamb/simplify_conjucntion branch May 13, 2024 15:20
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants