-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Documentation: Plan custom expressions #15353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @Jiashu-Hu -- this looks great.
I have some suggestions on how to improve this section and example, but we could also do it as a follow on PR.
It is great to see the docs being improved
| ctx.register_expr_planner(Arc::new(MyCustomPlanner))?; | ||
| let results = ctx.sql("select 'foo'->'bar';").await?.collect().await?; | ||
|
|
||
| pretty::print_batches(&results)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please change this to use `assert_batches_eq! so the actual output is in the test and it is tested in CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi alamb, Thank you very much for your review! Yes, absolutely, I've updated it with an assert_batches_eq! in the latest commit.
Co-authored-by: Andrew Lamb <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
added assert for CI process
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @Jiashu-Hu
* Added Custom Expression Planning Section * improve syntax * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update adding-udfs.md added assert for CI process * formatted language request by prettier --------- Co-authored-by: Andrew Lamb <[email protected]>
* Added Custom Expression Planning Section * improve syntax * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update docs/source/library-user-guide/adding-udfs.md Co-authored-by: Andrew Lamb <[email protected]> * Update adding-udfs.md added assert for CI process * formatted language request by prettier --------- Co-authored-by: Andrew Lamb <[email protected]>
Which issue does this PR close?
Rationale for this change
This PR adds documentation for using the
ExprPlannerAPI to plan custom expressions, as requested in #15267. Clear documentation with an example will help users extend DataFusion to support custom operators like PostgreSQL's->.What changes are included in this PR?
## Custom Expression Planningto documentExprPlannerdoes and why it’s useful.ExprPlannerto make the->operator concatenate strings (e.g.,'foo'->'bar'outputsfoobar).ExprPlannerandFunctionRegistry) for further reading.Are these changes tested?
yes
Are there any user-facing changes?
Yes, since this is an documentation chang