User documentation: Generate docs from macros, make`DocumentationBuilder::build` infallable #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?

Moved Document generation framework to separate crate
Created procedural macros crate and macros to generate user docs from rust attributes
Migrated UDF to_date as an example

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

Named it doc-gen as documentation sort of misleading, users gonna think this is a crate user documentation

proc-macros would also be a standard rust name for this kind of hting

I was thinking to make it proc-macro as a crate with all potential procedural macros in DF. Afaik we dont have other custom procedural macros?

We don't have any custom procedural macros, but I was thinking we could / should probably vendor (copy) the recursive one to reduce our dependency chain

I'm not quite following.... you mean copy stacker/recurisive macros introduced recently but this change not related to this PR?

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.

datafusion/macros/Cargo.toml

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

SInce we auto generate the documentation, part of the PR review would be to look at the generated pages. I think we might be able to catch typos at that stage too

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.

Still missing related udfs.

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

comphead · 2024-11-12T01:29:43Z

I'm closer to this

comphead · 2024-11-12T01:31:26Z

We should also take care on handing things like #13367

comphead · 2024-11-18T19:34:04Z

datafusion/functions/src/datetime/to_date.rs

@@ -162,7 +151,7 @@ impl ScalarUDFImpl for ToDateFunc {
    }

    fn documentation(&self) -> Option<&Documentation> {


I dont have better solutions here for now, so every impl have to copy this line because its not easy to inject/override a method into trait impl, I'll experiment more with this, but for now it is what it is

I think as long as it is well documented, this boiler plate is ok

Maybe we could call the function something like macro_doc or derived_documentation to make it easier to find (aka so someone who runs across it can easily grep and find the macro definition)

Co-authored-by: Bruce Ritchie <[email protected]>

comphead · 2024-11-24T17:10:57Z

@Omega359 I'm planning to merge it today, getting more code conflicts, let me know if any objections

Omega359 · 2024-11-24T17:26:03Z

I'm fine with that @comphead - we can file followup issues to add the alternative syntax, etc

comphead · 2024-11-24T20:31:33Z

Filed #13553 to add missing parts

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 force-pushed the dev1 branch from 48341b3 to 73a4764 Compare October 24, 2024 18:29

comphead force-pushed the dev1 branch from 73a4764 to 0b8ab0c Compare November 13, 2024 21:12

github-actions bot added the logical-expr Logical plan and expressions label Nov 15, 2024

comphead force-pushed the dev1 branch from bc9efb0 to 6abf225 Compare November 15, 2024 17:13

comphead commented Nov 18, 2024

View reviewed changes

comphead marked this pull request as ready for review November 18, 2024 19:34

comphead and others added 24 commits November 23, 2024 13:04

WIP. Test gen doc from macros

4635423

WIP. Test gen doc from macros

329c81c

WIP. Test gen doc from macros

0d07ca9

WIP. Test gen doc from macros

17419c2

WIP. Test gen doc from macros. to_date

865df68

WIP. Test gen doc from macros. to_date

48d7829

Merge

28fb501

Move doc gen into separate crate

462775f

Move doc gen into separate crate

86382ea

Move doc gen into separate crate

4895357

Move doc gen into separate crate

7fddde3

Move doc gen into separate crate

d60d356

Move doc gen into separate crate

d46a3a8

Toml fmt

7a7e63e

Toml fmt

09db493

Doc tests

6958012

Comments

ce5f5d8

Comments

b5763a1

doc tests

87cc0d8

doc tests

0c1a884

doc tests

3c9719c

Update datafusion/macros/src/lib.rs

b659a0e

Co-authored-by: Bruce Ritchie <[email protected]>

Update datafusion/macros/src/lib.rs

f2635c8

Co-authored-by: Bruce Ritchie <[email protected]>

Minor: clean up error entries

6115b69

comphead force-pushed the dev1 branch from ce2c1e4 to 6115b69 Compare November 23, 2024 21:10

comphead merged commit 1a6e9f5 into apache:main Nov 24, 2024
27 checks passed

comphead mentioned this pull request Nov 24, 2024

Documentation: Add related_udfs and alternative_syntax to doc gen macros #13553

Open

		@@ -162,7 +151,7 @@ impl ScalarUDFImpl for ToDateFunc {
		}

		fn documentation(&self) -> Option<&Documentation> {

User documentation: Generate docs from macros, makeDocumentationBuilder::build infallable #12822

User documentation: Generate docs from macros, makeDocumentationBuilder::build infallable #12822

Conversation

comphead commented Oct 8, 2024 • edited Loading

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

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

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

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 Nov 12, 2024

comphead commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Nov 24, 2024 • edited Loading

Omega359 commented Nov 24, 2024

comphead commented Nov 24, 2024

User documentation: Generate docs from macros, make`DocumentationBuilder::build` infallable #12822

User documentation: Generate docs from macros, make`DocumentationBuilder::build` infallable #12822

comphead commented Oct 8, 2024 •

edited

Loading

comphead commented Oct 10, 2024 •

edited

Loading

comphead commented Oct 11, 2024 •

edited

Loading

comphead commented Nov 24, 2024 •

edited

Loading