-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add documentation example for AggregateExprBuilder
#15504
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
Very nice example, thank you @Shreyaskr1409. Perhaps we can make this in datafusion-examples as well? |
@berkaysynnada thank you, actually I did use the datafusion-examples as reference.
Yeah that could also be done with maybe a few minor adjustments. If you want, I can do that as well once I have the active conversations reviewed and resolved. |
I think it would be good to avoid having duplicated content in datafusion-examples and the normal documentation. I personally think the documentation site is more accessable (more chance of someone reading / finding it via search) than rust examples in the repository |
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 @Shreyaskr1409 -- this is a great start. I left a suggestion on how to make a doc example without dependency -- let me know if it makes sense
@alamb Thank you so much, I have resolved all the conversations. Everything is working and tested, and even I got to learn new stuff from this. |
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 @Shreyaskr1409 -- I think this PR looks pretty good now, though the example could be shortened significantly.
We could also do that as a follow on PR
I also merged up from main to fix the CO |
@alamb thank you |
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.
Love it @Shreyaskr1409 ❤️
* Add formatting changes and fix the merge issue * Fix formatting * Fixed a faulty statement * Folded irrelevant sections from example * Fix formatting * Add unimplemented!() --------- Co-authored-by: Andrew Lamb <[email protected]>
Which issue does this PR close?
AggregateExprBuilder
#15369.Rationale for this change
Adding a documentation example for
AggregateExprBuilder
making it easier for users to follow along while creating an AggregateExpr.What changes are included in this PR?
Changes to physical-expr/src/aggregate.rs
These changes are majorly documentation additions to aggregate.rs
Are these changes tested?
AggregateUDF creation example is tested.
AggregateFunctionExpr creation example is not tested. (I will be adding the issue I am facing in comments below)
I really appreciate any help, even regarding making the documentation more detailed.
Are there any user-facing changes?
Yes they are. They can see documentation example for AggregateExprBuilder in rust docs.