comphead commented on code in PR #58:
URL: https://github.com/apache/datafusion-spark/pull/58#discussion_r4566025980
##########
crates/spark/src/function/string/substring.rs:
##########
@@ -14,21 +14,55 @@ 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. \
+ Unlike DataFusion's `substr`, which treats a position below 1 as
before the start of the string, \
+ a negative position counts from the end: `substring('abc', -3, 6)`
returns `abc`, where DataFusion's \
+ `substr('abc', -3, 6)` returns `ab`.",
Review Comment:
The contrast with DataFusion reads as if only DataFusion subtracts the
overshoot from `length`. Spark does too: when a negative position reaches past
the start, the part before the start comes off the length
(`UTF8String.substringSQL`). That is why `substr('abc', -10, 3)` is empty,
which `substring.slt` already tests. The real difference is where counting
starts. In Spark, -1 is the last character. In DataFusion, 0 is one before the
first. Could the text say that and drop "and no length is given"? The
`substring('abc', -3, 6)` example also has no `.slt` case yet.
`CONTRIBUTING.md` asks for one, so could we add one?
--
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]
