martin-g commented on code in PR #20328:
URL: https://github.com/apache/datafusion/pull/20328#discussion_r2816906070
##########
datafusion/functions/benches/trim.rs:
##########
@@ -221,6 +260,57 @@ fn run_trim_benchmark(
group.finish();
}
+#[expect(clippy::too_many_arguments)]
+fn run_space_trim_benchmark(
+ c: &mut Criterion,
+ group_name: &str,
+ trim_func: &ScalarUDF,
+ trim_type: TrimType,
+ string_types: &[StringArrayType],
+ size: usize,
+ pad_len: usize,
+ remaining_len: usize,
+) {
+ let mut group = c.benchmark_group(group_name);
+ group.sampling_mode(SamplingMode::Flat);
+ group.sample_size(10);
+
+ let total_len = pad_len + remaining_len;
Review Comment:
For Btrim the `pad_len` should be added twice, no ?
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -46,7 +46,7 @@ fn ltrim<T: OffsetSizeTrait>(args: &[ArrayRef]) ->
Result<ArrayRef> {
#[user_doc(
doc_section(label = "String Functions"),
- description = "Trims the specified trim string from the beginning of a
string. If no trim string is provided, all whitespace is removed from the start
of the input string.",
+ description = "Trims the specified trim string from the beginning of a
string. If no trim string is provided, spaces are removed from the start of the
input string.",
Review Comment:
This is a good improvement!
nit: Maybe also add a unit test showing that other whitespaces like \t, \n
are not trimmed.
Same for rtrim and btrim.
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -31,7 +31,7 @@ use datafusion_expr::{
};
use datafusion_macros::user_doc;
-/// Returns the longest string with leading characters removed. If the
characters are not specified, whitespace is removed.
+/// Returns the longest string with leading characters removed. If the
characters are not specified, spaces are removed.
Review Comment:
```suggestion
/// Returns the longest string with leading characters removed. If the
characters are not specified, spaces are removed.
```
##########
datafusion/functions/src/string/rtrim.rs:
##########
@@ -31,7 +31,7 @@ use datafusion_expr::{
};
use datafusion_macros::user_doc;
-/// Returns the longest string with trailing characters removed. If the
characters are not specified, whitespace is removed.
+/// Returns the longest string with trailing characters removed. If the
characters are not specified, spaces are removed.
Review Comment:
```suggestion
/// Returns the longest string with trailing characters removed. If the
characters are not specified, spaces are removed.
```
--
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]
