WIP: Generate docs from macros. #12822

comphead · 2024-10-08T21:01:58Z

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

comphead · 2024-10-08T21:04:18Z

@Omega359 @alamb I tried to play with custom attributes to wrap up the documentation on top of the what @Omega359 already built. I'm experimenting with just 2 fields(description and examples) and the attribute has to be placed on top of the struct.

comphead · 2024-10-08T21:04:44Z

Cargo.toml

@@ -48,6 +48,8 @@ members = [
    "datafusion-examples",
    "test-utils",
    "benchmarks",
+    "datafusion/macros",
+    "datafusion/pre-macros"


pre macros needed to have Documentation structure already precreated

if we go with this approach, maybe we can put the doc structure in a crate named specifically such as datafusion/documentation rather than something generic like pre-macros

comphead · 2024-10-08T21:07:50Z

datafusion/functions/src/math/log.rs

@@ -37,6 +38,7 @@ use datafusion_expr::{
 };
 use datafusion_expr::{ScalarUDFImpl, Signature, Volatility};

+#[udf_doc(description = "log_description", example = "log_example")]


here we put the documentation

This looks quite cool ❤️

One thing I would be interested in seeing is if/how a multi line description and example would look. I have had trouble with multi-line macro / docs in the past

comphead · 2024-10-08T21:08:03Z

datafusion/functions/src/math/log.rs

@@ -472,4 +471,16 @@ mod tests {
            SortProperties::Unordered
        );
    }
+
+    #[test]
+    fn test_doc() {


and the test shows it applied

comphead · 2024-10-08T21:58:29Z

WDYT should we move in this direction?

Omega359 · 2024-10-09T13:00:23Z

I find this intriguing and technically very interesting! I do have a few questions:

What are you goals with this? I am assuming cleaner looking/easier to create docs. If so I would wonder how it would help in cases were there is significant amounts of docs vs boilerplate such as in the to_date udf.
Can it be made to handle n arguments?
Is it possible to generate rustdoc with this approach?
I noticed that this approach creates a Documentation object on every call. It's obviously pretty minor but it would be nice if it could be static.

To me in general while I find this quite interesting I wonder if the benefit outweighs the added complexity of the code generation. To be clear I am absolutely not against this approach - I just have a few concerns.

comphead · 2024-10-09T14:51:14Z

I find this intriguing and technically very interesting! I do have a few questions:

What are you goals with this? I am assuming cleaner looking/easier to create docs. If so I would wonder how it would help in cases were there is significant amounts of docs vs boilerplate such as in the to_date udf.

Can it be made to handle n arguments?

Is it possible to generate rustdoc with this approach?

I noticed that this approach creates a Documentation object on every call. It's obviously pretty minor but it would be nice if it could be static.

To me in general while I find this quite interesting I wonder if the benefit outweighs the added complexity of the code generation. To be clear I am absolutely not against this approach - I just have a few concerns.

Thanks @Omega359
The idea is to reuse your current approach but make it on the level of custom Rust attribute instead of having the documentation directly in the code. So the steps can look like:

Move Documentation structure to pre-macros crate
generate the code from macros instead of having it statically
we can have it static everything will be the same as its now, the only difference is the code will be created through the macros.

I'm planning to play with to_date function as it covers also multiline comments as @alamb mentioned

comphead · 2024-10-10T18:42:39Z

@alamb @Omega359 please have a look on real example to_date (I still need to include argements to be called with the builder). As you can see it is the same approach as before, the only difference the documentation is set by attributes rather than code, but it generates documentation builders as before.

Btw multiline seems to be working

comphead · 2024-10-10T18:44:38Z

datafusion/pre-macros/src/lib.rs

+// under the License.
+
+#[derive(Debug, Clone)]
+pub struct DocumentationTest {


I copied all Documentation structures here, added Test so as not to break everything

alamb

I think this looks great personally

As long as the macro has enough documentation itself, I think this would help reduce a bunch of the boiler plate (though perhaps not all)

alamb · 2024-10-11T14:33:38Z

Cargo.toml

@@ -48,6 +48,8 @@ members = [
    "datafusion-examples",
    "test-utils",
    "benchmarks",
+    "datafusion/macros",
+    "datafusion/pre-macros"


if we go with this approach, maybe we can put the doc structure in a crate named specifically such as datafusion/documentation rather than something generic like pre-macros

alamb · 2024-10-11T14:33:55Z

datafusion/functions/src/datetime/to_date.rs

 use std::any::Any;
 use std::sync::OnceLock;

+#[udf_doc(


this is very cool

@Omega359 what do you think?

I dumped the example #[udf_doc ...] into my editor to do a direct comparison to the existing solution and to be honest it definitely is nicer to visually parse.

There are situations currently where this approach will not work, at least as it currently seems to be implemented. For example: bit_and_or_xor, some of the regex udfs, etc.

alamb · 2024-10-11T14:34:07Z

datafusion/macros/Cargo.toml

+
+[package]
+name = "datafusion-macros"
+description = "Proc macros for DataFusion query engine"


Suggested change

description = "Proc macros for DataFusion query engine"

description = "Procedural macros for DataFusion query engine"

comphead · 2024-10-11T16:59:20Z

Thanks @alamb I'll consider the changes, @Omega359 is it good to have a documentation method on struct level instead of impl?

Omega359 · 2024-10-11T17:32:30Z

@Omega359 is it good to have a documentation method on struct level instead of impl?

I can't think of a reason why not off the top of my head. I'm hoping to have time to look over your latest updates in this PR later tonight

Omega359 · 2024-10-11T17:37:01Z

datafusion/functions/src/datetime/to_date.rs

 use std::any::Any;
 use std::sync::OnceLock;

+#[udf_doc(
+    doc_section(include = "true", label = "Time and Date Functions"),


I worry about this since it's almost certainly going to drift at some point in the future via typos, etc. Is it possible to reference the const's in scalar_doc_sections/window_doc_sections/aggregate_doc_sections ?

I'm not sure if enum/constant can be used directly within the attribute, however there is a possibility to check the value in macro. So if the value is not satisfied to existing enum variants the macro can throw an exception with explaining what section names are supported

Omega359 · 2024-10-11T17:38:47Z

datafusion/functions/src/datetime/to_date.rs

+---------------------------------------------------------------+\n\
+```\n\n\
+Additional examples can be found [here](https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/to_date.rs)",
+    standard_argument(name = "expression", expression_type = "String"),


This is likely better as expression_type -> prefix

Omega359 · 2024-10-11T17:40:11Z

datafusion/macros/src/lib.rs

+    let mut syntax_example: Option<LitStr> = None;
+    let mut sql_example: Option<LitStr> = None;
+    let mut standard_args: Vec<(Option<LitStr>, Option<LitStr>)> = vec![];
+    let mut udf_args: Vec<(Option<LitStr>, Option<LitStr>)> = vec![];


might be missing related udfs here.

comphead · 2024-10-11T19:44:46Z

Thanks @Omega359 there is bunch of things needed to be added, its not a final PR. I'll add some minor missing pieces and the PR will be ready for the review, most likely next week. Because I need to make a cleanup, remove test structures, polish crates and everything.

comphead · 2024-10-18T19:28:10Z

I'll get back on it little later as need to finish some more urgent work with joins

github-actions bot added common Related to common crate functions labels Oct 8, 2024

comphead commented Oct 8, 2024

View reviewed changes

github-actions bot removed the common Related to common crate label Oct 8, 2024

comphead mentioned this pull request Oct 10, 2024

Simple Functions #12635

Open

comphead commented Oct 10, 2024

View reviewed changes

alamb reviewed Oct 11, 2024

View reviewed changes

Omega359 reviewed Oct 11, 2024

View reviewed changes

comphead marked this pull request as draft October 11, 2024 19:44

comphead added 7 commits October 24, 2024 11:25

WIP. Test gen doc from macros

c7ca5c8

WIP. Test gen doc from macros

bbee13b

WIP. Test gen doc from macros

bb3eef3

WIP. Test gen doc from macros

ddba35b

WIP. Test gen doc from macros. to_date

617a418

WIP. Test gen doc from macros. to_date

e03085c

Merge

73a4764

comphead force-pushed the dev1 branch from 48341b3 to 73a4764 Compare October 24, 2024 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Generate docs from macros. #12822

WIP: Generate docs from macros. #12822

comphead commented Oct 8, 2024

comphead commented Oct 8, 2024

comphead Oct 8, 2024

alamb Oct 11, 2024

comphead Oct 8, 2024

alamb Oct 9, 2024

comphead Oct 8, 2024

comphead commented Oct 8, 2024

Omega359 commented Oct 9, 2024

comphead commented Oct 9, 2024

comphead commented Oct 10, 2024 •

edited

Loading

comphead Oct 10, 2024

alamb left a comment

alamb Oct 11, 2024

alamb Oct 11, 2024

Omega359 Oct 11, 2024

alamb Oct 11, 2024

comphead commented Oct 11, 2024 •

edited

Loading

Omega359 commented Oct 11, 2024

Omega359 Oct 11, 2024

comphead Oct 11, 2024

Omega359 Oct 11, 2024

Omega359 Oct 11, 2024

comphead commented Oct 11, 2024

comphead commented Oct 18, 2024

	description = "Proc macros for DataFusion query engine"
	description = "Procedural macros for DataFusion query engine"

WIP: Generate docs from macros. #12822

Are you sure you want to change the base?

WIP: Generate docs from macros. #12822

Conversation

comphead commented Oct 8, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

comphead commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Oct 8, 2024

Omega359 commented Oct 9, 2024

comphead commented Oct 9, 2024

comphead commented Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Oct 11, 2024 • edited Loading

Omega359 commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Oct 11, 2024

comphead commented Oct 18, 2024

comphead commented Oct 10, 2024 •

edited

Loading

comphead commented Oct 11, 2024 •

edited

Loading