alamb commented on code in PR #12822:
URL: https://github.com/apache/datafusion/pull/12822#discussion_r1847317488
##########
datafusion/functions/src/datetime/to_date.rs:
##########
@@ -162,7 +151,7 @@ impl ScalarUDFImpl for ToDateFunc {
}
fn documentation(&self) -> Option<&Documentation> {
Review Comment:
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)
##########
datafusion/functions/src/datetime/to_date.rs:
##########
@@ -162,7 +151,7 @@ impl ScalarUDFImpl for ToDateFunc {
}
fn documentation(&self) -> Option<&Documentation> {
Review Comment:
I think as long as it is well documented, this boiler plate is ok
##########
datafusion/expr/src/lib.rs:
##########
@@ -66,6 +65,7 @@ pub mod var_provider;
pub mod window_frame;
pub mod window_state;
+pub use datafusion_doc_gen::{DocSection, Documentation, DocumentationBuilder};
Review Comment:
👍
##########
datafusion/functions/src/datetime/to_date.rs:
##########
@@ -26,9 +26,42 @@ use
datafusion_expr::scalar_doc_sections::DOC_SECTION_DATETIME;
use datafusion_expr::{
ColumnarValue, Documentation, ScalarUDFImpl, Signature, Volatility,
};
+use datafusion_macros::udf_doc;
use std::any::Any;
use std::sync::OnceLock;
+#[udf_doc(
+ doc_section(include = "true", label = "Time and Date Functions"),
Review Comment:
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
##########
datafusion/functions/src/datetime/to_date.rs:
##########
@@ -22,13 +22,46 @@ use arrow::error::ArrowError::ParseError;
use arrow::{array::types::Date32Type, compute::kernels::cast_utils::Parser};
use datafusion_common::error::DataFusionError;
use datafusion_common::{arrow_err, exec_err, internal_datafusion_err, Result};
-use datafusion_expr::scalar_doc_sections::DOC_SECTION_DATETIME;
use datafusion_expr::{
ColumnarValue, Documentation, ScalarUDFImpl, Signature, Volatility,
};
+use datafusion_macros::user_doc;
use std::any::Any;
use std::sync::OnceLock;
+#[user_doc(
+ doc_section(include = "true", label = "Time and Date Functions"),
+ description = r"Converts a value to a date (`YYYY-MM-DD`).
+Supports strings, integer and double types as input.
+Strings are parsed as YYYY-MM-DD (e.g. '2023-07-20') if no [Chrono
format](https://docs.rs/chrono/latest/chrono/format/strftime/index.html)s are
provided.
+Integers and doubles are interpreted as days since the unix epoch
(`1970-01-01T00:00:00Z`).
+Returns the corresponding date.
+
+Note: `to_date` returns Date32, which represents its values as the number of
days since unix epoch(`1970-01-01`) stored as signed 32 bit value. The largest
supported date value is `9999-12-31`.",
+ syntax_example = "to_date('2017-05-31', '%Y-%m-%d')",
Review Comment:
this is pretty fancy
##########
datafusion/macros/src/lib.rs:
##########
@@ -0,0 +1,154 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+extern crate proc_macro;
+use proc_macro::TokenStream;
+use quote::quote;
+use syn::{parse_macro_input, DeriveInput, LitStr};
+
+#[proc_macro_attribute]
+pub fn user_doc(args: TokenStream, input: TokenStream) -> TokenStream {
Review Comment:
I think this macro needs user facing documentation (I realize that is quite
meta 😆 ). Specifically a
1. Introduction of what it does
2. A list of the optional arguments (`doc_section_include`,
`doc_section_lbl`, etc) and examples
3. That it creates a standard `impl ... ` with a `doc_function`
It would also be really neat to make a doc example of it in use, if possible
##########
datafusion/doc-gen/src/lib.rs:
##########
@@ -190,7 +177,7 @@ impl DocumentationBuilder {
self
}
- pub fn build(self) -> Result<Documentation> {
+ pub fn build(self) -> Documentation {
Review Comment:
Perhaps it is worth documenting that this will panic if doc section,
description, or syntax example is missing
##########
datafusion/doc-gen/Cargo.toml:
##########
@@ -0,0 +1,35 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[package]
+name = "datafusion-doc-gen"
Review Comment:
Since this holds `Documentation`, it seems like a name like `datafusion-doc`
might be a name with more clarity
It would also likely help to explain why this needs to be in its own crate
(so the macros can use it?)
##########
datafusion/doc-gen/src/lib.rs:
##########
@@ -15,22 +15,6 @@
// specific language governing permissions and limitations
// under the License.
-use datafusion_common::exec_err;
-use datafusion_common::Result;
-
-/// Documentation for use by [`ScalarUDFImpl`](crate::ScalarUDFImpl),
-/// [`AggregateUDFImpl`](crate::AggregateUDFImpl) and
[`WindowUDFImpl`](crate::WindowUDFImpl) functions
-/// that will be used to generate public documentation.
-///
-/// The name of the udf will be pulled from the
[`ScalarUDFImpl::name`](crate::ScalarUDFImpl::name),
-/// [`AggregateUDFImpl::name`](crate::AggregateUDFImpl::name) or
[`WindowUDFImpl::name`](crate::WindowUDFImpl::name)
-/// function as appropriate.
-///
-/// All strings in the documentation are required to be
Review Comment:
I think leaving information about markdown is still valuable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
