alamb commented on code in PR #11948:
URL: https://github.com/apache/datafusion/pull/11948#discussion_r1714423105
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -77,15 +87,18 @@ impl ScalarUDFImpl for LtrimFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(
+ DataType::Utf8 | DataType::Utf8View => make_scalar_function(
ltrim::<i32>,
vec![Hint::Pad, Hint::AcceptsSingular],
)(args),
DataType::LargeUtf8 => make_scalar_function(
ltrim::<i64>,
vec![Hint::Pad, Hint::AcceptsSingular],
)(args),
- other => exec_err!("Unsupported data type {other:?} for function
ltrim"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function ltrim,\
+ expected for Utf8, LargeUtf8 or Utf8View."
Review Comment:
```suggestion
expected Utf8, LargeUtf8 or Utf8View."
```
##########
datafusion/functions/src/string/ltrim.rs:
##########
@@ -51,7 +52,16 @@ impl LtrimFunc {
use DataType::*;
Self {
signature: Signature::one_of(
- vec![Exact(vec![Utf8]), Exact(vec![Utf8, Utf8])],
+ vec![
+ // Planner attempts coercion to the target type starting
with the most preferred candidate.
+ // For example, given input `(Utf8View, Utf8)`, it first
tries coercing to `(Utf8View, Utf8View)`.
+ // If that fails, it proceeds to `(Utf8, Utf8)`.
+ Exact(vec![Utf8View, Utf8View]),
+ // Exact(vec![Utf8, Utf8View]),
Review Comment:
I think we can remove this (leftover?) comment
```suggestion
```
##########
datafusion/sqllogictest/test_files/string_view.slt:
##########
@@ -607,6 +607,97 @@ Xiangpeng Xiangpeng Xiangpeng NULL
Raphael Raphael Raphael NULL
NULL NULL NULL NULL
+## Ensure no casts for LTRIM
+# Test LTRIM with Utf8View input
+query TT
+EXPLAIN SELECT
+ LTRIM(column1_utf8view) AS l
+FROM test;
+----
+logical_plan
+01)Projection: ltrim(test.column1_utf8view) AS l
+02)--TableScan: test projection=[column1_utf8view]
+
+# Test LTRIM with Utf8View input and Utf8View pattern
+query TT
+EXPLAIN SELECT
+ LTRIM(column1_utf8view, 'foo') AS l
+FROM test;
+----
+logical_plan
+01)Projection: ltrim(test.column1_utf8view, Utf8View("foo")) AS l
+02)--TableScan: test projection=[column1_utf8view]
+
+# Test LTRIM with Utf8View bytes longer than 12
+query TT
+EXPLAIN SELECT
+ LTRIM(column1_utf8view, 'this is longer than 12') AS l
+FROM test;
+----
+logical_plan
+01)Projection: ltrim(test.column1_utf8view, Utf8View("this is longer than
12")) AS l
+02)--TableScan: test projection=[column1_utf8view]
+
+# Test LTRIM outputs
+query TTTT
+SELECT
+ LTRIM(column1_utf8view, 'foo') AS l1,
Review Comment:
Could you possible add a test to use a constant that was present too -- like
```sql
LTRIM(column1_utf8view, 'Xiang') AS l1,
```
##########
datafusion/functions/src/string/rtrim.rs:
##########
@@ -77,15 +87,18 @@ impl ScalarUDFImpl for RtrimFunc {
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
- DataType::Utf8 => make_scalar_function(
+ DataType::Utf8 | DataType::Utf8View => make_scalar_function(
rtrim::<i32>,
vec![Hint::Pad, Hint::AcceptsSingular],
)(args),
DataType::LargeUtf8 => make_scalar_function(
rtrim::<i64>,
vec![Hint::Pad, Hint::AcceptsSingular],
)(args),
- other => exec_err!("Unsupported data type {other:?} for function
rtrim"),
+ other => exec_err!(
+ "Unsupported data type {other:?} for function rtrim,\
+ expected for Utf8, LargeUtf8 or Utf8View."
Review Comment:
```suggestion
expected Utf8, LargeUtf8 or Utf8View."
```
##########
datafusion/functions/src/string/rtrim.rs:
##########
@@ -51,7 +52,16 @@ impl RtrimFunc {
use DataType::*;
Self {
signature: Signature::one_of(
- vec![Exact(vec![Utf8]), Exact(vec![Utf8, Utf8])],
+ vec![
+ // Planner attempts coercion to the target type starting
with the most preferred candidate.
+ // For example, given input `(Utf8View, Utf8)`, it first
tries coercing to `(Utf8View, Utf8View)`.
+ // If that fails, it proceeds to `(Utf8, Utf8)`.
+ Exact(vec![Utf8View, Utf8View]),
+ // Exact(vec![Utf8, Utf8View]),
Review Comment:
```suggestion
```
##########
datafusion/sqllogictest/test_files/string_view.slt:
##########
@@ -607,6 +607,97 @@ Xiangpeng Xiangpeng Xiangpeng NULL
Raphael Raphael Raphael NULL
NULL NULL NULL NULL
+## Ensure no casts for LTRIM
+# Test LTRIM with Utf8View input
+query TT
+EXPLAIN SELECT
+ LTRIM(column1_utf8view) AS l
+FROM test;
+----
+logical_plan
+01)Projection: ltrim(test.column1_utf8view) AS l
+02)--TableScan: test projection=[column1_utf8view]
+
+# Test LTRIM with Utf8View input and Utf8View pattern
+query TT
+EXPLAIN SELECT
+ LTRIM(column1_utf8view, 'foo') AS l
+FROM test;
+----
+logical_plan
+01)Projection: ltrim(test.column1_utf8view, Utf8View("foo")) AS l
+02)--TableScan: test projection=[column1_utf8view]
+
+# Test LTRIM with Utf8View bytes longer than 12
+query TT
+EXPLAIN SELECT
+ LTRIM(column1_utf8view, 'this is longer than 12') AS l
+FROM test;
+----
+logical_plan
+01)Projection: ltrim(test.column1_utf8view, Utf8View("this is longer than
12")) AS l
+02)--TableScan: test projection=[column1_utf8view]
+
+# Test LTRIM outputs
+query TTTT
+SELECT
+ LTRIM(column1_utf8view, 'foo') AS l1,
+ LTRIM(column1_utf8view, column2_utf8view) AS l2,
+ LTRIM(column1_utf8view) AS l3,
+ LTRIM(column1_utf8view, NULL) AS l4
+FROM test;
+----
+Andrew Andrew Andrew NULL
+Xiangpeng (empty) Xiangpeng NULL
+Raphael aphael Raphael NULL
+NULL NULL NULL NULL
+
+## ensure no casts for RTRIM
+# Test RTRIM with Utf8View input
+query TT
+EXPLAIN SELECT
+ RTRIM(column1_utf8view) AS l
+FROM test;
+----
+logical_plan
+01)Projection: rtrim(test.column1_utf8view) AS l
+02)--TableScan: test projection=[column1_utf8view]
+
+# Test RTRIM with Utf8View input and Utf8View pattern
+query TT
+EXPLAIN SELECT
+ RTRIM(column1_utf8view, 'foo') AS l
+FROM test;
+----
+logical_plan
+01)Projection: rtrim(test.column1_utf8view, Utf8View("foo")) AS l
+02)--TableScan: test projection=[column1_utf8view]
+
+# Test RTRIM with Utf8View bytes longer than 12
+query TT
+EXPLAIN SELECT
+ RTRIM(column1_utf8view, 'this is longer than 12') AS l
+FROM test;
+----
+logical_plan
+01)Projection: rtrim(test.column1_utf8view, Utf8View("this is longer than
12")) AS l
+02)--TableScan: test projection=[column1_utf8view]
+
+# Test RTRIM outputs
+query TTTT
+SELECT
+ RTRIM(column1_utf8view, 'foo') AS l1,
Review Comment:
Same thing here
```sql
RTRIM(column1_utf8view, 'Hao') AS l1,
```
--
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]
