Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: substr_index not handling negative occurrence correctly #9475

Merged
merged 4 commits into from
Mar 9, 2024
Merged

fix: substr_index not handling negative occurrence correctly #9475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce8ee13
fix: `substr_index` not handling negative occurrence correctly
jonahgao Mar 6, 2024
cc2a1c9
format test
jonahgao Mar 6, 2024
57e5faf
add test
jonahgao Mar 6, 2024
6eb7631
add more tests
jonahgao Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 21 additions & 33 deletions datafusion/physical-expr/src/unicode_expressions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,40 +481,28 @@ pub fn substr_index<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {
.zip(count_array.iter())
.map(|((string, delimiter), n)| match (string, delimiter, n) {
(Some(string), Some(delimiter), Some(n)) => {
let mut res = String::new();
match n {
0 => {
"".to_string();
}
_other => {
if n > 0 {
let idx = string
.split(delimiter)
.take(n as usize)
.fold(0, |len, x| len + x.len() + delimiter.len())
- delimiter.len();
res.push_str(if idx >= string.len() {
string
} else {
&string[..idx]
});
} else {
let idx = (string.split(delimiter).take((-n) as usize).fold(
string.len() as isize,
|len, x| {
len - x.len() as isize - delimiter.len() as isize
},
) + delimiter.len() as isize)
as usize;
res.push_str(if idx >= string.len() {
string
} else {
&string[idx..]
});
}
}
// In MySQL, these cases will return an empty string.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like new implementation which looks more concise to me and easier to read.
About the MySQL comments, does that mean some cases are not the same between MySQL and Postgres?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the MySQL comments, does that mean some cases are not the same between MySQL and Postgres?

No, actually there is no such function in PostgreSQL. The closest one is split_part, but their functionalities are different.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it out here too https://www.w3schools.com/mysql/trymysql.asp?filename=trysql_func_mysql_substring_index2

And this behavior is consistent with what I observed

if n == 0 || string.is_empty() || delimiter.is_empty() {
return Some(String::new());
}

let splitted: Box<dyn Iterator<Item = _>> = if n > 0 {
Box::new(string.split(delimiter))
} else {
Box::new(string.rsplit(delimiter))
};
let occurrences = usize::try_from(n.unsigned_abs()).unwrap_or(usize::MAX);
// The length of the substring covered by substr_index.
let length = splitted
.take(occurrences) // at least 1 element, since n != 0
.map(|s| s.len() + delimiter.len())
.sum::<usize>()
- delimiter.len();
if n > 0 {
Some(string[..length].to_owned())
} else {
Some(string[string.len() - length..].to_owned())
}
Some(res)
}
_ => None,
})
Expand Down
190 changes: 120 additions & 70 deletions datafusion/sqllogictest/test_files/functions.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,80 +933,130 @@ SELECT levenshtein(NULL, NULL)
----
NULL

query T
SELECT substr_index('www.apache.org', '.', 1)
----
www

query T
SELECT substr_index('www.apache.org', '.', 2)
----
www.apache

query T
SELECT substr_index('www.apache.org', '.', -1)
----
org

query T
SELECT substr_index('www.apache.org', '.', -2)
----
apache.org

query T
SELECT substr_index('www.apache.org', 'ac', 1)
----
www.ap

query T
SELECT substr_index('www.apache.org', 'ac', -1)
----
he.org

query T
SELECT substr_index('www.apache.org', 'ac', 2)
----
www.apache.org

query T
SELECT substr_index('www.apache.org', 'ac', -2)
----
www.apache.org

query ?
SELECT substr_index(NULL, 'ac', 1)
----
NULL

query T
SELECT substr_index('www.apache.org', NULL, 1)
----
NULL

query T
SELECT substr_index('www.apache.org', 'ac', NULL)
----
NULL

query T
SELECT substr_index('', 'ac', 1)
# Test substring_index using '.' as delimiter
# This query is compatible with MySQL(8.0.19 or later), convenient for comparing results
query TIT
SELECT str, n, substring_index(str, '.', n) AS c FROM
(VALUES
ROW('arrow.apache.org'),
ROW('.'),
ROW('...')
) AS strings(str),
(VALUES
ROW(1),
ROW(2),
ROW(3),
ROW(100),
ROW(-1),
ROW(-2),
ROW(-3),
ROW(-100)
) AS occurrences(n)
ORDER BY str DESC, n;
----
arrow.apache.org -100 arrow.apache.org
arrow.apache.org -3 arrow.apache.org
arrow.apache.org -2 apache.org
arrow.apache.org -1 org
arrow.apache.org 1 arrow
arrow.apache.org 2 arrow.apache
arrow.apache.org 3 arrow.apache.org
arrow.apache.org 100 arrow.apache.org
... -100 ...
... -3 ..
... -2 .
... -1 (empty)
... 1 (empty)
... 2 .
... 3 ..
... 100 ...
. -100 .
. -3 .
. -2 .
. -1 (empty)
. 1 (empty)
. 2 .
. 3 .
. 100 .

# Test substring_index using 'ac' as delimiter
query TIT
SELECT str, n, substring_index(str, 'ac', n) AS c FROM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the CTE style of these tests is very nice 👌

(VALUES
-- input string does not contain the delimiter
ROW('arrow'),
-- input string contains the delimiter
ROW('arrow.apache.org')
) AS strings(str),
(VALUES
ROW(1),
ROW(2),
ROW(-1),
ROW(-2)
) AS occurrences(n)
ORDER BY str DESC, n;
----
arrow.apache.org -2 arrow.apache.org
arrow.apache.org -1 he.org
arrow.apache.org 1 arrow.ap
arrow.apache.org 2 arrow.apache.org
arrow -2 arrow
arrow -1 arrow
arrow 1 arrow
arrow 2 arrow

# Test substring_index with NULL values
query ?TT?
SELECT
substring_index(NULL, '.', 1),
substring_index('arrow.apache.org', NULL, 1),
substring_index('arrow.apache.org', '.', NULL),
substring_index(NULL, NULL, NULL)
----
NULL NULL NULL NULL

# Test substring_index with empty strings
query TT
SELECT
-- input string is empty
substring_index('', '.', 1),
-- delimiter is empty
substring_index('arrow.apache.org', '', 1)
----
(empty) (empty)

# Test substring_index with 0 occurrence
query T
SELECT substring_index('arrow.apache.org', 'ac', 0)
----
(empty)

query T
SELECT substr_index('www.apache.org', '', 1)
----
(empty)
# Test substring_index with large occurrences
query TT
SELECT
-- i64::MIN
substring_index('arrow.apache.org', '.', -9223372036854775808) as c1,
-- i64::MAX
substring_index('arrow.apache.org', '.', 9223372036854775807) as c2;
----
arrow.apache.org arrow.apache.org

# Test substring_index issue https://github.com/apache/arrow-datafusion/issues/9472
query TTT
SELECT
url,
substring_index(url, '.', 1) AS subdomain,
substring_index(url, '.', -1) AS tld
FROM
(VALUES ROW('docs.apache.com'),
ROW('community.influxdata.com'),
ROW('arrow.apache.org')
) data(url)
----
docs.apache.com docs com
community.influxdata.com community com
arrow.apache.org arrow org

query T
SELECT substr_index('www.apache.org', 'ac', 0)
----
(empty)

query ?
SELECT substr_index(NULL, NULL, NULL)
----
NULL

query I
SELECT find_in_set('b', 'a,b,c,d')
Expand Down
Loading