Skip to content

Expose and document a simpler public API for simplify expressions - #3719

Merged
alamb merged 3 commits into
apache:masterfrom
ygf11:simplify_expr
Oct 7, 2022
Merged

Expose and document a simpler public API for simplify expressions#3719
alamb merged 3 commits into
apache:masterfrom
ygf11:simplify_expr

Conversation

@ygf11

@ygf11ygf11 commented Oct 5, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#3709.

Rationale for this change

This api is very useful for other projects which depends on datafusion, like filter pushdown, default-value when insert etc.

What changes are included in this PR?

Expose and document a simpler public API for simplify expressions

Are there any user-facing changes?

Added a public method simplify_expr.

@github-actionsgithub-actionsBot added the optimizer Optimizer rules label Oct 5, 2022
ygf11
ygf11 commented Oct 5, 2022
View reviewed changes
Comment threaddatafusion/optimizer/src/simplify_expressions.rs Outdated
/// when performing type coercion.
pub fn simplify_expr(
expr: Expr,
schema: DFSchemaRef,

@ygf11ygf11Oct 5, 2022

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.

Since SimplifyContext::new needs a Vec<&'Arc<DFSchema>, so I change the input schema to Arc<DFSchema> to avoid clone.

@alambalambOct 5, 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.

I think this makes sense -- the SimplifyContext constructor is somewhat akward

@alambalambOct 6, 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.

While working on #3741 I found having a signature of schema: &DFSchemaRef was actually more ergnonmic. What do you think?

We have to rerun the CI anyways give the github nonsense earlier today

@ygf11ygf11Oct 6, 2022

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.

Addressed.

@ygf11

ygf11 commented Oct 5, 2022

Copy link
Copy Markdown
ContributorAuthor

@alamb, PTAL.

andygrove
andygrove reviewed Oct 5, 2022
View reviewed changes
Comment threaddatafusion/optimizer/src/simplify_expressions.rs Outdated
alamb
alamb reviewed Oct 5, 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.

Thank you @ygf11 -- this looks great. I think it would be ready to merge after addressing @andygrove 's comments about testing style.

🏆

Comment threaddatafusion/optimizer/src/simplify_expressions.rs Outdated
/// when performing type coercion.
pub fn simplify_expr(
expr: Expr,
schema: DFSchemaRef,

@alambalambOct 5, 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.

I think this makes sense -- the SimplifyContext constructor is somewhat akward

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

Looks great -- thank you @ygf11

@alamb

alamb commented Oct 6, 2022

Copy link
Copy Markdown
Contributor

Github actions is having issues I think -- the CI failures are not related to changes in this PR

@ygf11

ygf11 commented Oct 6, 2022

Copy link
Copy Markdown
ContributorAuthor

Github actions is having issues I think -- the CI failures are not related to changes in this PR

Yes, I see other pr also meets the same ci error 😅.

@alambalamb mentioned this pull request Oct 6, 2022
Merged
3 tasks
@alamb

alamb commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

Thanks @ygf11 -- I may propose some changes to this API as part of working through the examples in #3741.

@alamb
alamb merged commit fef45e7 into apache:masterOct 7, 2022
@ursabot

ursabot commented Oct 7, 2022

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 1e1de82 and contender = fef45e7. fef45e7 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

@ygf11
ygf11 deleted the simplify_expr branch October 7, 2022 15:36
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes
@andygroveandygroveandygrove left review comments

Assignees

No one assigned

Labels

optimizerOptimizer rules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Expose and document a simpler public API for simplify expressions

4 participants

@ygf11@alamb@ursabot@andygrove