mbutrovich commented on code in PR #58:
URL: https://github.com/apache/datafusion-spark/pull/58#discussion_r4565078483
##########
crates/spark/src/function/string/substring.rs:
##########
@@ -14,21 +14,52 @@ use datafusion_common::types::{
NativeType, logical_int32, logical_int64, logical_string,
};
use datafusion_common::{Result, exec_err};
-use datafusion_expr::{Coercion, ReturnFieldArgs, TypeSignatureClass};
+use datafusion_expr::{Coercion, Documentation, ReturnFieldArgs,
TypeSignatureClass};
use datafusion_expr::{
ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl, Signature, TypeSignature,
Volatility,
};
use datafusion_functions::unicode::substr::{enable_ascii_fast_path,
get_true_start_end};
use datafusion_functions::utils::make_scalar_function;
+use datafusion_macros::user_doc;
use std::sync::Arc;
/// Spark-compatible `substring` expression
/// <https://spark.apache.org/docs/latest/api/sql/index.html#substring>
-///
-/// Returns the substring from string starting at position pos with length len.
-/// Position is 1-indexed. If pos is negative, it counts from the end of the
string.
-/// Returns NULL if any input is NULL.
+#[user_doc(
+ doc_section(label = "String Functions"),
+ description = "Returns the substring of `str` that starts at `pos` and is
`length` characters long. \
+ Positions are 1-based and count characters (bytes for binary input). A
position of 0 is treated as 1. \
+ A negative position counts from the end of the string; if it reaches
past the start and no length is given, \
+ the whole string is returned. A position past the end, or a length of
0 or less, returns an empty string, \
+ and a length that runs past the end is clamped. Returns NULL if any
input is NULL.",
Review Comment:
The Documentation Guidelines in `CONTRIBUTING.md` call the difference from
the DataFusion built-in the most useful thing to document, and the PR
description says each function covers it. `sha2` does, but `substring` doesn't
mention DataFusion's `substr`, which is registered under the same names in a
combined session.
The main difference is negative positions. DataFusion's `substr` treats a
start below 1 as before the string and subtracts the overshoot from `length`
([`substr.rs`](https://github.com/apache/datafusion/blob/3f197fe0bc6365756bb88ef2e51c01631da913ca/datafusion/functions/src/unicode/substr.rs#L44):
"`substr('abc', -3, 6)` returns `'ab'`"). Spark counts a negative position
from the end of the string, so this function's `substring('Spark SQL', -3)`
returns `SQL`.
Could we add a sentence to the description that says this, so readers of the
function reference can tell the two apart?
--
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]
