Skip to content

Conversation

@tshauck
Copy link
Contributor

@tshauck tshauck commented Aug 21, 2023

Which issue does this PR close?

Closes #7304

Rationale for this change

This fills out the expr and UDF pages a bit. I tried to cover enough of the UDFs so that I could link to it from the expr page. For the expr page, it doesn't match the outline exactly -- I probably have some blindspots, so certainly open to feedback.

What changes are included in this PR?

  • Scalar part of UDF library user page
  • Expr library user page

Are these changes tested?

Manually built the docs to review output.

image

Are there any user-facing changes?

Yes

@alamb
Copy link
Contributor

alamb commented Aug 23, 2023

Thanks @tshauck -- I will try and read this carefully either later today or tomorrow. I am very excited to do so

@alamb alamb changed the title docs: fill out expr page docs: Add Expr library developer page Aug 24, 2023
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @tshauck -- this was a great read. I had a few minor suggestions, but otherwise I think this PR is ready to go.

I think this kind of content will help future users of DataFusion a lot. Thank you so much for writing it ❤️

// recurse down and optimize children first
let optimized_plan = utils::optimize_children(self, plan, config)?;

match optimized_plan {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use plan.expressions() and then from_plan: https://docs.rs/datafusion/latest/datafusion/logical_expr/utils/fn.from_plan.html to rewrite expressions anywhere they appear.

It took me a while to find "from_plan" -- I will propose moving it to LogicalPlan as a method.

Copy link
Contributor Author

@tshauck tshauck Aug 25, 2023

Choose a reason for hiding this comment

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

Thanks for this, it seems to clean things up nicely, though please lmk if I'm not using something correctly. Also, I updated the code to use the new method proposed in #7396... if that doesn't go in I can update this back.

@tshauck
Copy link
Contributor Author

tshauck commented Aug 24, 2023

Thanks for the review! -- I've committed the quick changes and'll have a look at the rest over the next few days. I'll follow up when that's ready for review.

@tshauck
Copy link
Contributor Author

tshauck commented Aug 25, 2023

I think this is ready for review, thanks! cc/ @alamb

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks great -- thank you so much @tshauck


let inputs = plan.inputs().into_iter().cloned().collect::<Vec<_>>();

let plan = plan.with_new_exprs(&new_expressions, &inputs);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alamb alamb merged commit d059dd3 into apache:main Aug 25, 2023
@alamb alamb added documentation Improvements or additions to documentation devrel labels Aug 25, 2023
@tshauck tshauck deleted the expr-udf branch August 25, 2023 17:17
@andygrove andygrove added the enhancement New feature or request label Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Library Guide: Add Working with Exprs

3 participants